node: fix a bug where nil is recorded as node's address (#3740)

* node: fix a bug where `nil` is recorded as node's address

Solution

  AddOurAddress when we know it
  sw.NetAddress is nil in createAddrBookAndSetOnSwitch
  it's set by n.transport.Listen function, which is called during start

Fixes #3716

* use addr instead of n.sw.NetAddress

* add both ExternalAddress and ListenAddress as our addresses

CHANGELOG_PENDING.md

@@ -4,26 +4,25 @@
 
 ### BREAKING CHANGES:
 
-- \#3613 Switch from golang/dep to Go 1.11 Modules to resolve dependencies:
-  - it is recommended to switch to Go Modules if your project has tendermint 
-  as a dependency
-  - read more on Modules here: https://github.com/golang/go/wiki/Modules  
-
 * CLI/RPC/Config
-  * [rpc] \#3616 Improve `/block_results` response format (`results.DeliverTx` ->
-  `results.deliver_tx`). See docs for details.
+  - [cli] \#3613 Switch from golang/dep to Go Modules to resolve dependencies:
+    It is recommended to switch to Go Modules if your project has tendermint as
+    a dependency. Read more on Modules here:
+    https://github.com/golang/go/wiki/Modules
+  - [rpc] \#3616 Improve `/block_results` response format (`results.DeliverTx`
+    -> `results.deliver_tx`). See docs for details.
 
 * Apps
-  * [abci] \#1859 `ResponseCheckTx`, `ResponseDeliverTx`, `ResponseBeginBlock`,
-  and `ResponseEndBlock` now include `Events` instead of `Tags`. Each `Event`
-  contains a `type` and a list of `attributes` (list of key-value pairs) allowing
-  for inclusion of multiple distinct events in each response.
+  - [abci] \#1859 `ResponseCheckTx`, `ResponseDeliverTx`, `ResponseBeginBlock`,
+    and `ResponseEndBlock` now include `Events` instead of `Tags`. Each `Event`
+    contains a `type` and a list of `attributes` (list of key-value pairs)
+    allowing for inclusion of multiple distinct events in each response.
 
 * Go API
-  * [libs/db] Removed deprecated `LevelDBBackend` const
-    * If you have `db_backend` set to `leveldb` in your config file, please
+  - [libs/db] [\#3632](https://github.com/tendermint/tendermint/pull/3632) Removed deprecated `LevelDBBackend` const
+    If you have `db_backend` set to `leveldb` in your config file, please
     change it to `goleveldb` or `cleveldb`.
-- [p2p] \#3521 Remove NewNetAddressStringWithOptionalID
+  - [p2p] \#3521 Remove NewNetAddressStringWithOptionalID
 
 * Blockchain Protocol
 
@@ -32,10 +31,10 @@
 ### FEATURES:
 
 ### IMPROVEMENTS:
-- [p2p] \#3666 Add per channel telemtry to improve reactor observability
-
-* [rpc] [\#3686](https://github.com/tendermint/tendermint/pull/3686) `HTTPClient#Call` returns wrapped errors, so a caller could use `errors.Cause` to retrieve an error code.
+- [p2p] \#3666 Add per channel telemetry to improve reactor observability
+- [rpc] [\#3686](https://github.com/tendermint/tendermint/pull/3686) `HTTPClient#Call` returns wrapped errors, so a caller could use `errors.Cause` to retrieve an error code. (@wooparadog)
 
 ### BUG FIXES:
-- [libs/db] Fixed the BoltDB backend's Batch.Delete implementation (@Yawning)
-- [libs/db] Fixed the BoltDB backend's Get and Iterator implementation (@Yawning)
+- [libs/db] \#3717 Fixed the BoltDB backend's Batch.Delete implementation (@Yawning)
+- [libs/db] \#3718 Fixed the BoltDB backend's Get and Iterator implementation (@Yawning)
+- [node] \#3716 Fix a bug where `nil` is recorded as node's address

docs/.vuepress/config.js

@@ -44,7 +44,7 @@ module.exports = {
           "/app-dev/app-development",
           "/app-dev/subscribing-to-events-via-websocket",
           "/app-dev/indexing-transactions",
-          "/app-dev/abci-spec",
+          "/spec/abci/abci",
           "/app-dev/ecosystem"
         ]
       },

docs/app-dev/app-development.md

@@ -133,8 +133,8 @@ the mempool. If Tendermint is just started or the clients sent more than
 100k transactions, old transactions may be sent to the application. So
 it is important CheckTx implements some logic to handle them.
 
-There are cases where a transaction will (or may) become valid in some
-future state, in which case you probably want to disable Tendermint's
+If there are cases in your application where a transaction may become invalid in some
+future state, you probably want to disable Tendermint's
 cache. You can do that by setting `[mempool] cache_size = 0` in the
 config.
 
@@ -205,7 +205,7 @@ Once all processing of the block is complete, Tendermint sends the
 Commit request and blocks waiting for a response. While the mempool may
 run concurrently with block processing (the BeginBlock, DeliverTxs, and
 EndBlock), it is locked for the Commit request so that its state can be
-safely reset during Commit. This means the app _MUST NOT_ do any
+safely updated during Commit. This means the app _MUST NOT_ do any
 blocking communication with the mempool (ie. broadcast_tx) during
 Commit, or there will be deadlock. Note also that all remaining
 transactions in the mempool are replayed on the mempool connection

docs/app-dev/getting-started.md

@@ -137,7 +137,7 @@ The result should look like:
 Note the `value` in the result (`YWJjZA==`); this is the base64-encoding
 of the ASCII of `abcd`. You can verify this in a python 2 shell by
 running `"YWJjZA==".decode('base64')` or in python 3 shell by running
-`import codecs; codecs.decode("YWJjZA==", 'base64').decode('ascii')`.
+`import codecs; codecs.decode(b"YWJjZA==", 'base64').decode('ascii')`.
 Stay tuned for a future release that [makes this output more
 human-readable](https://github.com/tendermint/tendermint/issues/1794).
 

docs/spec/abci/abci.md

@@ -141,7 +141,7 @@ on them. All other fields in the `Response*` must be strictly deterministic.
 ## Block Execution
 
 The first time a new blockchain is started, Tendermint calls
-`InitChain`. From then on, the follow sequence of methods is executed for each
+`InitChain`. From then on, the following sequence of methods is executed for each
 block:
 
 `BeginBlock, [DeliverTx], EndBlock, Commit`

docs/spec/blockchain/encoding.md

@@ -218,7 +218,7 @@ func MerkleRoot(items [][]byte) []byte{
 	case 0:
 		return nil
 	case 1:
-		return leafHash(leafs[0])
+		return leafHash(items[0])
 	default:
 		k := getSplitPoint(len(items))
 		left := MerkleRoot(items[:k])

docs/spec/blockchain/state.md

@@ -59,7 +59,7 @@ type Validator struct {
 When hashing the Validator struct, the address is not included,
 because it is redundant with the pubkey.
 
-The `state.Validators`, `state.LastValidators`, and `state.NextValidators`, must always by sorted by validator address,
+The `state.Validators`, `state.LastValidators`, and `state.NextValidators`, must always be sorted by validator address,
 so that there is a canonical order for computing the MerkleRoot.
 
 We also define a `TotalVotingPower` function, to return the total voting power:

docs/tendermint-core/using-tendermint.md

@@ -202,8 +202,10 @@ Note that raw hex cannot be used in `POST` transactions.
 
 ## Reset
 
-**WARNING: UNSAFE** Only do this in development and only if you can
+::: warning
+**UNSAFE** Only do this in development and only if you can
 afford to lose all blockchain data!
+:::
 
 To reset a blockchain, stop the node and run:
 

node/node.go

@@ -441,17 +441,30 @@ func createSwitch(config *cfg.Config,
 }
 
 func createAddrBookAndSetOnSwitch(config *cfg.Config, sw *p2p.Switch,
-	p2pLogger log.Logger) pex.AddrBook {
+	p2pLogger log.Logger, nodeKey *p2p.NodeKey) (pex.AddrBook, error) {
 
 	addrBook := pex.NewAddrBook(config.P2P.AddrBookFile(), config.P2P.AddrBookStrict)
 	addrBook.SetLogger(p2pLogger.With("book", config.P2P.AddrBookFile()))
 
 	// Add ourselves to addrbook to prevent dialing ourselves
-	addrBook.AddOurAddress(sw.NetAddress())
+	if config.P2P.ExternalAddress != "" {
+		addr, err := p2p.NewNetAddressString(p2p.IDAddressString(nodeKey.ID(), config.P2P.ExternalAddress))
+		if err != nil {
+			return nil, errors.Wrap(err, "p2p.external_address is incorrect")
+		}
+		addrBook.AddOurAddress(addr)
+	}
+	if config.P2P.ListenAddress != "" {
+		addr, err := p2p.NewNetAddressString(p2p.IDAddressString(nodeKey.ID(), config.P2P.ListenAddress))
+		if err != nil {
+			return nil, errors.Wrap(err, "p2p.laddr is incorrect")
+		}
+		addrBook.AddOurAddress(addr)
+	}
 
 	sw.SetAddrBook(addrBook)
 
-	return addrBook
+	return addrBook, nil
 }
 
 func createPEXReactorAndAddToSwitch(addrBook pex.AddrBook, config *cfg.Config,
@@ -594,7 +607,10 @@ func NewNode(config *cfg.Config,
 		return nil, errors.Wrap(err, "could not add peers from persistent_peers field")
 	}
 
-	addrBook := createAddrBookAndSetOnSwitch(config, sw, p2pLogger)
+	addrBook, err := createAddrBookAndSetOnSwitch(config, sw, p2pLogger, nodeKey)
+	if err != nil {
+		return nil, errors.Wrap(err, "could not create addrbook")
+	}
 
 	// Optionally, start the pex reactor
 	//
@@ -714,7 +730,6 @@ func (n *Node) OnStop() {
 	n.indexerService.Stop()
 
 	// now stop the reactors
-	// TODO: gracefully disconnect from peers.
 	n.sw.Stop()
 
 	// stop mempool WAL

p2p/switch.go

@@ -221,11 +221,7 @@ func (sw *Switch) OnStart() error {
 func (sw *Switch) OnStop() {
 	// Stop peers
 	for _, p := range sw.peers.List() {
-		sw.transport.Cleanup(p)
-		p.Stop()
-		if sw.peers.Remove(p) {
-			sw.metrics.Peers.Add(float64(-1))
-		}
+		sw.stopAndRemovePeer(p, nil)
 	}
 
 	// Stop reactors

types/event_bus.go

@@ -130,8 +130,7 @@ func (b *EventBus) PublishEventNewBlock(data EventDataNewBlock) error {
 	// add predefined new block event
 	events[EventTypeKey] = append(events[EventTypeKey], EventNewBlock)
 
-	_ = b.pubsub.PublishWithEvents(ctx, data, events)
-	return nil
+	return b.pubsub.PublishWithEvents(ctx, data, events)
 }
 
 func (b *EventBus) PublishEventNewBlockHeader(data EventDataNewBlockHeader) error {
@@ -170,8 +169,7 @@ func (b *EventBus) PublishEventTx(data EventDataTx) error {
 	events[TxHashKey] = append(events[TxHashKey], fmt.Sprintf("%X", data.Tx.Hash()))
 	events[TxHeightKey] = append(events[TxHeightKey], fmt.Sprintf("%d", data.Height))
 
-	_ = b.pubsub.PublishWithEvents(ctx, data, events)
-	return nil
+	return b.pubsub.PublishWithEvents(ctx, data, events)
 }
 
 func (b *EventBus) PublishEventNewRoundStep(data EventDataRoundState) error {