From 52e21cebcfe65522f629b457e39b9dc8b2c30297 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 30 Sep 2018 13:28:34 -0400 Subject: [PATCH] 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 | 1 + config/config.go | 14 ++++++-------- config/toml.go | 1 - consensus/types/round_state.go | 4 ++-- docs/spec/reactors/mempool/config.md | 11 +++-------- docs/tendermint-core/configuration.md | 7 +++---- mempool/mempool.go | 4 +--- node/node.go | 1 - privval/priv_validator.go | 6 ++++-- types/params.go | 2 -- types/vote.go | 4 ++-- 11 files changed, 22 insertions(+), 33 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index bf381dce2..bca7ba478 100644 --- a/CHANGELOG_PENDING.md +++ b/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` diff --git a/config/config.go b/config/config.go index 619c0410f..8ff800053 100644 --- a/config/config.go +++ b/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"` - Recheck bool `mapstructure:"recheck"` - RecheckEmpty bool `mapstructure:"recheck_empty"` - Broadcast bool `mapstructure:"broadcast"` - WalPath string `mapstructure:"wal_dir"` - Size int `mapstructure:"size"` - CacheSize int `mapstructure:"cache_size"` + RootDir string `mapstructure:"home"` + Recheck bool `mapstructure:"recheck"` + Broadcast bool `mapstructure:"broadcast"` + WalPath string `mapstructure:"wal_dir"` + Size int `mapstructure:"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 diff --git a/config/toml.go b/config/toml.go index 846b33d16..ddfe5f055 100644 --- a/config/toml.go +++ b/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 }}" diff --git a/consensus/types/round_state.go b/consensus/types/round_state.go index c22880c2b..d3f6468bf 100644 --- a/consensus/types/round_state.go +++ b/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 - // if we want to avoid this, we may need synchronous events after all + // copy the RoundState. + // TODO: if we want to avoid this, we may need synchronous events after all rsCopy := *rs edrs := types.EventDataRoundState{ Height: rs.Height, diff --git a/docs/spec/reactors/mempool/config.md b/docs/spec/reactors/mempool/config.md index 3e3c0d373..4fb756fa4 100644 --- a/docs/spec/reactors/mempool/config.md +++ b/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) diff --git a/docs/tendermint-core/configuration.md b/docs/tendermint-core/configuration.md index c5b07497c..8b3c3c22f 100644 --- a/docs/tendermint-core/configuration.md +++ b/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). diff --git a/mempool/mempool.go b/mempool/mempool.go index 2096912f5..db5f6160c 100644 --- a/mempool/mempool.go +++ b/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, - // so we really still do need to recheck, but this is for debugging - if mem.config.Recheck && (mem.config.RecheckEmpty || len(goodTxs) > 0) { + if mem.config.Recheck && len(goodTxs) > 0 { mem.logger.Info("Recheck txs", "numtxs", len(goodTxs), "height", height) mem.recheckTxs(goodTxs) // At this point, mem.txs are being rechecked. diff --git a/node/node.go b/node/node.go index bba4dbda5..9f9e3636f 100644 --- a/node/node.go +++ b/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, diff --git a/privval/priv_validator.go b/privval/priv_validator.go index 8091744ce..e606b826a 100644 --- a/privval/priv_validator.go +++ b/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? - LastSignBytes cmn.HexBytes `json:"last_signbytes,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"` PrivKey crypto.PrivKey `json:"priv_key"` // For persistence. diff --git a/types/params.go b/types/params.go index a7301d063..014694ccb 100644 --- a/types/params.go +++ b/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 diff --git a/types/vote.go b/types/vote.go index ba2f1dfe4..5a31f0e2b 100644 --- a/types/vote.go +++ b/types/vote.go @@ -61,8 +61,8 @@ func IsVoteTypeValid(type_ byte) bool { } } -// Address is hex bytes. TODO: crypto.Address -type Address = cmn.HexBytes +// Address is hex bytes. +type Address = crypto.Address // Represents a prevote, precommit, or commit vote from validators for consensus. type Vote struct {