From ace06fc7e8305a349906e202f34f987753325ccb Mon Sep 17 00:00:00 2001 From: Daniel T <30197399+danwt@users.noreply.github.com> Date: Mon, 13 Jan 2025 10:52:53 +0000 Subject: [PATCH] fix(validation): fix nil deref , and use validation result (#1325) --- block/manager.go | 4 + .../dymint/p2p/mock_StateGetter.go | 74 +++++++++++-------- p2p/block_sync_test.go | 5 +- p2p/client.go | 11 ++- p2p/client_test.go | 17 +++-- p2p/gossip.go | 6 +- p2p/validator.go | 41 ++++++---- p2p/validator_test.go | 35 ++++----- types/state.go | 10 +++ 9 files changed, 122 insertions(+), 81 deletions(-) diff --git a/block/manager.go b/block/manager.go index b4d2170fa..4b1b5cdc0 100644 --- a/block/manager.go +++ b/block/manager.go @@ -348,6 +348,10 @@ func (m *Manager) GetProposerPubKey() tmcrypto.PubKey { return m.State.GetProposerPubKey() } +func (m *Manager) SafeProposerPubKey() (tmcrypto.PubKey, error) { + return m.State.SafeProposerPubKey() +} + func (m *Manager) GetRevision() uint64 { return m.State.GetRevision() } diff --git a/mocks/github.com/dymensionxyz/dymint/p2p/mock_StateGetter.go b/mocks/github.com/dymensionxyz/dymint/p2p/mock_StateGetter.go index 92afd5fff..a75ce7cba 100644 --- a/mocks/github.com/dymensionxyz/dymint/p2p/mock_StateGetter.go +++ b/mocks/github.com/dymensionxyz/dymint/p2p/mock_StateGetter.go @@ -20,94 +20,104 @@ func (_m *MockStateGetter) EXPECT() *MockStateGetter_Expecter { return &MockStateGetter_Expecter{mock: &_m.Mock} } -// GetProposerPubKey provides a mock function with no fields -func (_m *MockStateGetter) GetProposerPubKey() crypto.PubKey { +// GetRevision provides a mock function with no fields +func (_m *MockStateGetter) GetRevision() uint64 { ret := _m.Called() if len(ret) == 0 { - panic("no return value specified for GetProposerPubKey") + panic("no return value specified for GetRevision") } - var r0 crypto.PubKey - if rf, ok := ret.Get(0).(func() crypto.PubKey); ok { + var r0 uint64 + if rf, ok := ret.Get(0).(func() uint64); ok { r0 = rf() } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(crypto.PubKey) - } + r0 = ret.Get(0).(uint64) } return r0 } -// MockStateGetter_GetProposerPubKey_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetProposerPubKey' -type MockStateGetter_GetProposerPubKey_Call struct { +// MockStateGetter_GetRevision_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetRevision' +type MockStateGetter_GetRevision_Call struct { *mock.Call } -// GetProposerPubKey is a helper method to define mock.On call -func (_e *MockStateGetter_Expecter) GetProposerPubKey() *MockStateGetter_GetProposerPubKey_Call { - return &MockStateGetter_GetProposerPubKey_Call{Call: _e.mock.On("GetProposerPubKey")} +// GetRevision is a helper method to define mock.On call +func (_e *MockStateGetter_Expecter) GetRevision() *MockStateGetter_GetRevision_Call { + return &MockStateGetter_GetRevision_Call{Call: _e.mock.On("GetRevision")} } -func (_c *MockStateGetter_GetProposerPubKey_Call) Run(run func()) *MockStateGetter_GetProposerPubKey_Call { +func (_c *MockStateGetter_GetRevision_Call) Run(run func()) *MockStateGetter_GetRevision_Call { _c.Call.Run(func(args mock.Arguments) { run() }) return _c } -func (_c *MockStateGetter_GetProposerPubKey_Call) Return(_a0 crypto.PubKey) *MockStateGetter_GetProposerPubKey_Call { +func (_c *MockStateGetter_GetRevision_Call) Return(_a0 uint64) *MockStateGetter_GetRevision_Call { _c.Call.Return(_a0) return _c } -func (_c *MockStateGetter_GetProposerPubKey_Call) RunAndReturn(run func() crypto.PubKey) *MockStateGetter_GetProposerPubKey_Call { +func (_c *MockStateGetter_GetRevision_Call) RunAndReturn(run func() uint64) *MockStateGetter_GetRevision_Call { _c.Call.Return(run) return _c } -// GetRevision provides a mock function with no fields -func (_m *MockStateGetter) GetRevision() uint64 { +// SafeProposerPubKey provides a mock function with no fields +func (_m *MockStateGetter) SafeProposerPubKey() (crypto.PubKey, error) { ret := _m.Called() if len(ret) == 0 { - panic("no return value specified for GetRevision") + panic("no return value specified for SafeProposerPubKey") } - var r0 uint64 - if rf, ok := ret.Get(0).(func() uint64); ok { + var r0 crypto.PubKey + var r1 error + if rf, ok := ret.Get(0).(func() (crypto.PubKey, error)); ok { + return rf() + } + if rf, ok := ret.Get(0).(func() crypto.PubKey); ok { r0 = rf() } else { - r0 = ret.Get(0).(uint64) + if ret.Get(0) != nil { + r0 = ret.Get(0).(crypto.PubKey) + } } - return r0 + if rf, ok := ret.Get(1).(func() error); ok { + r1 = rf() + } else { + r1 = ret.Error(1) + } + + return r0, r1 } -// MockStateGetter_GetRevision_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetRevision' -type MockStateGetter_GetRevision_Call struct { +// MockStateGetter_SafeProposerPubKey_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'SafeProposerPubKey' +type MockStateGetter_SafeProposerPubKey_Call struct { *mock.Call } -// GetRevision is a helper method to define mock.On call -func (_e *MockStateGetter_Expecter) GetRevision() *MockStateGetter_GetRevision_Call { - return &MockStateGetter_GetRevision_Call{Call: _e.mock.On("GetRevision")} +// SafeProposerPubKey is a helper method to define mock.On call +func (_e *MockStateGetter_Expecter) SafeProposerPubKey() *MockStateGetter_SafeProposerPubKey_Call { + return &MockStateGetter_SafeProposerPubKey_Call{Call: _e.mock.On("SafeProposerPubKey")} } -func (_c *MockStateGetter_GetRevision_Call) Run(run func()) *MockStateGetter_GetRevision_Call { +func (_c *MockStateGetter_SafeProposerPubKey_Call) Run(run func()) *MockStateGetter_SafeProposerPubKey_Call { _c.Call.Run(func(args mock.Arguments) { run() }) return _c } -func (_c *MockStateGetter_GetRevision_Call) Return(_a0 uint64) *MockStateGetter_GetRevision_Call { - _c.Call.Return(_a0) +func (_c *MockStateGetter_SafeProposerPubKey_Call) Return(_a0 crypto.PubKey, _a1 error) *MockStateGetter_SafeProposerPubKey_Call { + _c.Call.Return(_a0, _a1) return _c } -func (_c *MockStateGetter_GetRevision_Call) RunAndReturn(run func() uint64) *MockStateGetter_GetRevision_Call { +func (_c *MockStateGetter_SafeProposerPubKey_Call) RunAndReturn(run func() (crypto.PubKey, error)) *MockStateGetter_SafeProposerPubKey_Call { _c.Call.Return(run) return _c } diff --git a/p2p/block_sync_test.go b/p2p/block_sync_test.go index dcbe124e5..3bf98a24d 100644 --- a/p2p/block_sync_test.go +++ b/p2p/block_sync_test.go @@ -8,6 +8,7 @@ import ( "github.com/dymensionxyz/dymint/testutil" "github.com/dymensionxyz/dymint/version" "github.com/ipfs/go-datastore" + pubsub "github.com/libp2p/go-libp2p-pubsub" "github.com/stretchr/testify/require" "github.com/tendermint/tendermint/libs/log" ) @@ -23,8 +24,8 @@ func TestBlockSync(t *testing.T) { require.NotNil(t, manager) // required for tx validator - assertRecv := func(tx *p2p.GossipMessage) bool { - return true + assertRecv := func(tx *p2p.GossipMessage) pubsub.ValidationResult { + return pubsub.ValidationAccept } // Create a block for height 1 diff --git a/p2p/client.go b/p2p/client.go index 596669f99..4f395a2f5 100644 --- a/p2p/client.go +++ b/p2p/client.go @@ -523,8 +523,8 @@ func (c *Client) getBlockTopic() string { // NewTxValidator creates a pubsub validator that uses the node's mempool to check the // transaction. If the transaction is valid, then it is added to the mempool func (c *Client) NewTxValidator() GossipValidator { - return func(g *GossipMessage) bool { - return true + return func(g *GossipMessage) pubsub.ValidationResult { + return pubsub.ValidationAccept } } @@ -640,7 +640,12 @@ func (c *Client) retrieveBlockSyncLoop(ctx context.Context, msgHandler BlockSync if err != nil { return } - if err := block.Validate(state.GetProposerPubKey()); err != nil { + propKey, err := state.SafeProposerPubKey() + if err != nil { + c.logger.Error("Get proposer public key.", "err", err) + continue + } + if err := block.Validate(propKey); err != nil { c.logger.Error("Failed to validate blocksync block.", "height", block.Block.Header.Height) continue } diff --git a/p2p/client_test.go b/p2p/client_test.go index ab56f09ae..ce15cf03e 100644 --- a/p2p/client_test.go +++ b/p2p/client_test.go @@ -25,6 +25,7 @@ import ( "github.com/dymensionxyz/dymint/config" "github.com/dymensionxyz/dymint/p2p" "github.com/dymensionxyz/dymint/testutil" + p2ppubsub "github.com/libp2p/go-libp2p-pubsub" ) func TestClientStartup(t *testing.T) { @@ -104,17 +105,17 @@ func TestGossiping(t *testing.T) { wg.Add(2) // ensure that Tx is delivered to client - assertRecv := func(tx *p2p.GossipMessage) bool { + assertRecv := func(tx *p2p.GossipMessage) p2ppubsub.ValidationResult { logger.Debug("received tx", "body", string(tx.Data), "from", tx.From) assert.Equal(expectedMsg, tx.Data) wg.Done() - return true + return p2ppubsub.ValidationAccept } // ensure that Tx is not delivered to client - assertNotRecv := func(*p2p.GossipMessage) bool { + assertNotRecv := func(*p2p.GossipMessage) p2ppubsub.ValidationResult { t.Fatal("unexpected Tx received") - return false + return p2ppubsub.ValidationReject } validators := []p2p.GossipValidator{assertRecv, assertNotRecv, assertNotRecv, assertRecv, assertNotRecv} @@ -151,8 +152,8 @@ func TestAdvertiseBlock(t *testing.T) { ctx := context.Background() // required for tx validator - assertRecv := func(tx *p2p.GossipMessage) bool { - return true + assertRecv := func(tx *p2p.GossipMessage) p2ppubsub.ValidationResult { + return p2ppubsub.ValidationAccept } // Create a cid manually by specifying the 'prefix' parameters @@ -201,8 +202,8 @@ func TestAdvertiseWrongCid(t *testing.T) { ctx := context.Background() // required for tx validator - assertRecv := func(tx *p2p.GossipMessage) bool { - return true + assertRecv := func(tx *p2p.GossipMessage) p2ppubsub.ValidationResult { + return p2ppubsub.ValidationAccept } validators := []p2p.GossipValidator{assertRecv, assertRecv, assertRecv, assertRecv, assertRecv} diff --git a/p2p/gossip.go b/p2p/gossip.go index 6d4236e4c..2a2290262 100644 --- a/p2p/gossip.go +++ b/p2p/gossip.go @@ -108,12 +108,12 @@ func (g *Gossiper) ProcessMessages(ctx context.Context) { } } -func wrapValidator(gossiper *Gossiper, validator GossipValidator) pubsub.Validator { - return func(_ context.Context, _ peer.ID, msg *pubsub.Message) bool { +func wrapValidator(gossiper *Gossiper, validator GossipValidator) pubsub.ValidatorEx { + return func(_ context.Context, _ peer.ID, msg *pubsub.Message) pubsub.ValidationResult { // Make sure we don't process our own messages. // In this case we'll want to return true but not to actually handle the message. if msg.GetFrom() == gossiper.ownID { - return true + return pubsub.ValidationAccept } return validator(&GossipMessage{ Data: msg.Data, diff --git a/p2p/validator.go b/p2p/validator.go index 513714e4f..167e9de52 100644 --- a/p2p/validator.go +++ b/p2p/validator.go @@ -6,23 +6,24 @@ import ( "github.com/dymensionxyz/dymint/mempool" nodemempool "github.com/dymensionxyz/dymint/node/mempool" "github.com/dymensionxyz/dymint/types" + pubsub "github.com/libp2p/go-libp2p-pubsub" abci "github.com/tendermint/tendermint/abci/types" tmcrypto "github.com/tendermint/tendermint/crypto" corep2p "github.com/tendermint/tendermint/p2p" ) type StateGetter interface { - GetProposerPubKey() tmcrypto.PubKey + SafeProposerPubKey() (tmcrypto.PubKey, error) GetRevision() uint64 } // GossipValidator is a callback function type. -type GossipValidator func(*GossipMessage) bool +type GossipValidator func(*GossipMessage) pubsub.ValidationResult // IValidator is an interface for implementing validators of messages gossiped in the p2p network. type IValidator interface { // TxValidator creates a pubsub validator that uses the node's mempool to check the - // transaction. If the transaction is valid, then it is added to the mempool + // transaction. If the transaction is want, then it is added to the mempool TxValidator(mp mempool.Mempool, mpoolIDS *nodemempool.MempoolIDs) GossipValidator } @@ -46,7 +47,7 @@ func NewValidator(logger types.Logger, blockmanager StateGetter) *Validator { // transaction. // False means the TX is considered invalid and should not be gossiped. func (v *Validator) TxValidator(mp mempool.Mempool, mpoolIDS *nodemempool.MempoolIDs) GossipValidator { - return func(txMessage *GossipMessage) bool { + return func(txMessage *GossipMessage) pubsub.ValidationResult { v.logger.Debug("Transaction received.", "bytes", len(txMessage.Data)) var res *abci.Response err := mp.CheckTx(txMessage.Data, func(resp *abci.Response) { @@ -57,38 +58,46 @@ func (v *Validator) TxValidator(mp mempool.Mempool, mpoolIDS *nodemempool.Mempoo }) switch { case errors.Is(err, mempool.ErrTxInCache): - return true + return pubsub.ValidationAccept case errors.Is(err, mempool.ErrMempoolIsFull{}): - return true // we have no reason to believe that we should throw away the message + return pubsub.ValidationAccept // we have no reason to believe that we should throw away the message case errors.Is(err, mempool.ErrTxTooLarge{}): - return false + return pubsub.ValidationReject case errors.Is(err, mempool.ErrPreCheck{}): - return false + return pubsub.ValidationReject case err != nil: v.logger.Error("Check tx.", "error", err) - return false + return pubsub.ValidationReject } - return res.GetCheckTx().Code == abci.CodeTypeOK + if res.GetCheckTx().Code == abci.CodeTypeOK { + return pubsub.ValidationAccept + } + return pubsub.ValidationReject } } // BlockValidator runs basic checks on the gossiped block func (v *Validator) BlockValidator() GossipValidator { - return func(blockMsg *GossipMessage) bool { + return func(blockMsg *GossipMessage) pubsub.ValidationResult { var gossipedBlock BlockData if err := gossipedBlock.UnmarshalBinary(blockMsg.Data); err != nil { v.logger.Error("Deserialize gossiped block.", "error", err) - return false + return pubsub.ValidationReject } if v.stateGetter.GetRevision() != gossipedBlock.Block.GetRevision() { - return false + return pubsub.ValidationReject + } + propKey, err := v.stateGetter.SafeProposerPubKey() + if err != nil { + v.logger.Error("Get proposer pubkey.", "error", err) + return pubsub.ValidationIgnore } - if err := gossipedBlock.Validate(v.stateGetter.GetProposerPubKey()); err != nil { + if err := gossipedBlock.Validate(propKey); err != nil { v.logger.Error("P2P block validation.", "height", gossipedBlock.Block.Header.Height, "err", err) - return false + return pubsub.ValidationReject } - return true + return pubsub.ValidationAccept } } diff --git a/p2p/validator_test.go b/p2p/validator_test.go index 5e2f05159..bfce589ce 100644 --- a/p2p/validator_test.go +++ b/p2p/validator_test.go @@ -5,6 +5,7 @@ import ( mempoolv1 "github.com/dymensionxyz/dymint/mempool/v1" "github.com/dymensionxyz/dymint/types" + pubsub "github.com/libp2p/go-libp2p-pubsub" "github.com/libp2p/go-libp2p/core/peer" "github.com/stretchr/testify/assert" @@ -35,50 +36,50 @@ func TestValidator_TxValidator(t *testing.T) { tests := []struct { name string args args - want bool + want pubsub.ValidationResult }{ { - name: "valid: tx already in cache", + name: "want: tx already in cache", args: args{ mp: &mockMP{err: mempool.ErrTxInCache}, numMsgs: 3, }, - want: true, + want: pubsub.ValidationAccept, }, { - name: "valid: mempool is full", + name: "want: mempool is full", args: args{ mp: &mockMP{err: mempool.ErrMempoolIsFull{}}, numMsgs: 3, }, - want: true, + want: pubsub.ValidationAccept, }, { name: "invalid: tx too large", args: args{ mp: &mockMP{err: mempool.ErrTxTooLarge{}}, numMsgs: 3, }, - want: false, + want: pubsub.ValidationReject, }, { name: "invalid: pre-check error", args: args{ mp: &mockMP{err: mempool.ErrPreCheck{}}, numMsgs: 3, }, - want: false, + want: pubsub.ValidationReject, }, { - name: "valid: no error", + name: "want: no error", args: args{ mp: &mockMP{}, numMsgs: 3, }, - want: true, + want: pubsub.ValidationAccept, }, { name: "unknown error", args: args{ mp: &mockMP{err: assert.AnError}, numMsgs: 3, }, - want: false, + want: pubsub.ValidationReject, }, } for _, tt := range tests { @@ -100,16 +101,16 @@ func TestValidator_BlockValidator(t *testing.T) { tests := []struct { name string proposerKey ed25519.PrivKey - valid bool + want pubsub.ValidationResult }{ { - name: "valid: block signed by proposer", + name: "want: block signed by proposer", proposerKey: proposerKey, - valid: true, + want: pubsub.ValidationAccept, }, { name: "invalid: bad signer", proposerKey: attackerKey, - valid: false, + want: pubsub.ValidationReject, }, } for _, tt := range tests { @@ -139,7 +140,7 @@ func TestValidator_BlockValidator(t *testing.T) { block := executor.CreateBlock(1, &types.Commit{}, [32]byte{}, [32]byte(state.GetProposerHash()), state, maxBytes) getProposer := &p2pmock.MockStateGetter{} - getProposer.On("GetProposerPubKey").Return(proposerKey.PubKey()) + getProposer.On("SafeProposerPubKey").Return(proposerKey.PubKey(), nil) getProposer.On("GetRevision").Return(uint64(0)) // Create commit for the block @@ -147,7 +148,7 @@ func TestValidator_BlockValidator(t *testing.T) { abciHeaderBytes, err := abciHeaderPb.Marshal() require.NoError(t, err) var signature []byte - if tt.valid { + if tt.want == pubsub.ValidationAccept { signature, err = proposerKey.Sign(abciHeaderBytes) require.NoError(t, err) } else { @@ -172,7 +173,7 @@ func TestValidator_BlockValidator(t *testing.T) { // Check block validity validateBlock := p2p.NewValidator(logger, getProposer).BlockValidator() valid := validateBlock(blockMsg) - require.Equal(t, tt.valid, valid) + require.Equal(t, tt.want, valid) }) } } diff --git a/types/state.go b/types/state.go index aa96bc985..b306eeb80 100644 --- a/types/state.go +++ b/types/state.go @@ -7,6 +7,7 @@ import ( // TODO(tzdybal): copy to local project? + "github.com/dymensionxyz/gerr-cosmos/gerrc" tmcrypto "github.com/tendermint/tendermint/crypto" tmstate "github.com/tendermint/tendermint/proto/tendermint/state" tmproto "github.com/tendermint/tendermint/proto/tendermint/types" @@ -51,6 +52,7 @@ func (s *State) GetProposer() *Sequencer { return s.Proposer.Load() } +// Warning: can be nil during 'graceful' shutdown func (s *State) GetProposerPubKey() tmcrypto.PubKey { proposer := s.Proposer.Load() if proposer == nil { @@ -59,6 +61,14 @@ func (s *State) GetProposerPubKey() tmcrypto.PubKey { return proposer.PubKey() } +func (s *State) SafeProposerPubKey() (tmcrypto.PubKey, error) { + ret := s.GetProposerPubKey() + if ret == nil { + return nil, gerrc.ErrNotFound.Wrap("proposer") + } + return ret, nil +} + // GetProposerHash returns the hash of the proposer func (s *State) GetProposerHash() []byte { proposer := s.Proposer.Load()