remove some xxx comments and the config.mempool.recheck_empty (#2505)

* remove some XXX

* config: remove Mempool.RecheckEmpty

* docs: remove recheck_empty

CHANGELOG_PENDING.md

@@ -5,6 +5,7 @@ Special thanks to external contributors on this release:
 BREAKING CHANGES:
 
 * CLI/RPC/Config
+  * [config] \#2505 Remove Mempool.RecheckEmpty (it was effectively useless anyways)
   * [config] `mempool.wal` is disabled by default
   * [rpc] \#2298 `/abci_query` takes `prove` argument instead of `trusted` and switches the default
     behaviour to `prove=false`

config/config.go

@@ -488,20 +488,18 @@ func DefaultFuzzConnConfig() *FuzzConnConfig {
 
 // MempoolConfig defines the configuration options for the Tendermint mempool
 type MempoolConfig struct {
-	RootDir      string `mapstructure:"home"`
+	RootDir   string `mapstructure:"home"`
-	Recheck      bool   `mapstructure:"recheck"`
+	Recheck   bool   `mapstructure:"recheck"`
-	RecheckEmpty bool   `mapstructure:"recheck_empty"`
+	Broadcast bool   `mapstructure:"broadcast"`
-	Broadcast    bool   `mapstructure:"broadcast"`
+	WalPath   string `mapstructure:"wal_dir"`
-	WalPath      string `mapstructure:"wal_dir"`
+	Size      int    `mapstructure:"size"`
-	Size         int    `mapstructure:"size"`
+	CacheSize int    `mapstructure:"cache_size"`
-	CacheSize    int    `mapstructure:"cache_size"`
 }
 
 // DefaultMempoolConfig returns a default configuration for the Tendermint mempool
 func DefaultMempoolConfig() *MempoolConfig {
 	return &MempoolConfig{
 		Recheck:      true,
-		RecheckEmpty: true,
 		Broadcast:    true,
 		WalPath:      "",
 		// Each signature verification takes .5ms, size reduced until we implement

config/toml.go

@@ -213,7 +213,6 @@ dial_timeout = "{{ .P2P.DialTimeout }}"
 [mempool]
 
 recheck = {{ .Mempool.Recheck }}
-recheck_empty = {{ .Mempool.RecheckEmpty }}
 broadcast = {{ .Mempool.Broadcast }}
 wal_dir = "{{ js .Mempool.WalPath }}"
 

consensus/types/round_state.go

@@ -107,8 +107,8 @@ func (rs *RoundState) RoundStateSimple() RoundStateSimple {
 
 // RoundStateEvent returns the H/R/S of the RoundState as an event.
 func (rs *RoundState) RoundStateEvent() types.EventDataRoundState {
-	// XXX: copy the RoundState
+	// copy the RoundState.
-	// if we want to avoid this, we may need synchronous events after all
+	// TODO: if we want to avoid this, we may need synchronous events after all
 	rsCopy := *rs
 	edrs := types.EventDataRoundState{
 		Height:     rs.Height,

docs/spec/reactors/mempool/config.md

@@ -6,23 +6,21 @@ as command-line flags, but they can also be passed in as
 environmental variables or in the config.toml file. The
 following are all equivalent:
 
-Flag: `--mempool.recheck_empty=false`
+Flag: `--mempool.recheck=false`
 
-Environment: `TM_MEMPOOL_RECHECK_EMPTY=false`
+Environment: `TM_MEMPOOL_RECHECK=false`
 
 Config:
 
 ```
 [mempool]
-recheck_empty = false
+recheck = false
 ```
 
 ## Recheck
 
 `--mempool.recheck=false` (default: true)
 
-`--mempool.recheck_empty=false` (default: true)
-
 Recheck determines if the mempool rechecks all pending
 transactions after a block was committed. Once a block
 is committed, the mempool removes all valid transactions
@@ -31,9 +29,6 @@ that were successfully included in the block.
 If `recheck` is true, then it will rerun CheckTx on
 all remaining transactions with the new block state.
 
-If the block contained no transactions, it will skip the
-recheck unless `recheck_empty` is true.
-
 ## Broadcast
 
 `--mempool.broadcast=false` (default: true)

docs/tendermint-core/configuration.md

@@ -156,7 +156,6 @@ dial_timeout = "3s"
 [mempool]
 
 recheck = true
-recheck_empty = true
 broadcast = true
 wal_dir = "data/mempool.wal"
 
@@ -203,15 +202,15 @@ indexer = "kv"
 # Comma-separated list of tags to index (by default the only tag is "tx.hash")
 #
 # You can also index transactions by height by adding "tx.height" tag here.
-# 
+#
 # It's recommended to index only a subset of tags due to possible memory
 # bloat. This is, of course, depends on the indexer's DB and the volume of
 # transactions.
 index_tags = ""
 
 # When set to true, tells indexer to index all tags (predefined tags:
-# "tx.hash", "tx.height" and all tags from DeliverTx responses). 
+# "tx.hash", "tx.height" and all tags from DeliverTx responses).
-#	
+#
 # Note this may be not desirable (see the comment above). IndexTags has a
 # precedence over IndexAllTags (i.e. when given both, IndexTags will be
 # indexed).

mempool/mempool.go

@@ -513,9 +513,7 @@ func (mem *Mempool) Update(
 	// Remove transactions that are already in txs.
 	goodTxs := mem.filterTxs(txsMap)
 	// Recheck mempool txs if any txs were committed in the block
-	// NOTE/XXX: in some apps a tx could be invalidated due to EndBlock,
+	if mem.config.Recheck && len(goodTxs) > 0 {
-	//	so we really still do need to recheck, but this is for debugging
-	if mem.config.Recheck && (mem.config.RecheckEmpty || len(goodTxs) > 0) {
 		mem.logger.Info("Recheck txs", "numtxs", len(goodTxs), "height", height)
 		mem.recheckTxs(goodTxs)
 		// At this point, mem.txs are being rechecked.

node/node.go

@@ -359,7 +359,6 @@ func NewNode(config *cfg.Config,
 
 	// Filter peers by addr or pubkey with an ABCI query.
 	// If the query return code is OK, add peer.
-	// XXX: Query format subject to change
 	if config.FilterPeers {
 		connFilters = append(
 			connFilters,

privval/priv_validator.go

@@ -38,14 +38,16 @@ func voteToStep(vote *types.Vote) int8 {
 // FilePV implements PrivValidator using data persisted to disk
 // to prevent double signing.
 // NOTE: the directory containing the pv.filePath must already exist.
+// It includes the LastSignature and LastSignBytes so we don't lose the signature
+// if the process crashes after signing but before the resulting consensus message is processed.
 type FilePV struct {
 	Address       types.Address  `json:"address"`
 	PubKey        crypto.PubKey  `json:"pub_key"`
 	LastHeight    int64          `json:"last_height"`
 	LastRound     int            `json:"last_round"`
 	LastStep      int8           `json:"last_step"`
-	LastSignature []byte         `json:"last_signature,omitempty"` // so we dont lose signatures XXX Why would we lose signatures?
+	LastSignature []byte         `json:"last_signature,omitempty"`
-	LastSignBytes cmn.HexBytes   `json:"last_signbytes,omitempty"` // so we dont lose signatures XXX Why would we lose signatures?
+	LastSignBytes cmn.HexBytes   `json:"last_signbytes,omitempty"`
 	PrivKey       crypto.PrivKey `json:"priv_key"`
 
 	// For persistence.

types/params.go

@@ -99,8 +99,6 @@ func (params ConsensusParams) Update(params2 *abci.ConsensusParams) ConsensusPar
 	}
 
 	// we must defensively consider any structs may be nil
-	// XXX: it's cast city over here. It's ok because we only do int32->int
-	// but still, watch it champ.
 	if params2.BlockSize != nil {
 		res.BlockSize.MaxBytes = params2.BlockSize.MaxBytes
 		res.BlockSize.MaxGas = params2.BlockSize.MaxGas

types/vote.go

@@ -61,8 +61,8 @@ func IsVoteTypeValid(type_ byte) bool {
 	}
 }
 
-// Address is hex bytes. TODO: crypto.Address
+// Address is hex bytes.
-type Address = cmn.HexBytes
+type Address = crypto.Address
 
 // Represents a prevote, precommit, or commit vote from validators for consensus.
 type Vote struct {