Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(validation): fix nil deref , and use validation result #1325

Merged
merged 4 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions block/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
74 changes: 42 additions & 32 deletions mocks/github.com/dymensionxyz/dymint/p2p/mock_StateGetter.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions p2p/block_sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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
Expand Down
11 changes: 8 additions & 3 deletions p2p/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}

Expand Down Expand Up @@ -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
}
Expand Down
17 changes: 9 additions & 8 deletions p2p/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}
Expand Down
6 changes: 3 additions & 3 deletions p2p/gossip.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
41 changes: 25 additions & 16 deletions p2p/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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) {
Expand All @@ -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
}
}
Loading
Loading