From a67ae814698b76206049e378a9ed64e7a2ed3832 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Thu, 25 Oct 2018 16:40:20 +0200 Subject: [PATCH 01/17] if some process locks a block in round 0, then 0 is valid proposal.POLRound in rounds > 0 This condition is really hard to get. Initially, lockedRound and validRound are set to -1 as we start with round 0. Refs #2702 --- consensus/state.go | 6 +++--- types/vote_set.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/consensus/state.go b/consensus/state.go index 0b079f13d..c6efe98f0 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -1425,9 +1425,9 @@ func (cs *ConsensusState) defaultSetProposal(proposal *types.Proposal) error { return nil } - // Verify POLRound, which must be -1 or between 0 and proposal.Round exclusive. - if proposal.POLRound != -1 && - (proposal.POLRound < 0 || proposal.Round <= proposal.POLRound) { + // Verify POLRound, which must be -1 or between 0 inclusive and proposal.Round exclusive. + if proposal.POLRound < -1 || + (proposal.POLRound >= 0 && proposal.POLRound >= proposal.Round) { return ErrInvalidProposalPOLRound } diff --git a/types/vote_set.go b/types/vote_set.go index cdfa3d40d..0cf6cbb7f 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -158,7 +158,7 @@ func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { if (vote.Height != voteSet.height) || (vote.Round != voteSet.round) || (vote.Type != voteSet.type_) { - return false, errors.Wrapf(ErrVoteUnexpectedStep, "Got %d/%d/%d, expected %d/%d/%d", + return false, errors.Wrapf(ErrVoteUnexpectedStep, "Expected %d/%d/%d, but got %d/%d/%d", voteSet.height, voteSet.round, voteSet.type_, vote.Height, vote.Round, vote.Type) } From 7246ffc48fa5a45daa5acddcf5b726b7bb6f259c Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Mon, 5 Nov 2018 22:32:52 -0800 Subject: [PATCH 02/17] mempool: ErrPreCheck and more log info (#2724) * mempool: ErrPreCheck and more log info * change Pre/PostCheckFunc to return errors also, continue execution when checking txs in mempool_test if err=PreCheckErr --- mempool/mempool.go | 62 +++++++++++++++++++++++++++++++++-------- mempool/mempool_test.go | 54 +++++++++++++---------------------- 2 files changed, 69 insertions(+), 47 deletions(-) diff --git a/mempool/mempool.go b/mempool/mempool.go index 65cd55354..5e52989aa 100644 --- a/mempool/mempool.go +++ b/mempool/mempool.go @@ -25,12 +25,12 @@ import ( // PreCheckFunc is an optional filter executed before CheckTx and rejects // transaction if false is returned. An example would be to ensure that a // transaction doesn't exceeded the block size. -type PreCheckFunc func(types.Tx) bool +type PreCheckFunc func(types.Tx) error // PostCheckFunc is an optional filter executed after CheckTx and rejects // transaction if false is returned. An example would be to ensure a // transaction doesn't require more gas than available for the block. -type PostCheckFunc func(types.Tx, *abci.ResponseCheckTx) bool +type PostCheckFunc func(types.Tx, *abci.ResponseCheckTx) error /* @@ -68,24 +68,48 @@ var ( ErrMempoolIsFull = errors.New("Mempool is full") ) +// ErrPreCheck is returned when tx is too big +type ErrPreCheck struct { + Reason error +} + +func (e ErrPreCheck) Error() string { + return e.Reason.Error() +} + +// IsPreCheckError returns true if err is due to pre check failure. +func IsPreCheckError(err error) bool { + _, ok := err.(ErrPreCheck) + return ok +} + // PreCheckAminoMaxBytes checks that the size of the transaction plus the amino // overhead is smaller or equal to the expected maxBytes. func PreCheckAminoMaxBytes(maxBytes int64) PreCheckFunc { - return func(tx types.Tx) bool { + return func(tx types.Tx) error { // We have to account for the amino overhead in the tx size as well aminoOverhead := amino.UvarintSize(uint64(len(tx))) - return int64(len(tx)+aminoOverhead) <= maxBytes + txSize := int64(len(tx) + aminoOverhead) + if txSize > maxBytes { + return fmt.Errorf("Tx size (including amino overhead) is too big: %d, max: %d", + txSize, maxBytes) + } + return nil } } // PostCheckMaxGas checks that the wanted gas is smaller or equal to the passed -// maxGas. Returns true if maxGas is -1. +// maxGas. Returns nil if maxGas is -1. func PostCheckMaxGas(maxGas int64) PostCheckFunc { - return func(tx types.Tx, res *abci.ResponseCheckTx) bool { + return func(tx types.Tx, res *abci.ResponseCheckTx) error { if maxGas == -1 { - return true + return nil + } + if res.GasWanted > maxGas { + return fmt.Errorf("gas wanted %d is greater than max gas %d", + res.GasWanted, maxGas) } - return res.GasWanted <= maxGas + return nil } } @@ -285,8 +309,10 @@ func (mem *Mempool) CheckTx(tx types.Tx, cb func(*abci.Response)) (err error) { return ErrMempoolIsFull } - if mem.preCheck != nil && !mem.preCheck(tx) { - return + if mem.preCheck != nil { + if err := mem.preCheck(tx); err != nil { + return ErrPreCheck{err} + } } // CACHE @@ -346,7 +372,13 @@ func (mem *Mempool) resCbNormal(req *abci.Request, res *abci.Response) { tx: tx, } mem.txs.PushBack(memTx) - mem.logger.Info("Added good transaction", "tx", TxID(tx), "res", r, "total", mem.Size()) + mem.logger.Info("Added good transaction", + "tx", TxID(tx), + "res", r, + "height", memTx.height, + "total", mem.Size(), + "counter", memTx.counter, + ) mem.metrics.TxSizeBytes.Observe(float64(len(tx))) mem.notifyTxsAvailable() } else { @@ -566,7 +598,13 @@ func (mem *Mempool) recheckTxs(goodTxs []types.Tx) { } func (mem *Mempool) isPostCheckPass(tx types.Tx, r *abci.ResponseCheckTx) bool { - return mem.postCheck == nil || mem.postCheck(tx, r) + if mem.postCheck == nil { + return true + } + if err := mem.postCheck(tx, r); err != nil { + return false + } + return true } //-------------------------------------------------------------------------------- diff --git a/mempool/mempool_test.go b/mempool/mempool_test.go index 5aabd00ee..44917afba 100644 --- a/mempool/mempool_test.go +++ b/mempool/mempool_test.go @@ -14,7 +14,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - amino "github.com/tendermint/go-amino" "github.com/tendermint/tendermint/abci/example/counter" "github.com/tendermint/tendermint/abci/example/kvstore" abci "github.com/tendermint/tendermint/abci/types" @@ -66,7 +65,10 @@ func checkTxs(t *testing.T, mempool *Mempool, count int) types.Txs { t.Error(err) } if err := mempool.CheckTx(txBytes, nil); err != nil { - t.Fatalf("Error after CheckTx: %v", err) + if IsPreCheckError(err) { + continue + } + t.Fatalf("CheckTx failed: %v while checking #%d tx", err, i) } } return txs @@ -126,47 +128,29 @@ func TestMempoolFilters(t *testing.T) { mempool := newMempoolWithApp(cc) emptyTxArr := []types.Tx{[]byte{}} - nopPreFilter := func(tx types.Tx) bool { return true } - nopPostFilter := func(tx types.Tx, res *abci.ResponseCheckTx) bool { return true } - - // This is the same filter we expect to be used within node/node.go and state/execution.go - nBytePreFilter := func(n int) func(tx types.Tx) bool { - return func(tx types.Tx) bool { - // We have to account for the amino overhead in the tx size as well - aminoOverhead := amino.UvarintSize(uint64(len(tx))) - return (len(tx) + aminoOverhead) <= n - } - } - - nGasPostFilter := func(n int64) func(tx types.Tx, res *abci.ResponseCheckTx) bool { - return func(tx types.Tx, res *abci.ResponseCheckTx) bool { - if n == -1 { - return true - } - return res.GasWanted <= n - } - } + nopPreFilter := func(tx types.Tx) error { return nil } + nopPostFilter := func(tx types.Tx, res *abci.ResponseCheckTx) error { return nil } // each table driven test creates numTxsToCreate txs with checkTx, and at the end clears all remaining txs. // each tx has 20 bytes + amino overhead = 21 bytes, 1 gas tests := []struct { numTxsToCreate int - preFilter func(tx types.Tx) bool - postFilter func(tx types.Tx, res *abci.ResponseCheckTx) bool + preFilter PreCheckFunc + postFilter PostCheckFunc expectedNumTxs int }{ {10, nopPreFilter, nopPostFilter, 10}, - {10, nBytePreFilter(10), nopPostFilter, 0}, - {10, nBytePreFilter(20), nopPostFilter, 0}, - {10, nBytePreFilter(21), nopPostFilter, 10}, - {10, nopPreFilter, nGasPostFilter(-1), 10}, - {10, nopPreFilter, nGasPostFilter(0), 0}, - {10, nopPreFilter, nGasPostFilter(1), 10}, - {10, nopPreFilter, nGasPostFilter(3000), 10}, - {10, nBytePreFilter(10), nGasPostFilter(20), 0}, - {10, nBytePreFilter(30), nGasPostFilter(20), 10}, - {10, nBytePreFilter(21), nGasPostFilter(1), 10}, - {10, nBytePreFilter(21), nGasPostFilter(0), 0}, + {10, PreCheckAminoMaxBytes(10), nopPostFilter, 0}, + {10, PreCheckAminoMaxBytes(20), nopPostFilter, 0}, + {10, PreCheckAminoMaxBytes(21), nopPostFilter, 10}, + {10, nopPreFilter, PostCheckMaxGas(-1), 10}, + {10, nopPreFilter, PostCheckMaxGas(0), 0}, + {10, nopPreFilter, PostCheckMaxGas(1), 10}, + {10, nopPreFilter, PostCheckMaxGas(3000), 10}, + {10, PreCheckAminoMaxBytes(10), PostCheckMaxGas(20), 0}, + {10, PreCheckAminoMaxBytes(30), PostCheckMaxGas(20), 10}, + {10, PreCheckAminoMaxBytes(21), PostCheckMaxGas(1), 10}, + {10, PreCheckAminoMaxBytes(21), PostCheckMaxGas(0), 0}, } for tcIndex, tt := range tests { mempool.Update(1, emptyTxArr, tt.preFilter, tt.postFilter) From b8a9b0bf782c17d97af3fda46ddc73f7d35bd680 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 6 Nov 2018 07:39:05 +0100 Subject: [PATCH 03/17] Mempool WAL is still created by default in home directory, leads to permission errors (#2758) * only invoke InitWAL/CloseWAL if WalPath is not empty Closes #2717 * panic if WAL is not initialized when calling CloseWAL * add a changelog entry --- CHANGELOG_PENDING.md | 1 + config/config.go | 5 +++++ mempool/mempool.go | 46 ++++++++++++++++++----------------------- mempool/mempool_test.go | 7 ++----- node/node.go | 11 ++++++++-- 5 files changed, 37 insertions(+), 33 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index f5e56a123..ea0a666bd 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -25,3 +25,4 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi ### IMPROVEMENTS: ### BUG FIXES: +- [mempool] fix a bug where we create a WAL despite `wal_dir` being empty diff --git a/config/config.go b/config/config.go index ede57207c..fa6182114 100644 --- a/config/config.go +++ b/config/config.go @@ -497,6 +497,11 @@ func (cfg *MempoolConfig) WalDir() string { return rootify(cfg.WalPath, cfg.RootDir) } +// WalEnabled returns true if the WAL is enabled. +func (cfg *MempoolConfig) WalEnabled() bool { + return cfg.WalPath != "" +} + // ValidateBasic performs basic validation (checking param bounds, etc.) and // returns an error if any check fails. func (cfg *MempoolConfig) ValidateBasic() error { diff --git a/mempool/mempool.go b/mempool/mempool.go index 5e52989aa..9c9563165 100644 --- a/mempool/mempool.go +++ b/mempool/mempool.go @@ -213,39 +213,33 @@ func WithMetrics(metrics *Metrics) MempoolOption { return func(mem *Mempool) { mem.metrics = metrics } } -// CloseWAL closes and discards the underlying WAL file. -// Any further writes will not be relayed to disk. -func (mem *Mempool) CloseWAL() bool { - if mem == nil { - return false +// InitWAL creates a directory for the WAL file and opens a file itself. +// +// *panics* if can't create directory or open file. +// *not thread safe* +func (mem *Mempool) InitWAL() { + walDir := mem.config.WalDir() + err := cmn.EnsureDir(walDir, 0700) + if err != nil { + panic(errors.Wrap(err, "Error ensuring Mempool WAL dir")) + } + af, err := auto.OpenAutoFile(walDir + "/wal") + if err != nil { + panic(errors.Wrap(err, "Error opening Mempool WAL file")) } + mem.wal = af +} +// CloseWAL closes and discards the underlying WAL file. +// Any further writes will not be relayed to disk. +func (mem *Mempool) CloseWAL() { mem.proxyMtx.Lock() defer mem.proxyMtx.Unlock() - if mem.wal == nil { - return false - } - if err := mem.wal.Close(); err != nil && mem.logger != nil { - mem.logger.Error("Mempool.CloseWAL", "err", err) + if err := mem.wal.Close(); err != nil { + mem.logger.Error("Error closing WAL", "err", err) } mem.wal = nil - return true -} - -func (mem *Mempool) InitWAL() { - walDir := mem.config.WalDir() - if walDir != "" { - err := cmn.EnsureDir(walDir, 0700) - if err != nil { - cmn.PanicSanity(errors.Wrap(err, "Error ensuring Mempool wal dir")) - } - af, err := auto.OpenAutoFile(walDir + "/wal") - if err != nil { - cmn.PanicSanity(errors.Wrap(err, "Error opening Mempool wal file")) - } - mem.wal = af - } } // Lock locks the mempool. The consensus must be able to hold lock to safely update. diff --git a/mempool/mempool_test.go b/mempool/mempool_test.go index 44917afba..b8df6f737 100644 --- a/mempool/mempool_test.go +++ b/mempool/mempool_test.go @@ -369,15 +369,12 @@ func TestMempoolCloseWAL(t *testing.T) { // 7. Invoke CloseWAL() and ensure it discards the // WAL thus any other write won't go through. - require.True(t, mempool.CloseWAL(), "CloseWAL should CloseWAL") + mempool.CloseWAL() mempool.CheckTx(types.Tx([]byte("bar")), nil) sum2 := checksumFile(walFilepath, t) require.Equal(t, sum1, sum2, "expected no change to the WAL after invoking CloseWAL() since it was discarded") - // 8. Second CloseWAL should do nothing - require.False(t, mempool.CloseWAL(), "CloseWAL should CloseWAL") - - // 9. Sanity check to ensure that the WAL file still exists + // 8. Sanity check to ensure that the WAL file still exists m3, err := filepath.Glob(filepath.Join(rootDir, "*")) require.Nil(t, err, "successful globbing expected") require.Equal(t, 1, len(m3), "expecting the wal match in") diff --git a/node/node.go b/node/node.go index f62a8b472..4710397fa 100644 --- a/node/node.go +++ b/node/node.go @@ -13,8 +13,8 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" - "github.com/tendermint/go-amino" + amino "github.com/tendermint/go-amino" abci "github.com/tendermint/tendermint/abci/types" bc "github.com/tendermint/tendermint/blockchain" cfg "github.com/tendermint/tendermint/config" @@ -279,7 +279,9 @@ func NewNode(config *cfg.Config, ) mempoolLogger := logger.With("module", "mempool") mempool.SetLogger(mempoolLogger) - mempool.InitWAL() // no need to have the mempool wal during tests + if config.Mempool.WalEnabled() { + mempool.InitWAL() // no need to have the mempool wal during tests + } mempoolReactor := mempl.NewMempoolReactor(config.Mempool, mempool) mempoolReactor.SetLogger(mempoolLogger) @@ -586,6 +588,11 @@ func (n *Node) OnStop() { // TODO: gracefully disconnect from peers. n.sw.Stop() + // stop mempool WAL + if n.config.Mempool.WalEnabled() { + n.mempoolReactor.Mempool.CloseWAL() + } + if err := n.transport.Close(); err != nil { n.Logger.Error("Error closing transport", "err", err) } From 03e42d2e3866f01a00625f608e3bbfaeb30690de Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Mon, 5 Nov 2018 22:53:44 -0800 Subject: [PATCH 04/17] =?UTF-8?q?Fix=20crypto/merkle=20ProofOperators.Veri?= =?UTF-8?q?fy=20to=20check=20bounds=20on=20keypath=20pa=E2=80=A6=20(#2756)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix crypto/merkle ProofOperators.Verify to check bounds on keypath parts. * Update PENDING --- CHANGELOG_PENDING.md | 2 ++ crypto/merkle/proof.go | 3 +++ crypto/merkle/proof_test.go | 4 ++++ 3 files changed, 9 insertions(+) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index ea0a666bd..68a550398 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -25,4 +25,6 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi ### IMPROVEMENTS: ### BUG FIXES: + +- [crypto/merkle] [\#2756](https://github.com/tendermint/tendermint/issues/2756) Fix crypto/merkle ProofOperators.Verify to check bounds on keypath parts. - [mempool] fix a bug where we create a WAL despite `wal_dir` being empty diff --git a/crypto/merkle/proof.go b/crypto/merkle/proof.go index 5705c96bd..8f8b460c9 100644 --- a/crypto/merkle/proof.go +++ b/crypto/merkle/proof.go @@ -43,6 +43,9 @@ func (poz ProofOperators) Verify(root []byte, keypath string, args [][]byte) (er for i, op := range poz { key := op.GetKey() if len(key) != 0 { + if len(keys) == 0 { + return cmn.NewError("Key path has insufficient # of parts: expected no more keys but got %+v", string(key)) + } lastKey := keys[len(keys)-1] if !bytes.Equal(lastKey, key) { return cmn.NewError("Key mismatch on operation #%d: expected %+v but got %+v", i, string(lastKey), string(key)) diff --git a/crypto/merkle/proof_test.go b/crypto/merkle/proof_test.go index cc208e9a1..320b9188a 100644 --- a/crypto/merkle/proof_test.go +++ b/crypto/merkle/proof_test.go @@ -107,6 +107,10 @@ func TestProofOperators(t *testing.T) { err = popz.Verify(bz("OUTPUT4"), "//KEY4/KEY2/KEY1", [][]byte{bz("INPUT1")}) assert.NotNil(t, err) + // BAD KEY 5 + err = popz.Verify(bz("OUTPUT4"), "/KEY2/KEY1", [][]byte{bz("INPUT1")}) + assert.NotNil(t, err) + // BAD OUTPUT 1 err = popz.Verify(bz("OUTPUT4_WRONG"), "/KEY4/KEY2/KEY1", [][]byte{bz("INPUT1")}) assert.NotNil(t, err) From d460df1335fb3e0af2a9346c16cc35013552ba5d Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 7 Nov 2018 05:23:44 +0100 Subject: [PATCH 05/17] mempool: print postCheck error (#2762) This is a follow-up from https://github.com/tendermint/tendermint/pull/2724 Closes #2761 --- mempool/mempool.go | 29 ++++++++++++++--------------- mempool/mempool_test.go | 3 +++ 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/mempool/mempool.go b/mempool/mempool.go index 9c9563165..15e3119c4 100644 --- a/mempool/mempool.go +++ b/mempool/mempool.go @@ -356,8 +356,11 @@ func (mem *Mempool) resCbNormal(req *abci.Request, res *abci.Response) { switch r := res.Value.(type) { case *abci.Response_CheckTx: tx := req.GetCheckTx().Tx - if (r.CheckTx.Code == abci.CodeTypeOK) && - mem.isPostCheckPass(tx, r.CheckTx) { + var postCheckErr error + if mem.postCheck != nil { + postCheckErr = mem.postCheck(tx, r.CheckTx) + } + if (r.CheckTx.Code == abci.CodeTypeOK) && postCheckErr == nil { mem.counter++ memTx := &mempoolTx{ counter: mem.counter, @@ -377,7 +380,7 @@ func (mem *Mempool) resCbNormal(req *abci.Request, res *abci.Response) { mem.notifyTxsAvailable() } else { // ignore bad transaction - mem.logger.Info("Rejected bad transaction", "tx", TxID(tx), "res", r) + mem.logger.Info("Rejected bad transaction", "tx", TxID(tx), "res", r, "err", postCheckErr) mem.metrics.FailedTxs.Add(1) // remove from cache (it might be good later) mem.cache.Remove(tx) @@ -390,6 +393,7 @@ func (mem *Mempool) resCbNormal(req *abci.Request, res *abci.Response) { func (mem *Mempool) resCbRecheck(req *abci.Request, res *abci.Response) { switch r := res.Value.(type) { case *abci.Response_CheckTx: + tx := req.GetCheckTx().Tx memTx := mem.recheckCursor.Value.(*mempoolTx) if !bytes.Equal(req.GetCheckTx().Tx, memTx.tx) { cmn.PanicSanity( @@ -400,15 +404,20 @@ func (mem *Mempool) resCbRecheck(req *abci.Request, res *abci.Response) { ), ) } - if (r.CheckTx.Code == abci.CodeTypeOK) && mem.isPostCheckPass(memTx.tx, r.CheckTx) { + var postCheckErr error + if mem.postCheck != nil { + postCheckErr = mem.postCheck(tx, r.CheckTx) + } + if (r.CheckTx.Code == abci.CodeTypeOK) && postCheckErr == nil { // Good, nothing to do. } else { // Tx became invalidated due to newly committed block. + mem.logger.Info("Tx is no longer valid", "tx", TxID(tx), "res", r, "err", postCheckErr) mem.txs.Remove(mem.recheckCursor) mem.recheckCursor.DetachPrev() // remove from cache (it might be good later) - mem.cache.Remove(req.GetCheckTx().Tx) + mem.cache.Remove(tx) } if mem.recheckCursor == mem.recheckEnd { mem.recheckCursor = nil @@ -591,16 +600,6 @@ func (mem *Mempool) recheckTxs(goodTxs []types.Tx) { mem.proxyAppConn.FlushAsync() } -func (mem *Mempool) isPostCheckPass(tx types.Tx, r *abci.ResponseCheckTx) bool { - if mem.postCheck == nil { - return true - } - if err := mem.postCheck(tx, r); err != nil { - return false - } - return true -} - //-------------------------------------------------------------------------------- // mempoolTx is a transaction that successfully ran diff --git a/mempool/mempool_test.go b/mempool/mempool_test.go index b8df6f737..e25da7fed 100644 --- a/mempool/mempool_test.go +++ b/mempool/mempool_test.go @@ -65,6 +65,9 @@ func checkTxs(t *testing.T, mempool *Mempool, count int) types.Txs { t.Error(err) } if err := mempool.CheckTx(txBytes, nil); err != nil { + // Skip invalid txs. + // TestMempoolFilters will fail otherwise. It asserts a number of txs + // returned. if IsPreCheckError(err) { continue } From 6e9aee546061423864fef3f9fa332ef37c3dca96 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Tue, 6 Nov 2018 21:12:46 -0800 Subject: [PATCH 06/17] p2p: peer-id -> peer_id (#2771) * p2p: peer-id -> peer_id * update changelog --- CHANGELOG_PENDING.md | 1 + p2p/peer.go | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 68a550398..1996bfcc4 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -28,3 +28,4 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi - [crypto/merkle] [\#2756](https://github.com/tendermint/tendermint/issues/2756) Fix crypto/merkle ProofOperators.Verify to check bounds on keypath parts. - [mempool] fix a bug where we create a WAL despite `wal_dir` being empty +- [p2p] \#2771 Fix `peer-id` label name in prometheus metrics diff --git a/p2p/peer.go b/p2p/peer.go index 944174b0e..e98c16d26 100644 --- a/p2p/peer.go +++ b/p2p/peer.go @@ -240,7 +240,7 @@ func (p *peer) Send(chID byte, msgBytes []byte) bool { } res := p.mconn.Send(chID, msgBytes) if res { - p.metrics.PeerSendBytesTotal.With("peer-id", string(p.ID())).Add(float64(len(msgBytes))) + p.metrics.PeerSendBytesTotal.With("peer_id", string(p.ID())).Add(float64(len(msgBytes))) } return res } @@ -255,7 +255,7 @@ func (p *peer) TrySend(chID byte, msgBytes []byte) bool { } res := p.mconn.TrySend(chID, msgBytes) if res { - p.metrics.PeerSendBytesTotal.With("peer-id", string(p.ID())).Add(float64(len(msgBytes))) + p.metrics.PeerSendBytesTotal.With("peer_id", string(p.ID())).Add(float64(len(msgBytes))) } return res } @@ -330,7 +330,7 @@ func (p *peer) metricsReporter() { sendQueueSize += float64(chStatus.SendQueueSize) } - p.metrics.PeerPendingSendBytes.With("peer-id", string(p.ID())).Set(sendQueueSize) + p.metrics.PeerPendingSendBytes.With("peer_id", string(p.ID())).Set(sendQueueSize) case <-p.Quit(): return } From 8b773283132dd62a999131cba79130fa8f064bc3 Mon Sep 17 00:00:00 2001 From: Zach Date: Fri, 9 Nov 2018 09:11:06 -0500 Subject: [PATCH 07/17] [docs] improve organization of ABCI docs & fix links (#2749) * dedup with spec/abci/client-server * fixup abci/readme * link to getting started in abci/README * https * spec/abci: some deduplication * docs: remove extraneous comment --- abci/README.md | 156 +++----------------------------- docs/app-dev/app-development.md | 84 ----------------- docs/spec/abci/client-server.md | 44 +++++---- 3 files changed, 36 insertions(+), 248 deletions(-) diff --git a/abci/README.md b/abci/README.md index 63b43e54f..110ad40e9 100644 --- a/abci/README.md +++ b/abci/README.md @@ -1,7 +1,5 @@ # Application BlockChain Interface (ABCI) -[![CircleCI](https://circleci.com/gh/tendermint/abci.svg?style=svg)](https://circleci.com/gh/tendermint/abci) - Blockchains are systems for multi-master state machine replication. **ABCI** is an interface that defines the boundary between the replication engine (the blockchain), and the state machine (the application). @@ -12,160 +10,28 @@ Previously, the ABCI was referred to as TMSP. The community has provided a number of addtional implementations, see the [Tendermint Ecosystem](https://tendermint.com/ecosystem) -## Specification -A detailed description of the ABCI methods and message types is contained in: +## Installation & Usage -- [A prose specification](specification.md) -- [A protobuf file](https://github.com/tendermint/tendermint/blob/master/abci/types/types.proto) -- [A Go interface](https://github.com/tendermint/tendermint/blob/master/abci/types/application.go). +To get up and running quickly, see the [getting started guide](../docs/app-dev/getting-started.md) along with the [abci-cli documentation](../docs/app-dev/abci-cli.md) which will go through the examples found in the [examples](./example/) directory. -For more background information on ABCI, motivations, and tendermint, please visit [the documentation](https://tendermint.com/docs/). -The two guides to focus on are the `Application Development Guide` and `Using ABCI-CLI`. +## Specification + +A detailed description of the ABCI methods and message types is contained in: +- [The main spec](../docs/spec/abci/abci.md) +- [A protobuf file](./types/types.proto) +- [A Go interface](./types/application.go) ## Protocol Buffers -To compile the protobuf file, run: +To compile the protobuf file, run (from the root of the repo): ``` -cd $GOPATH/src/github.com/tendermint/tendermint/; make protoc_abci +make protoc_abci ``` See `protoc --help` and [the Protocol Buffers site](https://developers.google.com/protocol-buffers) -for details on compiling for other languages. Note we also include a [GRPC](http://www.grpc.io/docs) +for details on compiling for other languages. Note we also include a [GRPC](https://www.grpc.io/docs) service definition. -## Install ABCI-CLI - -The `abci-cli` is a simple tool for debugging ABCI servers and running some -example apps. To install it: - -``` -mkdir -p $GOPATH/src/github.com/tendermint -cd $GOPATH/src/github.com/tendermint -git clone https://github.com/tendermint/tendermint.git -cd tendermint -make get_tools -make get_vendor_deps -make install_abci -``` - -## Implementation - -We provide three implementations of the ABCI in Go: - -- Golang in-process -- ABCI-socket -- GRPC - -Note the GRPC version is maintained primarily to simplify onboarding and prototyping and is not receiving the same -attention to security and performance as the others - -### In Process - -The simplest implementation just uses function calls within Go. -This means ABCI applications written in Golang can be compiled with TendermintCore and run as a single binary. - -See the [examples](#examples) below for more information. - -### Socket (TSP) - -ABCI is best implemented as a streaming protocol. -The socket implementation provides for asynchronous, ordered message passing over unix or tcp. -Messages are serialized using Protobuf3 and length-prefixed with a [signed Varint](https://developers.google.com/protocol-buffers/docs/encoding?csw=1#signed-integers) - -For example, if the Protobuf3 encoded ABCI message is `0xDEADBEEF` (4 bytes), the length-prefixed message is `0x08DEADBEEF`, since `0x08` is the signed varint -encoding of `4`. If the Protobuf3 encoded ABCI message is 65535 bytes long, the length-prefixed message would be like `0xFEFF07...`. - -Note the benefit of using this `varint` encoding over the old version (where integers were encoded as `` is that -it is the standard way to encode integers in Protobuf. It is also generally shorter. - -### GRPC - -GRPC is an rpc framework native to Protocol Buffers with support in many languages. -Implementing the ABCI using GRPC can allow for faster prototyping, but is expected to be much slower than -the ordered, asynchronous socket protocol. The implementation has also not received as much testing or review. - -Note the length-prefixing used in the socket implementation does not apply for GRPC. - -## Usage - -The `abci-cli` tool wraps an ABCI client and can be used for probing/testing an ABCI server. -For instance, `abci-cli test` will run a test sequence against a listening server running the Counter application (see below). -It can also be used to run some example applications. -See [the documentation](https://tendermint.com/docs/) for more details. - -### Examples - -Check out the variety of example applications in the [example directory](example/). -It also contains the code refered to by the `counter` and `kvstore` apps; these apps come -built into the `abci-cli` binary. - -#### Counter - -The `abci-cli counter` application illustrates nonce checking in transactions. It's code looks like: - -```golang -func cmdCounter(cmd *cobra.Command, args []string) error { - - app := counter.NewCounterApplication(flagSerial) - - logger := log.NewTMLogger(log.NewSyncWriter(os.Stdout)) - - // Start the listener - srv, err := server.NewServer(flagAddrC, flagAbci, app) - if err != nil { - return err - } - srv.SetLogger(logger.With("module", "abci-server")) - if err := srv.Start(); err != nil { - return err - } - - // Wait forever - cmn.TrapSignal(func() { - // Cleanup - srv.Stop() - }) - return nil -} -``` - -and can be found in [this file](cmd/abci-cli/abci-cli.go). - -#### kvstore - -The `abci-cli kvstore` application, which illustrates a simple key-value Merkle tree - -```golang -func cmdKVStore(cmd *cobra.Command, args []string) error { - logger := log.NewTMLogger(log.NewSyncWriter(os.Stdout)) - - // Create the application - in memory or persisted to disk - var app types.Application - if flagPersist == "" { - app = kvstore.NewKVStoreApplication() - } else { - app = kvstore.NewPersistentKVStoreApplication(flagPersist) - app.(*kvstore.PersistentKVStoreApplication).SetLogger(logger.With("module", "kvstore")) - } - - // Start the listener - srv, err := server.NewServer(flagAddrD, flagAbci, app) - if err != nil { - return err - } - srv.SetLogger(logger.With("module", "abci-server")) - if err := srv.Start(); err != nil { - return err - } - - // Wait forever - cmn.TrapSignal(func() { - // Cleanup - srv.Stop() - }) - return nil -} -``` diff --git a/docs/app-dev/app-development.md b/docs/app-dev/app-development.md index 2618bb1e9..d157ce378 100644 --- a/docs/app-dev/app-development.md +++ b/docs/app-dev/app-development.md @@ -47,90 +47,6 @@ The mempool and consensus logic act as clients, and each maintains an open ABCI connection with the application, which hosts an ABCI server. Shown are the request and response types sent on each connection. -## Message Protocol - -The message protocol consists of pairs of requests and responses. Some -messages have no fields, while others may include byte-arrays, strings, -or integers. See the `message Request` and `message Response` -definitions in [the protobuf definition -file](https://github.com/tendermint/tendermint/blob/develop/abci/types/types.proto), -and the [protobuf -documentation](https://developers.google.com/protocol-buffers/docs/overview) -for more details. - -For each request, a server should respond with the corresponding -response, where order of requests is preserved in the order of -responses. - -## Server - -To use ABCI in your programming language of choice, there must be a ABCI -server in that language. Tendermint supports two kinds of implementation -of the server: - -- Asynchronous, raw socket server (Tendermint Socket Protocol, also - known as TSP or Teaspoon) -- GRPC - -Both can be tested using the `abci-cli` by setting the `--abci` flag -appropriately (ie. to `socket` or `grpc`). - -See examples, in various stages of maintenance, in -[Go](https://github.com/tendermint/tendermint/tree/develop/abci/server), -[JavaScript](https://github.com/tendermint/js-abci), -[Python](https://github.com/tendermint/tendermint/tree/develop/abci/example/python3/abci), -[C++](https://github.com/mdyring/cpp-tmsp), and -[Java](https://github.com/jTendermint/jabci). - -### GRPC - -If GRPC is available in your language, this is the easiest approach, -though it will have significant performance overhead. - -To get started with GRPC, copy in the [protobuf -file](https://github.com/tendermint/tendermint/blob/develop/abci/types/types.proto) -and compile it using the GRPC plugin for your language. For instance, -for golang, the command is `protoc --go_out=plugins=grpc:. types.proto`. -See the [grpc documentation for more details](http://www.grpc.io/docs/). -`protoc` will autogenerate all the necessary code for ABCI client and -server in your language, including whatever interface your application -must satisfy to be used by the ABCI server for handling requests. - -### TSP - -If GRPC is not available in your language, or you require higher -performance, or otherwise enjoy programming, you may implement your own -ABCI server using the Tendermint Socket Protocol, known affectionately -as Teaspoon. The first step is still to auto-generate the relevant data -types and codec in your language using `protoc`. Messages coming over -the socket are proto3 encoded, but additionally length-prefixed to -facilitate use as a streaming protocol. proto3 doesn't have an -official length-prefix standard, so we use our own. The first byte in -the prefix represents the length of the Big Endian encoded length. The -remaining bytes in the prefix are the Big Endian encoded length. - -For example, if the proto3 encoded ABCI message is 0xDEADBEEF (4 -bytes), the length-prefixed message is 0x0104DEADBEEF. If the proto3 -encoded ABCI message is 65535 bytes long, the length-prefixed message -would be like 0x02FFFF.... - -Note this prefixing does not apply for grpc. - -An ABCI server must also be able to support multiple connections, as -Tendermint uses three connections. - -## Client - -There are currently two use-cases for an ABCI client. One is a testing -tool, as in the `abci-cli`, which allows ABCI requests to be sent via -command line. The other is a consensus engine, such as Tendermint Core, -which makes requests to the application every time a new transaction is -received or a block is committed. - -It is unlikely that you will need to implement a client. For details of -our client, see -[here](https://github.com/tendermint/tendermint/tree/develop/abci/client). - Most of the examples below are from [kvstore application](https://github.com/tendermint/tendermint/blob/develop/abci/example/kvstore/kvstore.go), which is a part of the abci repo. [persistent_kvstore diff --git a/docs/spec/abci/client-server.md b/docs/spec/abci/client-server.md index 822bfd1fc..5ac7b3eb4 100644 --- a/docs/spec/abci/client-server.md +++ b/docs/spec/abci/client-server.md @@ -3,12 +3,8 @@ This section is for those looking to implement their own ABCI Server, perhaps in a new programming language. -You are expected to have read [ABCI Methods and Types](abci.md) and [ABCI -Applications](apps.md). - -See additional details in the [ABCI -readme](https://github.com/tendermint/tendermint/blob/develop/abci/README.md)(TODO: deduplicate -those details). +You are expected to have read [ABCI Methods and Types](./abci.md) and [ABCI +Applications](./apps.md). ## Message Protocol @@ -24,17 +20,16 @@ For each request, a server should respond with the corresponding response, where the order of requests is preserved in the order of responses. -## Server +## Server Implementations To use ABCI in your programming language of choice, there must be a ABCI -server in that language. Tendermint supports two kinds of implementation -of the server: +server in that language. Tendermint supports three implementations of the ABCI, written in Go: -- Asynchronous, raw socket server (Tendermint Socket Protocol, also - known as TSP or Teaspoon) +- In-process (Golang only) +- ABCI-socket - GRPC -Both can be tested using the `abci-cli` by setting the `--abci` flag +The latter two can be tested using the `abci-cli` by setting the `--abci` flag appropriately (ie. to `socket` or `grpc`). See examples, in various stages of maintenance, in @@ -44,6 +39,12 @@ See examples, in various stages of maintenance, in [C++](https://github.com/mdyring/cpp-tmsp), and [Java](https://github.com/jTendermint/jabci). +### In Process + +The simplest implementation uses function calls within Golang. +This means ABCI applications written in Golang can be compiled with TendermintCore and run as a single binary. + + ### GRPC If GRPC is available in your language, this is the easiest approach, @@ -58,15 +59,18 @@ See the [grpc documentation for more details](http://www.grpc.io/docs/). server in your language, including whatever interface your application must satisfy to be used by the ABCI server for handling requests. +Note the length-prefixing used in the socket implementation (TSP) does not apply for GRPC. + ### TSP +Tendermint Socket Protocol is an asynchronous, raw socket server which provides ordered message passing over unix or tcp. +Messages are serialized using Protobuf3 and length-prefixed with a [signed Varint](https://developers.google.com/protocol-buffers/docs/encoding?csw=1#signed-integers) + If GRPC is not available in your language, or you require higher performance, or otherwise enjoy programming, you may implement your own -ABCI server using the Tendermint Socket Protocol, known affectionately -as Teaspoon. The first step is still to auto-generate the relevant data -types and codec in your language using `protoc`. Messages coming over -the socket are proto3 encoded, but additionally length-prefixed to -facilitate use as a streaming protocol. proto3 doesn't have an +ABCI server using the Tendermint Socket Protocol. The first step is still to auto-generate the relevant data +types and codec in your language using `protoc`. In addition to being proto3 encoded, messages coming over +the socket are length-prefixed to facilitate use as a streaming protocol. proto3 doesn't have an official length-prefix standard, so we use our own. The first byte in the prefix represents the length of the Big Endian encoded length. The remaining bytes in the prefix are the Big Endian encoded length. @@ -76,12 +80,14 @@ bytes), the length-prefixed message is 0x0104DEADBEEF. If the proto3 encoded ABCI message is 65535 bytes long, the length-prefixed message would be like 0x02FFFF.... -Note this prefixing does not apply for grpc. +The benefit of using this `varint` encoding over the old version (where integers were encoded as `` is that +it is the standard way to encode integers in Protobuf. It is also generally shorter. + +As noted above, this prefixing does not apply for GRPC. An ABCI server must also be able to support multiple connections, as Tendermint uses three connections. - ### Async vs Sync The main ABCI server (ie. non-GRPC) provides ordered asynchronous messages. From 46d32af055fec0b2c101352e729a620600df83e7 Mon Sep 17 00:00:00 2001 From: Catalin Pirvu Date: Fri, 9 Nov 2018 16:59:04 +0200 Subject: [PATCH 08/17] Add tests for ValidateBasic methods (#2754) Fixes #2740 --- evidence/reactor_test.go | 28 +++++++++++++++++++++++++ types/evidence_test.go | 37 ++++++++++++++++++++++++++++++++- types/heartbeat_test.go | 44 +++++++++++++++++++++++++++++++++++++++ types/part_set_test.go | 45 ++++++++++++++++++++++++++++++++++++++++ types/proposal_test.go | 41 ++++++++++++++++++++++++++++++++++++ types/vote_test.go | 28 +++++++++++++++++++++++++ 6 files changed, 222 insertions(+), 1 deletion(-) diff --git a/evidence/reactor_test.go b/evidence/reactor_test.go index ea9657d23..69dcdec57 100644 --- a/evidence/reactor_test.go +++ b/evidence/reactor_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" cfg "github.com/tendermint/tendermint/config" + "github.com/tendermint/tendermint/crypto/secp256k1" dbm "github.com/tendermint/tendermint/libs/db" "github.com/tendermint/tendermint/libs/log" "github.com/tendermint/tendermint/p2p" @@ -178,3 +179,30 @@ func TestReactorSelectiveBroadcast(t *testing.T) { peers := reactors[1].Switch.Peers().List() assert.Equal(t, 1, len(peers)) } +func TestEvidenceListMessageValidationBasic(t *testing.T) { + + testCases := []struct { + testName string + malleateEvListMsg func(*EvidenceListMessage) + expectErr bool + }{ + {"Good EvidenceListMessage", func(evList *EvidenceListMessage) {}, false}, + {"Invalid EvidenceListMessage", func(evList *EvidenceListMessage) { + evList.Evidence = append(evList.Evidence, + &types.DuplicateVoteEvidence{PubKey: secp256k1.GenPrivKey().PubKey()}) + }, true}, + } + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + evListMsg := &EvidenceListMessage{} + n := 3 + valAddr := []byte("myval") + evListMsg.Evidence = make([]types.Evidence, n) + for i := 0; i < n; i++ { + evListMsg.Evidence[i] = types.NewMockGoodEvidence(int64(i+1), 0, valAddr) + } + tc.malleateEvListMsg(evListMsg) + assert.Equal(t, tc.expectErr, evListMsg.ValidateBasic() != nil, "Validate Basic had an unexpected result") + }) + } +} diff --git a/types/evidence_test.go b/types/evidence_test.go index 033b51e5d..a96b63a9e 100644 --- a/types/evidence_test.go +++ b/types/evidence_test.go @@ -61,7 +61,7 @@ func TestEvidence(t *testing.T) { {vote1, makeVote(val, chainID, 0, 10, 3, 1, blockID2), false}, // wrong round {vote1, makeVote(val, chainID, 0, 10, 2, 2, blockID2), false}, // wrong step {vote1, makeVote(val2, chainID, 0, 10, 2, 1, blockID), false}, // wrong validator - {vote1, badVote, false}, // signed by wrong key + {vote1, badVote, false}, // signed by wrong key } pubKey := val.GetPubKey() @@ -121,3 +121,38 @@ func randomDuplicatedVoteEvidence() *DuplicateVoteEvidence { VoteB: makeVote(val, chainID, 0, 10, 2, 1, blockID2), } } + +func TestDuplicateVoteEvidenceValidation(t *testing.T) { + val := NewMockPV() + blockID := makeBlockID(tmhash.Sum([]byte("blockhash")), math.MaxInt64, tmhash.Sum([]byte("partshash"))) + blockID2 := makeBlockID(tmhash.Sum([]byte("blockhash2")), math.MaxInt64, tmhash.Sum([]byte("partshash"))) + const chainID = "mychain" + + testCases := []struct { + testName string + malleateEvidence func(*DuplicateVoteEvidence) + expectErr bool + }{ + {"Good DuplicateVoteEvidence", func(ev *DuplicateVoteEvidence) {}, false}, + {"Nil vote A", func(ev *DuplicateVoteEvidence) { ev.VoteA = nil }, true}, + {"Nil vote B", func(ev *DuplicateVoteEvidence) { ev.VoteB = nil }, true}, + {"Nil votes", func(ev *DuplicateVoteEvidence) { + ev.VoteA = nil + ev.VoteB = nil + }, true}, + {"Invalid vote type", func(ev *DuplicateVoteEvidence) { + ev.VoteA = makeVote(val, chainID, math.MaxInt64, math.MaxInt64, math.MaxInt64, 0, blockID2) + }, true}, + } + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + ev := &DuplicateVoteEvidence{ + PubKey: secp256k1.GenPrivKey().PubKey(), + VoteA: makeVote(val, chainID, math.MaxInt64, math.MaxInt64, math.MaxInt64, 0x02, blockID), + VoteB: makeVote(val, chainID, math.MaxInt64, math.MaxInt64, math.MaxInt64, 0x02, blockID2), + } + tc.malleateEvidence(ev) + assert.Equal(t, tc.expectErr, ev.ValidateBasic() != nil, "Validate Basic had an unexpected result") + }) + } +} diff --git a/types/heartbeat_test.go b/types/heartbeat_test.go index e1ffdd6f1..0951c7b9d 100644 --- a/types/heartbeat_test.go +++ b/types/heartbeat_test.go @@ -3,8 +3,10 @@ package types import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/tendermint/tendermint/crypto/ed25519" + "github.com/tendermint/tendermint/crypto/secp256k1" ) func TestHeartbeatCopy(t *testing.T) { @@ -58,3 +60,45 @@ func TestHeartbeatWriteSignBytes(t *testing.T) { require.Equal(t, string(signBytes), "null") }) } + +func TestHeartbeatValidateBasic(t *testing.T) { + testCases := []struct { + testName string + malleateHeartBeat func(*Heartbeat) + expectErr bool + }{ + {"Good HeartBeat", func(hb *Heartbeat) {}, false}, + {"Invalid address size", func(hb *Heartbeat) { + hb.ValidatorAddress = nil + }, true}, + {"Negative validator index", func(hb *Heartbeat) { + hb.ValidatorIndex = -1 + }, true}, + {"Negative height", func(hb *Heartbeat) { + hb.Height = -1 + }, true}, + {"Negative round", func(hb *Heartbeat) { + hb.Round = -1 + }, true}, + {"Negative sequence", func(hb *Heartbeat) { + hb.Sequence = -1 + }, true}, + {"Missing signature", func(hb *Heartbeat) { + hb.Signature = nil + }, true}, + {"Signature too big", func(hb *Heartbeat) { + hb.Signature = make([]byte, MaxSignatureSize+1) + }, true}, + } + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + hb := &Heartbeat{ + ValidatorAddress: secp256k1.GenPrivKey().PubKey().Address(), + Signature: make([]byte, 4), + ValidatorIndex: 1, Height: 10, Round: 1} + + tc.malleateHeartBeat(hb) + assert.Equal(t, tc.expectErr, hb.ValidateBasic() != nil, "Validate Basic had an unexpected result") + }) + } +} diff --git a/types/part_set_test.go b/types/part_set_test.go index 3576e747e..e597088c6 100644 --- a/types/part_set_test.go +++ b/types/part_set_test.go @@ -83,3 +83,48 @@ func TestWrongProof(t *testing.T) { t.Errorf("Expected to fail adding a part with bad bytes.") } } + +func TestPartSetHeaderSetValidateBasic(t *testing.T) { + + testCases := []struct { + testName string + malleatePartSetHeader func(*PartSetHeader) + expectErr bool + }{ + {"Good PartSet", func(psHeader *PartSetHeader) {}, false}, + {"Negative Total", func(psHeader *PartSetHeader) { psHeader.Total = -2 }, true}, + {"Invalid Hash", func(psHeader *PartSetHeader) { psHeader.Hash = make([]byte, 1) }, true}, + } + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + data := cmn.RandBytes(testPartSize * 100) + ps := NewPartSetFromData(data, testPartSize) + psHeader := ps.Header() + tc.malleatePartSetHeader(&psHeader) + assert.Equal(t, tc.expectErr, psHeader.ValidateBasic() != nil, "Validate Basic had an unexpected result") + }) + } +} + +func TestPartValidateBasic(t *testing.T) { + + testCases := []struct { + testName string + malleatePart func(*Part) + expectErr bool + }{ + {"Good Part", func(pt *Part) {}, false}, + {"Negative index", func(pt *Part) { pt.Index = -1 }, true}, + {"Too big part", func(pt *Part) { pt.Bytes = make([]byte, BlockPartSizeBytes+1) }, true}, + } + + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + data := cmn.RandBytes(testPartSize * 100) + ps := NewPartSetFromData(data, testPartSize) + part := ps.GetPart(0) + tc.malleatePart(part) + assert.Equal(t, tc.expectErr, part.ValidateBasic() != nil, "Validate Basic had an unexpected result") + }) + } +} diff --git a/types/proposal_test.go b/types/proposal_test.go index 9738db2d2..f1c048e1d 100644 --- a/types/proposal_test.go +++ b/types/proposal_test.go @@ -1,10 +1,13 @@ package types import ( + "math" "testing" "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/tendermint/tendermint/crypto/tmhash" ) var testProposal *Proposal @@ -97,3 +100,41 @@ func BenchmarkProposalVerifySignature(b *testing.B) { pubKey.VerifyBytes(testProposal.SignBytes("test_chain_id"), testProposal.Signature) } } + +func TestProposalValidateBasic(t *testing.T) { + + privVal := NewMockPV() + testCases := []struct { + testName string + malleateProposal func(*Proposal) + expectErr bool + }{ + {"Good Proposal", func(p *Proposal) {}, false}, + {"Invalid Type", func(p *Proposal) { p.Type = PrecommitType }, true}, + {"Invalid Height", func(p *Proposal) { p.Height = -1 }, true}, + {"Invalid Round", func(p *Proposal) { p.Round = -1 }, true}, + {"Invalid POLRound", func(p *Proposal) { p.POLRound = -2 }, true}, + {"Invalid BlockId", func(p *Proposal) { + p.BlockID = BlockID{[]byte{1, 2, 3}, PartSetHeader{111, []byte("blockparts")}} + }, true}, + {"Invalid Signature", func(p *Proposal) { + p.Signature = make([]byte, 0) + }, true}, + {"Too big Signature", func(p *Proposal) { + p.Signature = make([]byte, MaxSignatureSize+1) + }, true}, + } + blockID := makeBlockID(tmhash.Sum([]byte("blockhash")), math.MaxInt64, tmhash.Sum([]byte("partshash"))) + + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + prop := NewProposal( + 4, 2, 2, + blockID) + err := privVal.SignProposal("test_chain_id", prop) + require.NoError(t, err) + tc.malleateProposal(prop) + assert.Equal(t, tc.expectErr, prop.ValidateBasic() != nil, "Validate Basic had an unexpected result") + }) + } +} diff --git a/types/vote_test.go b/types/vote_test.go index cda54f898..942f2d6b9 100644 --- a/types/vote_test.go +++ b/types/vote_test.go @@ -250,3 +250,31 @@ func TestVoteString(t *testing.T) { t.Errorf("Got unexpected string for Vote. Expected:\n%v\nGot:\n%v", expected, str2) } } + +func TestVoteValidateBasic(t *testing.T) { + privVal := NewMockPV() + + testCases := []struct { + testName string + malleateVote func(*Vote) + expectErr bool + }{ + {"Good Vote", func(v *Vote) {}, false}, + {"Negative Height", func(v *Vote) { v.Height = -1 }, true}, + {"Negative Round", func(v *Vote) { v.Round = -1 }, true}, + {"Invalid BlockID", func(v *Vote) { v.BlockID = BlockID{[]byte{1, 2, 3}, PartSetHeader{111, []byte("blockparts")}} }, true}, + {"Invalid Address", func(v *Vote) { v.ValidatorAddress = make([]byte, 1) }, true}, + {"Invalid ValidatorIndex", func(v *Vote) { v.ValidatorIndex = -1 }, true}, + {"Invalid Signature", func(v *Vote) { v.Signature = nil }, true}, + {"Too big Signature", func(v *Vote) { v.Signature = make([]byte, MaxSignatureSize+1) }, true}, + } + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + vote := examplePrecommit() + err := privVal.SignVote("test_chain_id", vote) + require.NoError(t, err) + tc.malleateVote(vote) + assert.Equal(t, tc.expectErr, vote.ValidateBasic() != nil, "Validate Basic had an unexpected result") + }) + } +} From d178ea9eaf6ef3113dc951f16bbe4f54a1c505ce Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 6 Nov 2018 13:12:12 +0100 Subject: [PATCH 09/17] use our logger in autofile/group --- consensus/wal.go | 6 ++++++ libs/autofile/autofile.go | 15 ++++++++++++++- libs/autofile/group.go | 7 +++---- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/consensus/wal.go b/consensus/wal.go index 6472c2573..bbc9908fb 100644 --- a/consensus/wal.go +++ b/consensus/wal.go @@ -13,6 +13,7 @@ import ( amino "github.com/tendermint/go-amino" auto "github.com/tendermint/tendermint/libs/autofile" cmn "github.com/tendermint/tendermint/libs/common" + "github.com/tendermint/tendermint/libs/log" "github.com/tendermint/tendermint/types" tmtime "github.com/tendermint/tendermint/types/time" ) @@ -95,6 +96,11 @@ func (wal *baseWAL) Group() *auto.Group { return wal.group } +func (wal *baseWAL) SetLogger(l log.Logger) { + wal.BaseService.Logger = l + wal.group.SetLogger(l) +} + func (wal *baseWAL) OnStart() error { size, err := wal.group.Head.Size() if err != nil { diff --git a/libs/autofile/autofile.go b/libs/autofile/autofile.go index 6822545e2..8e820d7d0 100644 --- a/libs/autofile/autofile.go +++ b/libs/autofile/autofile.go @@ -83,6 +83,8 @@ func OpenAutoFile(path string) (*AutoFile, error) { return af, nil } +// Close shuts down the closing goroutine, SIGHUP handler and closes the +// AutoFile. func (af *AutoFile) Close() error { af.closeTicker.Stop() close(af.closeTickerStopc) @@ -116,6 +118,10 @@ func (af *AutoFile) closeFile() (err error) { return file.Close() } +// Write writes len(b) bytes to the AutoFile. It returns the number of bytes +// written and an error, if any. Write returns a non-nil error when n != +// len(b). +// Opens AutoFile if needed. func (af *AutoFile) Write(b []byte) (n int, err error) { af.mtx.Lock() defer af.mtx.Unlock() @@ -130,6 +136,10 @@ func (af *AutoFile) Write(b []byte) (n int, err error) { return } +// Sync commits the current contents of the file to stable storage. Typically, +// this means flushing the file system's in-memory copy of recently written +// data to disk. +// Opens AutoFile if needed. func (af *AutoFile) Sync() error { af.mtx.Lock() defer af.mtx.Unlock() @@ -158,6 +168,9 @@ func (af *AutoFile) openFile() error { return nil } +// Size returns the size of the AutoFile. It returns -1 and an error if fails +// get stats or open file. +// Opens AutoFile if needed. func (af *AutoFile) Size() (int64, error) { af.mtx.Lock() defer af.mtx.Unlock() @@ -171,10 +184,10 @@ func (af *AutoFile) Size() (int64, error) { return -1, err } } + stat, err := af.file.Stat() if err != nil { return -1, err } return stat.Size(), nil - } diff --git a/libs/autofile/group.go b/libs/autofile/group.go index ea272b61a..fcdf0d0dd 100644 --- a/libs/autofile/group.go +++ b/libs/autofile/group.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "io" - "log" "os" "path" "path/filepath" @@ -253,18 +252,18 @@ func (g *Group) checkTotalSizeLimit() { } if index == gInfo.MaxIndex { // Special degenerate case, just do nothing. - log.Println("WARNING: Group's head " + g.Head.Path + "may grow without bound") + g.Logger.Info("Group's head may grow without bound", "head", g.Head.Path) return } pathToRemove := filePathForIndex(g.Head.Path, index, gInfo.MaxIndex) fileInfo, err := os.Stat(pathToRemove) if err != nil { - log.Println("WARNING: Failed to fetch info for file @" + pathToRemove) + g.Logger.Error("Failed to fetch info for file", "file", pathToRemove) continue } err = os.Remove(pathToRemove) if err != nil { - log.Println(err) + g.Logger.Error("Failed to remove path", "path", pathToRemove) return } totalSize -= fileInfo.Size() From 091d2c3e5ecb595f69d1f7273ed7c907c087c2c7 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 6 Nov 2018 13:12:42 +0100 Subject: [PATCH 10/17] openFile creates a file if not exist => ErrNotExist is not possible --- libs/autofile/autofile.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/libs/autofile/autofile.go b/libs/autofile/autofile.go index 8e820d7d0..a1e2f49e6 100644 --- a/libs/autofile/autofile.go +++ b/libs/autofile/autofile.go @@ -176,11 +176,7 @@ func (af *AutoFile) Size() (int64, error) { defer af.mtx.Unlock() if af.file == nil { - err := af.openFile() - if err != nil { - if err == os.ErrNotExist { - return 0, nil - } + if err := af.openFile(); err != nil { return -1, err } } From 13badc1d29eafed52a836b69471ac5d3db3a18ae Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 6 Nov 2018 13:14:47 +0100 Subject: [PATCH 11/17] [autofile/group] do not panic when checking size It's OK if the head will grow a little bit bigger, but we'll avoid panic. Refs #2703 --- CHANGELOG_PENDING.md | 1 + libs/autofile/group.go | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 1996bfcc4..f63bcdc4b 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -29,3 +29,4 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi - [crypto/merkle] [\#2756](https://github.com/tendermint/tendermint/issues/2756) Fix crypto/merkle ProofOperators.Verify to check bounds on keypath parts. - [mempool] fix a bug where we create a WAL despite `wal_dir` being empty - [p2p] \#2771 Fix `peer-id` label name in prometheus metrics +- [autofile] [\#2703] do not panic when checking Head size diff --git a/libs/autofile/group.go b/libs/autofile/group.go index fcdf0d0dd..1ec6b240d 100644 --- a/libs/autofile/group.go +++ b/libs/autofile/group.go @@ -230,7 +230,8 @@ func (g *Group) checkHeadSizeLimit() { } size, err := g.Head.Size() if err != nil { - panic(err) + g.Logger.Error("Group's head may grow without bound", "head", g.Head.Path, "err", err) + return } if size >= limit { g.RotateFile() @@ -252,11 +253,11 @@ func (g *Group) checkTotalSizeLimit() { } if index == gInfo.MaxIndex { // Special degenerate case, just do nothing. - g.Logger.Info("Group's head may grow without bound", "head", g.Head.Path) + g.Logger.Error("Group's head may grow without bound", "head", g.Head.Path) return } pathToRemove := filePathForIndex(g.Head.Path, index, gInfo.MaxIndex) - fileInfo, err := os.Stat(pathToRemove) + fInfo, err := os.Stat(pathToRemove) if err != nil { g.Logger.Error("Failed to fetch info for file", "file", pathToRemove) continue @@ -266,7 +267,7 @@ func (g *Group) checkTotalSizeLimit() { g.Logger.Error("Failed to remove path", "path", pathToRemove) return } - totalSize -= fileInfo.Size() + totalSize -= fInfo.Size() } } From 1944d8534b6048666bb73fa3142c666acddb098d Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 6 Nov 2018 15:54:04 +0100 Subject: [PATCH 12/17] test AutoFile#Size (happy path) --- libs/autofile/autofile_test.go | 37 ++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/libs/autofile/autofile_test.go b/libs/autofile/autofile_test.go index 5408d8204..0b3521c2a 100644 --- a/libs/autofile/autofile_test.go +++ b/libs/autofile/autofile_test.go @@ -84,3 +84,40 @@ func TestOpenAutoFilePerms(t *testing.T) { t.Errorf("unexpected error %v", e) } } + +func TestAutoFileSize(t *testing.T) { + // First, create an AutoFile writing to a tempfile dir + f, err := ioutil.TempFile("", "sighup_test") + require.NoError(t, err) + err = f.Close() + require.NoError(t, err) + + // Here is the actual AutoFile. + af, err := OpenAutoFile(f.Name()) + require.NoError(t, err) + + // 1. Empty file + size, err := af.Size() + require.Zero(t, size) + require.NoError(t, err) + + // 2. Not empty file + data := []byte("Maniac\n") + _, err = af.Write(data) + require.NoError(t, err) + size, err = af.Size() + require.EqualValues(t, len(data), size) + require.NoError(t, err) + + // 3. Not existing file + err = af.Close() + require.NoError(t, err) + err = os.Remove(f.Name()) + require.NoError(t, err) + size, err = af.Size() + require.EqualValues(t, 0, size, "Expected a new file to be empty") + require.NoError(t, err) + + // Cleanup + _ = os.Remove(f.Name()) +} From 5b19fcf20450de8351213ea3dd8ada1f820b444d Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Sun, 11 Nov 2018 03:50:25 -0800 Subject: [PATCH 13/17] p2p: AddressBook requires addresses to have IDs; Do not close conn immediately after sending pex addrs in seed mode (#2797) * Require addressbook to only store addresses with valid ID * Do not shut down peer immediately after sending pex addrs in SeedMode * p2p: fix #2773 * seed mode: use go-routine to sleep before stopping peer --- CHANGELOG.md | 4 ++-- p2p/netaddress.go | 18 ++++++++++++++++-- p2p/node_info.go | 3 ++- p2p/pex/addrbook.go | 4 ++++ p2p/pex/errors.go | 8 ++++++++ p2p/pex/pex_reactor.go | 6 +++++- p2p/switch.go | 2 +- 7 files changed, 38 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 792386e5c..fd36e3853 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -139,8 +139,8 @@ increasing attention to backwards compatibility. Thanks for bearing with us! - [node] [\#2434](https://github.com/tendermint/tendermint/issues/2434) Make node respond to signal interrupts while sleeping for genesis time - [state] [\#2616](https://github.com/tendermint/tendermint/issues/2616) Pass nil to NewValidatorSet() when genesis file's Validators field is nil - [p2p] [\#2555](https://github.com/tendermint/tendermint/issues/2555) Fix p2p switch FlushThrottle value (@goolAdapter) -- [p2p] [\#2668](https://github.com/tendermint/tendermint/issues/2668) Reconnect to originally dialed address (not self-reported - address) for persistent peers +- [p2p] [\#2668](https://github.com/tendermint/tendermint/issues/2668) Reconnect to originally dialed address (not self-reported address) for persistent peers +- [p2p] [\#2797](https://github.com/tendermint/tendermint/pull/2797) AddressBook requires addresses to have IDs; Do not crap out immediately after sending pex addrs in seed mode ## v0.25.0 diff --git a/p2p/netaddress.go b/p2p/netaddress.go index ec9a0ea7c..f60271bcc 100644 --- a/p2p/netaddress.go +++ b/p2p/netaddress.go @@ -32,8 +32,10 @@ type NetAddress struct { str string } -// IDAddressString returns id@hostPort. -func IDAddressString(id ID, hostPort string) string { +// IDAddressString returns id@hostPort. It strips the leading +// protocol from protocolHostPort if it exists. +func IDAddressString(id ID, protocolHostPort string) string { + hostPort := removeProtocolIfDefined(protocolHostPort) return fmt.Sprintf("%s@%s", id, hostPort) } @@ -218,10 +220,22 @@ func (na *NetAddress) Routable() bool { // For IPv4 these are either a 0 or all bits set address. For IPv6 a zero // address or one that matches the RFC3849 documentation address format. func (na *NetAddress) Valid() bool { + if string(na.ID) != "" { + data, err := hex.DecodeString(string(na.ID)) + if err != nil || len(data) != IDByteLength { + return false + } + } return na.IP != nil && !(na.IP.IsUnspecified() || na.RFC3849() || na.IP.Equal(net.IPv4bcast)) } +// HasID returns true if the address has an ID. +// NOTE: It does not check whether the ID is valid or not. +func (na *NetAddress) HasID() bool { + return string(na.ID) != "" +} + // Local returns true if it is a local address. func (na *NetAddress) Local() bool { return na.IP.IsLoopback() || zero4.Contains(na.IP) diff --git a/p2p/node_info.go b/p2p/node_info.go index e46174e07..c36d98d9c 100644 --- a/p2p/node_info.go +++ b/p2p/node_info.go @@ -216,7 +216,8 @@ OUTER_LOOP: // ListenAddr. Note that the ListenAddr is not authenticated and // may not match that address actually dialed if its an outbound peer. func (info DefaultNodeInfo) NetAddress() *NetAddress { - netAddr, err := NewNetAddressString(IDAddressString(info.ID(), info.ListenAddr)) + idAddr := IDAddressString(info.ID(), info.ListenAddr) + netAddr, err := NewNetAddressString(idAddr) if err != nil { switch err.(type) { case ErrNetAddressLookup: diff --git a/p2p/pex/addrbook.go b/p2p/pex/addrbook.go index 61710bbf2..405a4628b 100644 --- a/p2p/pex/addrbook.go +++ b/p2p/pex/addrbook.go @@ -652,6 +652,10 @@ func (a *addrBook) addAddress(addr, src *p2p.NetAddress) error { return ErrAddrBookInvalidAddr{addr} } + if !addr.HasID() { + return ErrAddrBookInvalidAddrNoID{addr} + } + // TODO: we should track ourAddrs by ID and by IP:PORT and refuse both. if _, ok := a.ourAddrs[addr.String()]; ok { return ErrAddrBookSelf{addr} diff --git a/p2p/pex/errors.go b/p2p/pex/errors.go index fbee748ac..1f44ceee7 100644 --- a/p2p/pex/errors.go +++ b/p2p/pex/errors.go @@ -54,3 +54,11 @@ type ErrAddrBookInvalidAddr struct { func (err ErrAddrBookInvalidAddr) Error() string { return fmt.Sprintf("Cannot add invalid address %v", err.Addr) } + +type ErrAddrBookInvalidAddrNoID struct { + Addr *p2p.NetAddress +} + +func (err ErrAddrBookInvalidAddrNoID) Error() string { + return fmt.Sprintf("Cannot add address with no ID %v", err.Addr) +} diff --git a/p2p/pex/pex_reactor.go b/p2p/pex/pex_reactor.go index 46a12c488..85d292b09 100644 --- a/p2p/pex/pex_reactor.go +++ b/p2p/pex/pex_reactor.go @@ -221,7 +221,11 @@ func (r *PEXReactor) Receive(chID byte, src Peer, msgBytes []byte) { // 2) limit the output size if r.config.SeedMode { r.SendAddrs(src, r.book.GetSelectionWithBias(biasToSelectNewPeers)) - r.Switch.StopPeerGracefully(src) + go func() { + // TODO Fix properly #2092 + time.Sleep(time.Second * 5) + r.Switch.StopPeerGracefully(src) + }() } else { r.SendAddrs(src, r.book.GetSelection()) } diff --git a/p2p/switch.go b/p2p/switch.go index b1406b9b0..47a42cd09 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -616,7 +616,7 @@ func (sw *Switch) addPeer(p Peer) error { return err } - p.SetLogger(sw.Logger.With("peer", p.NodeInfo().NetAddress().String)) + p.SetLogger(sw.Logger.With("peer", p.NodeInfo().NetAddress())) // All good. Start peer if sw.IsRunning() { From 7a4b62d3be77425e83b489b9e44f6bdec4246ef5 Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Sun, 11 Nov 2018 13:57:08 +0100 Subject: [PATCH 14/17] check the result of `ps.peer.Send` before calling `ps.setHasVote` (#2787) - actually call `ps.SetHasVote` instead to avoid carrying around `votes.Height()`, `votes.Round()`, `types.SignedMsgType(votes.Type())` --- consensus/reactor.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/consensus/reactor.go b/consensus/reactor.go index fc41e5734..12e8e0f14 100644 --- a/consensus/reactor.go +++ b/consensus/reactor.go @@ -1017,7 +1017,11 @@ func (ps *PeerState) PickSendVote(votes types.VoteSetReader) bool { if vote, ok := ps.PickVoteToSend(votes); ok { msg := &VoteMessage{vote} ps.logger.Debug("Sending vote message", "ps", ps, "vote", vote) - return ps.peer.Send(VoteChannel, cdc.MustMarshalBinaryBare(msg)) + if ps.peer.Send(VoteChannel, cdc.MustMarshalBinaryBare(msg)) { + ps.SetHasVote(vote) + return true + } + return false } return false } @@ -1046,7 +1050,6 @@ func (ps *PeerState) PickVoteToSend(votes types.VoteSetReader) (vote *types.Vote return nil, false // Not something worth sending } if index, ok := votes.BitArray().Sub(psVotes).PickRandom(); ok { - ps.setHasVote(height, round, type_, index) return votes.GetByIndex(index), true } return nil, false From 905abf1388a89db2c7ae4ee7df422aaa19796964 Mon Sep 17 00:00:00 2001 From: Mehmet Gurevin Date: Sun, 11 Nov 2018 16:14:52 +0300 Subject: [PATCH 15/17] p2p: re-check after sleeps (#2664) * p2p: re-check after sleeps * use NodeInfo as an interface * Revert "use NodeInfo as an interface" This reverts commit 5f7d055e6c745ac8c8e5a9a7f0bd5ea5bc3d448c. * Revert "p2p: re-check after sleeps" This reverts commit 7f41070da070eadd3312efce1cc821aaf3e23771. * preserve dial to itself * ignore ensured connections while re-connecting * re-check after sleep * keep protocol definition on net addresses * decrease log level * Revert "preserve dial to itself" This reverts commit 0c6e0fc58da78c378c32bb9ded2dd04ad5e754a9. * correct func comment according to modification Co-Authored-By: mgurevin --- p2p/switch.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/p2p/switch.go b/p2p/switch.go index 47a42cd09..b70900ea9 100644 --- a/p2p/switch.go +++ b/p2p/switch.go @@ -328,6 +328,11 @@ func (sw *Switch) reconnectToPeer(addr *NetAddress) { return } + if sw.IsDialingOrExistingAddress(addr) { + sw.Logger.Debug("Peer connection has been established or dialed while we waiting next try", "addr", addr) + return + } + err := sw.DialPeerWithAddress(addr, true) if err == nil { return // success @@ -415,12 +420,15 @@ func (sw *Switch) DialPeersAsync(addrBook AddrBook, peers []string, persistent b if addr.Same(ourAddr) { sw.Logger.Debug("Ignore attempt to connect to ourselves", "addr", addr, "ourAddr", ourAddr) return - } else if sw.IsDialingOrExistingAddress(addr) { + } + + sw.randomSleep(0) + + if sw.IsDialingOrExistingAddress(addr) { sw.Logger.Debug("Ignore attempt to connect to an existing peer", "addr", addr) return } - sw.randomSleep(0) err := sw.DialPeerWithAddress(addr, persistent) if err != nil { switch err.(type) { From 3ff820bdf41663b4fab27f6145dc2a777db08275 Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Sun, 11 Nov 2018 16:09:33 +0100 Subject: [PATCH 16/17] fix amino overhead computation for Tx (#2792) * fix amino overhead computation for Tx: - also count the fieldnum / typ3 - add method to compute overhead per Tx - slightly clarify comment on MaxAminoOverheadForBlock - add tests * fix TestReapMaxBytesMaxGas according to amino overhead * fix TestMempoolFilters according to amino overhead * address review comments: - add a note about fieldNum = 1 - add forgotten godoc comment * fix and use sm.TxPreCheck * fix test * remove print statement --- mempool/mempool.go | 11 +++++--- mempool/mempool_test.go | 16 ++++++------ node/node.go | 13 ++-------- state/execution.go | 10 ++------ state/tx_filter.go | 15 ++++++++--- state/tx_filter_test.go | 22 +++++++++++----- types/block.go | 2 ++ types/block_test.go | 2 +- types/tx.go | 17 ++++++++++++ types/tx_test.go | 57 +++++++++++++++++++++++++++++++++++++++++ 10 files changed, 123 insertions(+), 42 deletions(-) diff --git a/mempool/mempool.go b/mempool/mempool.go index 15e3119c4..b84eb4a68 100644 --- a/mempool/mempool.go +++ b/mempool/mempool.go @@ -11,7 +11,6 @@ import ( "github.com/pkg/errors" - amino "github.com/tendermint/go-amino" abci "github.com/tendermint/tendermint/abci/types" cfg "github.com/tendermint/tendermint/config" auto "github.com/tendermint/tendermint/libs/autofile" @@ -88,8 +87,12 @@ func IsPreCheckError(err error) bool { func PreCheckAminoMaxBytes(maxBytes int64) PreCheckFunc { return func(tx types.Tx) error { // We have to account for the amino overhead in the tx size as well - aminoOverhead := amino.UvarintSize(uint64(len(tx))) - txSize := int64(len(tx) + aminoOverhead) + // NOTE: fieldNum = 1 as types.Block.Data contains Txs []Tx as first field. + // If this field order ever changes this needs to updated here accordingly. + // NOTE: if some []Tx are encoded without a parenting struct, the + // fieldNum is also equal to 1. + aminoOverhead := types.ComputeAminoOverhead(tx, 1) + txSize := int64(len(tx)) + aminoOverhead if txSize > maxBytes { return fmt.Errorf("Tx size (including amino overhead) is too big: %d, max: %d", txSize, maxBytes) @@ -482,7 +485,7 @@ func (mem *Mempool) ReapMaxBytesMaxGas(maxBytes, maxGas int64) types.Txs { for e := mem.txs.Front(); e != nil; e = e.Next() { memTx := e.Value.(*mempoolTx) // Check total size requirement - aminoOverhead := int64(amino.UvarintSize(uint64(len(memTx.tx)))) + aminoOverhead := types.ComputeAminoOverhead(memTx.tx, 1) if maxBytes > -1 && totalBytes+int64(len(memTx.tx))+aminoOverhead > maxBytes { return txs } diff --git a/mempool/mempool_test.go b/mempool/mempool_test.go index e25da7fed..d7ab82737 100644 --- a/mempool/mempool_test.go +++ b/mempool/mempool_test.go @@ -107,11 +107,11 @@ func TestReapMaxBytesMaxGas(t *testing.T) { {20, 0, -1, 0}, {20, 0, 10, 0}, {20, 10, 10, 0}, - {20, 21, 10, 1}, - {20, 210, -1, 10}, - {20, 210, 5, 5}, - {20, 210, 10, 10}, - {20, 210, 15, 10}, + {20, 22, 10, 1}, + {20, 220, -1, 10}, + {20, 220, 5, 5}, + {20, 220, 10, 10}, + {20, 220, 15, 10}, {20, 20000, -1, 20}, {20, 20000, 5, 5}, {20, 20000, 30, 20}, @@ -145,15 +145,15 @@ func TestMempoolFilters(t *testing.T) { {10, nopPreFilter, nopPostFilter, 10}, {10, PreCheckAminoMaxBytes(10), nopPostFilter, 0}, {10, PreCheckAminoMaxBytes(20), nopPostFilter, 0}, - {10, PreCheckAminoMaxBytes(21), nopPostFilter, 10}, + {10, PreCheckAminoMaxBytes(22), nopPostFilter, 10}, {10, nopPreFilter, PostCheckMaxGas(-1), 10}, {10, nopPreFilter, PostCheckMaxGas(0), 0}, {10, nopPreFilter, PostCheckMaxGas(1), 10}, {10, nopPreFilter, PostCheckMaxGas(3000), 10}, {10, PreCheckAminoMaxBytes(10), PostCheckMaxGas(20), 0}, {10, PreCheckAminoMaxBytes(30), PostCheckMaxGas(20), 10}, - {10, PreCheckAminoMaxBytes(21), PostCheckMaxGas(1), 10}, - {10, PreCheckAminoMaxBytes(21), PostCheckMaxGas(0), 0}, + {10, PreCheckAminoMaxBytes(22), PostCheckMaxGas(1), 10}, + {10, PreCheckAminoMaxBytes(22), PostCheckMaxGas(0), 0}, } for tcIndex, tt := range tests { mempool.Update(1, emptyTxArr, tt.preFilter, tt.postFilter) diff --git a/node/node.go b/node/node.go index 4710397fa..0652a392f 100644 --- a/node/node.go +++ b/node/node.go @@ -265,17 +265,8 @@ func NewNode(config *cfg.Config, proxyApp.Mempool(), state.LastBlockHeight, mempl.WithMetrics(memplMetrics), - mempl.WithPreCheck( - mempl.PreCheckAminoMaxBytes( - types.MaxDataBytesUnknownEvidence( - state.ConsensusParams.BlockSize.MaxBytes, - state.Validators.Size(), - ), - ), - ), - mempl.WithPostCheck( - mempl.PostCheckMaxGas(state.ConsensusParams.BlockSize.MaxGas), - ), + mempl.WithPreCheck(sm.TxPreCheck(state)), + mempl.WithPostCheck(sm.TxPostCheck(state)), ) mempoolLogger := logger.With("module", "mempool") mempool.SetLogger(mempoolLogger) diff --git a/state/execution.go b/state/execution.go index cc8e7e75f..4f5a1545c 100644 --- a/state/execution.go +++ b/state/execution.go @@ -8,7 +8,6 @@ import ( dbm "github.com/tendermint/tendermint/libs/db" "github.com/tendermint/tendermint/libs/fail" "github.com/tendermint/tendermint/libs/log" - "github.com/tendermint/tendermint/mempool" "github.com/tendermint/tendermint/proxy" "github.com/tendermint/tendermint/types" ) @@ -180,13 +179,8 @@ func (blockExec *BlockExecutor) Commit( err = blockExec.mempool.Update( block.Height, block.Txs, - mempool.PreCheckAminoMaxBytes( - types.MaxDataBytesUnknownEvidence( - state.ConsensusParams.BlockSize.MaxBytes, - state.Validators.Size(), - ), - ), - mempool.PostCheckMaxGas(state.ConsensusParams.BlockSize.MaxGas), + TxPreCheck(state), + TxPostCheck(state), ) return res.Data, err diff --git a/state/tx_filter.go b/state/tx_filter.go index b8882d8ed..518eb1877 100644 --- a/state/tx_filter.go +++ b/state/tx_filter.go @@ -1,15 +1,22 @@ package state import ( + mempl "github.com/tendermint/tendermint/mempool" "github.com/tendermint/tendermint/types" ) -// TxFilter returns a function to filter transactions. The function limits the -// size of a transaction to the maximum block's data size. -func TxFilter(state State) func(tx types.Tx) bool { +// TxPreCheck returns a function to filter transactions before processing. +// The function limits the size of a transaction to the block's maximum data size. +func TxPreCheck(state State) mempl.PreCheckFunc { maxDataBytes := types.MaxDataBytesUnknownEvidence( state.ConsensusParams.BlockSize.MaxBytes, state.Validators.Size(), ) - return func(tx types.Tx) bool { return int64(len(tx)) <= maxDataBytes } + return mempl.PreCheckAminoMaxBytes(maxDataBytes) +} + +// TxPostCheck returns a function to filter transactions after processing. +// The function limits the gas wanted by a transaction to the block's maximum total gas. +func TxPostCheck(state State) mempl.PostCheckFunc { + return mempl.PostCheckMaxGas(state.ConsensusParams.BlockSize.MaxGas) } diff --git a/state/tx_filter_test.go b/state/tx_filter_test.go index e6b8999f4..52ae396bf 100644 --- a/state/tx_filter_test.go +++ b/state/tx_filter_test.go @@ -18,12 +18,18 @@ func TestTxFilter(t *testing.T) { genDoc := randomGenesisDoc() genDoc.ConsensusParams.BlockSize.MaxBytes = 3000 + // Max size of Txs is much smaller than size of block, + // since we need to account for commits and evidence. testCases := []struct { - tx types.Tx - isTxValid bool + tx types.Tx + isErr bool }{ - {types.Tx(cmn.RandBytes(250)), true}, - {types.Tx(cmn.RandBytes(3001)), false}, + {types.Tx(cmn.RandBytes(250)), false}, + {types.Tx(cmn.RandBytes(1809)), false}, + {types.Tx(cmn.RandBytes(1810)), false}, + {types.Tx(cmn.RandBytes(1811)), true}, + {types.Tx(cmn.RandBytes(1812)), true}, + {types.Tx(cmn.RandBytes(3000)), true}, } for i, tc := range testCases { @@ -31,8 +37,12 @@ func TestTxFilter(t *testing.T) { state, err := LoadStateFromDBOrGenesisDoc(stateDB, genDoc) require.NoError(t, err) - f := TxFilter(state) - assert.Equal(t, tc.isTxValid, f(tc.tx), "#%v", i) + f := TxPreCheck(state) + if tc.isErr { + assert.NotNil(t, f(tc.tx), "#%v", i) + } else { + assert.Nil(t, f(tc.tx), "#%v", i) + } } } diff --git a/types/block.go b/types/block.go index 4ae51d4df..b2cddb5e3 100644 --- a/types/block.go +++ b/types/block.go @@ -21,6 +21,8 @@ const ( // MaxAminoOverheadForBlock - maximum amino overhead to encode a block (up to // MaxBlockSizeBytes in size) not including it's parts except Data. + // This means it also excludes the overhead for individual transactions. + // To compute individual transactions' overhead use types.ComputeAminoOverhead(tx types.Tx, fieldNum int). // // Uvarint length of MaxBlockSizeBytes: 4 bytes // 2 fields (2 embedded): 2 bytes diff --git a/types/block_test.go b/types/block_test.go index cdea293f0..bedd8c8da 100644 --- a/types/block_test.go +++ b/types/block_test.go @@ -250,7 +250,7 @@ func TestMaxHeaderBytes(t *testing.T) { timestamp := time.Date(math.MaxInt64, 0, 0, 0, 0, 0, math.MaxInt64, time.UTC) h := Header{ - Version: version.Consensus{math.MaxInt64, math.MaxInt64}, + Version: version.Consensus{Block: math.MaxInt64, App: math.MaxInt64}, ChainID: maxChainID, Height: math.MaxInt64, Time: timestamp, diff --git a/types/tx.go b/types/tx.go index 10c097e36..41be77946 100644 --- a/types/tx.go +++ b/types/tx.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" + "github.com/tendermint/go-amino" + abci "github.com/tendermint/tendermint/abci/types" "github.com/tendermint/tendermint/crypto/merkle" "github.com/tendermint/tendermint/crypto/tmhash" @@ -118,3 +120,18 @@ type TxResult struct { Tx Tx `json:"tx"` Result abci.ResponseDeliverTx `json:"result"` } + +// ComputeAminoOverhead calculates the overhead for amino encoding a transaction. +// The overhead consists of varint encoding the field number and the wire type +// (= length-delimited = 2), and another varint encoding the length of the +// transaction. +// The field number can be the field number of the particular transaction, or +// the field number of the parenting struct that contains the transactions []Tx +// as a field (this field number is repeated for each contained Tx). +// If some []Tx are encoded directly (without a parenting struct), the default +// fieldNum is also 1 (see BinFieldNum in amino.MarshalBinaryBare). +func ComputeAminoOverhead(tx Tx, fieldNum int) int64 { + fnum := uint64(fieldNum) + typ3AndFieldNum := (uint64(fnum) << 3) | uint64(amino.Typ3_ByteLength) + return int64(amino.UvarintSize(typ3AndFieldNum)) + int64(amino.UvarintSize(uint64(len(tx)))) +} diff --git a/types/tx_test.go b/types/tx_test.go index 6ce23d6f5..3afaaccc1 100644 --- a/types/tx_test.go +++ b/types/tx_test.go @@ -96,6 +96,63 @@ func TestTxProofUnchangable(t *testing.T) { } } +func TestComputeTxsOverhead(t *testing.T) { + cases := []struct { + txs Txs + wantOverhead int + }{ + {Txs{[]byte{6, 6, 6, 6, 6, 6}}, 2}, + // one 21 Mb transaction: + {Txs{make([]byte, 22020096, 22020096)}, 5}, + // two 21Mb/2 sized transactions: + {Txs{make([]byte, 11010048, 11010048), make([]byte, 11010048, 11010048)}, 10}, + {Txs{[]byte{1, 2, 3}, []byte{1, 2, 3}, []byte{4, 5, 6}}, 6}, + {Txs{[]byte{100, 5, 64}, []byte{42, 116, 118}, []byte{6, 6, 6}, []byte{6, 6, 6}}, 8}, + } + + for _, tc := range cases { + totalBytes := int64(0) + totalOverhead := int64(0) + for _, tx := range tc.txs { + aminoOverhead := ComputeAminoOverhead(tx, 1) + totalOverhead += aminoOverhead + totalBytes += aminoOverhead + int64(len(tx)) + } + bz, err := cdc.MarshalBinaryBare(tc.txs) + assert.EqualValues(t, tc.wantOverhead, totalOverhead) + assert.NoError(t, err) + assert.EqualValues(t, len(bz), totalBytes) + } +} + +func TestComputeAminoOverhead(t *testing.T) { + cases := []struct { + tx Tx + fieldNum int + want int + }{ + {[]byte{6, 6, 6}, 1, 2}, + {[]byte{6, 6, 6}, 16, 3}, + {[]byte{6, 6, 6}, 32, 3}, + {[]byte{6, 6, 6}, 64, 3}, + {[]byte{6, 6, 6}, 512, 3}, + {[]byte{6, 6, 6}, 1024, 3}, + {[]byte{6, 6, 6}, 2048, 4}, + {make([]byte, 64), 1, 2}, + {make([]byte, 65), 1, 2}, + {make([]byte, 127), 1, 2}, + {make([]byte, 128), 1, 3}, + {make([]byte, 256), 1, 3}, + {make([]byte, 512), 1, 3}, + {make([]byte, 1024), 1, 3}, + {make([]byte, 128), 16, 4}, + } + for _, tc := range cases { + got := ComputeAminoOverhead(tc.tx, tc.fieldNum) + assert.EqualValues(t, tc.want, got) + } +} + func testTxProofUnchangable(t *testing.T) { // make some proof txs := makeTxs(randInt(2, 100), randInt(16, 128)) From 533b3cfb739aaa432faa25b58a4729305267329e Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sun, 11 Nov 2018 12:08:28 -0500 Subject: [PATCH 17/17] Release/v0.26.1 (#2803) * changelog and version * fix changelog --- CHANGELOG.md | 32 ++++++++++++++++++++++++++++---- CHANGELOG_PENDING.md | 6 +----- version/version.go | 2 +- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd36e3853..eb22e8b84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,31 @@ # Changelog +## v0.26.1 + +*November 11, 2018* + +Special thanks to external contributors on this release: @katakonst + +Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermint). + +### IMPROVEMENTS: + +- [consensus] [\#2704](https://github.com/tendermint/tendermint/issues/2704) Simplify valid POL round logic +- [docs] [\#2749](https://github.com/tendermint/tendermint/issues/2749) Deduplicate some ABCI docs +- [mempool] More detailed log messages + - [\#2724](https://github.com/tendermint/tendermint/issues/2724) + - [\#2762](https://github.com/tendermint/tendermint/issues/2762) + +### BUG FIXES: + +- [autofile] [\#2703](https://github.com/tendermint/tendermint/issues/2703) Do not panic when checking Head size +- [crypto/merkle] [\#2756](https://github.com/tendermint/tendermint/issues/2756) Fix crypto/merkle ProofOperators.Verify to check bounds on keypath parts. +- [mempool] fix a bug where we create a WAL despite `wal_dir` being empty +- [p2p] [\#2771](https://github.com/tendermint/tendermint/issues/2771) Fix `peer-id` label name to `peer_id` in prometheus metrics +- [p2p] [\#2797](https://github.com/tendermint/tendermint/pull/2797) Fix IDs in peer NodeInfo and require them for addresses + in AddressBook +- [p2p] [\#2797](https://github.com/tendermint/tendermint/pull/2797) Do not close conn immediately after sending pex addrs in seed mode. Partial fix for [\#2092](https://github.com/tendermint/tendermint/issues/2092). + ## v0.26.0 *November 2, 2018* @@ -140,8 +166,6 @@ increasing attention to backwards compatibility. Thanks for bearing with us! - [state] [\#2616](https://github.com/tendermint/tendermint/issues/2616) Pass nil to NewValidatorSet() when genesis file's Validators field is nil - [p2p] [\#2555](https://github.com/tendermint/tendermint/issues/2555) Fix p2p switch FlushThrottle value (@goolAdapter) - [p2p] [\#2668](https://github.com/tendermint/tendermint/issues/2668) Reconnect to originally dialed address (not self-reported address) for persistent peers -- [p2p] [\#2797](https://github.com/tendermint/tendermint/pull/2797) AddressBook requires addresses to have IDs; Do not crap out immediately after sending pex addrs in seed mode - ## v0.25.0 @@ -307,8 +331,8 @@ BUG FIXES: *August 22nd, 2018* BUG FIXES: -- [libs/autofile] \#2261 Fix log rotation so it actually happens. - - Fixes issues with consensus WAL growing unbounded ala \#2259 +- [libs/autofile] [\#2261](https://github.com/tendermint/tendermint/issues/2261) Fix log rotation so it actually happens. + - Fixes issues with consensus WAL growing unbounded ala [\#2259](https://github.com/tendermint/tendermint/issues/2259) ## 0.23.0 diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index f63bcdc4b..ea3b97599 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -1,6 +1,6 @@ # Pending -## v0.26.1 +## v0.26.2 *TBA* @@ -26,7 +26,3 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi ### BUG FIXES: -- [crypto/merkle] [\#2756](https://github.com/tendermint/tendermint/issues/2756) Fix crypto/merkle ProofOperators.Verify to check bounds on keypath parts. -- [mempool] fix a bug where we create a WAL despite `wal_dir` being empty -- [p2p] \#2771 Fix `peer-id` label name in prometheus metrics -- [autofile] [\#2703] do not panic when checking Head size diff --git a/version/version.go b/version/version.go index b4664fd77..b7a72a7f7 100644 --- a/version/version.go +++ b/version/version.go @@ -18,7 +18,7 @@ const ( // TMCoreSemVer is the current version of Tendermint Core. // It's the Semantic Version of the software. // Must be a string because scripts like dist.sh read this file. - TMCoreSemVer = "0.26.0" + TMCoreSemVer = "0.26.1" // ABCISemVer is the semantic version of the ABCI library ABCISemVer = "0.15.0"