From 7dc4479dc22e0f36277a9d7984bb5b27fbffd701 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Mon, 11 Nov 2024 19:58:12 +0300 Subject: [PATCH 1/4] check: extend ProcessBlock callback with error handler For anti-MEV extension it's possible that some commits carry invalid share which is impossible to check at the commit verification level. Signature shares verification happens inside ProcessBlock callback. If some of Commits contain invalid shares, the node should collect more commits. Hence, ProcessBlock callback is allowed to return an error. Signed-off-by: Anna Shaleva --- CHANGELOG.md | 2 ++ check.go | 16 +++++++++++++--- config.go | 9 +++++---- dbft_test.go | 2 +- internal/consensus/consensus.go | 2 +- internal/simulation/main.go | 3 ++- 6 files changed, 24 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 848e2ffa..61463744 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ New features: Behaviour changes: * adjust behaviour of ProcessPreBlock callback (#129) * (*DBFT).Header() and (*DBFT).PreHeader() are moved to (*Context) receiver (#133) + * support error handling for ProcessBlock callback if anti-MEV extension is enabled + (#134) Improvements: * minimum required Go version is 1.22 (#122, #126) diff --git a/check.go b/check.go index 4109f721..233877a5 100644 --- a/check.go +++ b/check.go @@ -112,8 +112,6 @@ func (d *DBFT[H]) checkCommit() { return } - d.lastBlockIndex = d.BlockIndex - d.lastBlockTime = d.Timer.Now() d.block = d.CreateBlock() hash := d.block.Hash() @@ -124,8 +122,20 @@ func (d *DBFT[H]) checkCommit() { zap.Stringer("merkle", d.block.MerkleRoot()), zap.Stringer("prev", d.block.PrevHash())) + err := d.ProcessBlock(d.block) + if err != nil { + if d.isAntiMEVExtensionEnabled() { + d.Logger.Info("can't process Block, waiting for more Commits to be collected", + zap.Error(err), + zap.Int("count", count)) + return + } + d.Logger.Fatal("block processing failed", zap.Error(err)) + } + + d.lastBlockIndex = d.BlockIndex + d.lastBlockTime = d.Timer.Now() d.blockProcessed = true - d.ProcessBlock(d.block) // Do not initialize consensus process immediately. It's the caller's duty to // start the new block acceptance process and call Reset at the diff --git a/config.go b/config.go index 3db107a2..4e9b4804 100644 --- a/config.go +++ b/config.go @@ -51,7 +51,7 @@ type Config[H Hash] struct { // ProcessBlock is called every time new preBlock is accepted. ProcessPreBlock func(b PreBlock[H]) error // ProcessBlock is called every time new block is accepted. - ProcessBlock func(b Block[H]) + ProcessBlock func(b Block[H]) error // GetBlock should return block with hash. GetBlock func(h H) Block[H] // WatchOnly tells if a node should only watch. @@ -108,7 +108,7 @@ func defaultConfig[H Hash]() *Config[H] { GetVerified: func() []Transaction[H] { return make([]Transaction[H], 0) }, VerifyBlock: func(Block[H]) bool { return true }, Broadcast: func(ConsensusPayload[H]) {}, - ProcessBlock: func(Block[H]) {}, + ProcessBlock: func(Block[H]) error { return nil }, GetBlock: func(H) Block[H] { return nil }, WatchOnly: func() bool { return false }, CurrentHeight: nil, @@ -275,8 +275,9 @@ func WithBroadcast[H Hash](f func(m ConsensusPayload[H])) func(config *Config[H] } } -// WithProcessBlock sets ProcessBlock. -func WithProcessBlock[H Hash](f func(b Block[H])) func(config *Config[H]) { +// WithProcessBlock sets ProcessBlock callback. Note that for anti-MEV extension +// disabled non-nil error return is a no-op. +func WithProcessBlock[H Hash](f func(b Block[H]) error) func(config *Config[H]) { return func(cfg *Config[H]) { cfg.ProcessBlock = f } diff --git a/dbft_test.go b/dbft_test.go index da113e80..2fcdea1a 100644 --- a/dbft_test.go +++ b/dbft_test.go @@ -1131,7 +1131,7 @@ func (s *testState) getOptions() []func(*dbft.Config[crypto.Uint256]) { }), dbft.WithBroadcast[crypto.Uint256](func(p Payload) { s.ch = append(s.ch, p) }), dbft.WithGetTx[crypto.Uint256](s.pool.Get), - dbft.WithProcessBlock[crypto.Uint256](func(b dbft.Block[crypto.Uint256]) { s.blocks = append(s.blocks, b) }), + dbft.WithProcessBlock[crypto.Uint256](func(b dbft.Block[crypto.Uint256]) error { s.blocks = append(s.blocks, b); return nil }), dbft.WithWatchOnly[crypto.Uint256](func() bool { return false }), dbft.WithGetBlock[crypto.Uint256](func(crypto.Uint256) dbft.Block[crypto.Uint256] { return nil }), dbft.WithTimer[crypto.Uint256](timer.New()), diff --git a/internal/consensus/consensus.go b/internal/consensus/consensus.go index c4cd7b69..e0ebdd6d 100644 --- a/internal/consensus/consensus.go +++ b/internal/consensus/consensus.go @@ -13,7 +13,7 @@ func New(logger *zap.Logger, key dbft.PrivateKey, pub dbft.PublicKey, getTx func(uint256 crypto.Uint256) dbft.Transaction[crypto.Uint256], getVerified func() []dbft.Transaction[crypto.Uint256], broadcast func(dbft.ConsensusPayload[crypto.Uint256]), - processBlock func(dbft.Block[crypto.Uint256]), + processBlock func(dbft.Block[crypto.Uint256]) error, currentHeight func() uint32, currentBlockHash func() crypto.Uint256, getValidators func(...dbft.Transaction[crypto.Uint256]) []dbft.PublicKey, diff --git a/internal/simulation/main.go b/internal/simulation/main.go index 0a2973ec..0e12bc1a 100644 --- a/internal/simulation/main.go +++ b/internal/simulation/main.go @@ -179,7 +179,7 @@ func (n *simNode) GetValidators(...dbft.Transaction[crypto.Uint256]) []dbft.Publ return n.validators } -func (n *simNode) ProcessBlock(b dbft.Block[crypto.Uint256]) { +func (n *simNode) ProcessBlock(b dbft.Block[crypto.Uint256]) error { n.d.Logger.Debug("received block", zap.Uint32("height", b.Index())) for _, tx := range b.Transactions() { @@ -188,6 +188,7 @@ func (n *simNode) ProcessBlock(b dbft.Block[crypto.Uint256]) { n.height = b.Index() n.lastHash = b.Hash() + return nil } // VerifyPayload verifies that payload was received from a good validator. From 9af239725fbdd8022ca1bcecda658c1da33b60c2 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 27 Nov 2024 19:00:05 +0300 Subject: [PATCH 2/4] dbft: remove invalid PreCommit message if verification fails Signed-off-by: Anna Shaleva --- CHANGELOG.md | 1 + dbft.go | 1 + 2 files changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61463744..bd7f86ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ Bugs fixed: * enable anti-MEV extension with respect to the current block index (#132) * (*Context).PreBlock() method returns PreHeader instead of PreBlock (#133) * WatchOnly node may send RecoveryMessage on RecoveryRequest (#135) + * invalid PreCommit message is not removed from cache (#134) ## [0.3.0] (01 August 2024) diff --git a/dbft.go b/dbft.go index 01f4774c..18999e11 100644 --- a/dbft.go +++ b/dbft.go @@ -537,6 +537,7 @@ func (d *DBFT[H]) onPreCommit(msg ConsensusPayload[H]) { d.PreCommitPayloads[msg.ValidatorIndex()] = msg if d.ViewNumber == msg.ViewNumber() { if err := d.VerifyPreCommit(msg); err != nil { + d.PreCommitPayloads[msg.ValidatorIndex()] = nil d.Logger.Warn("invalid PreCommit", zap.Uint16("from", msg.ValidatorIndex()), zap.String("error", err.Error())) return } From 77ce58173a9ce34f6ea885642e68495cd20ba8e7 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 27 Nov 2024 19:01:10 +0300 Subject: [PATCH 3/4] dbft: log err details on invalid commit For better user experience. Signed-off-by: Anna Shaleva --- CHANGELOG.md | 1 + dbft.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd7f86ae..51621eb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ Behaviour changes: Improvements: * minimum required Go version is 1.22 (#122, #126) + * log Commit signature verification error (#134) Bugs fixed: * context-bound PreBlock and PreHeader are not reset properly (#127) diff --git a/dbft.go b/dbft.go index 18999e11..6fe2e328 100644 --- a/dbft.go +++ b/dbft.go @@ -591,12 +591,13 @@ func (d *DBFT[H]) onCommit(msg ConsensusPayload[H]) { header := d.MakeHeader() if header != nil { pub := d.Validators[msg.ValidatorIndex()] - if header.Verify(pub, msg.GetCommit().Signature()) == nil { + if err := header.Verify(pub, msg.GetCommit().Signature()); err == nil { d.checkCommit() } else { d.CommitPayloads[msg.ValidatorIndex()] = nil d.Logger.Warn("invalid commit signature", zap.Uint("validator", uint(msg.ValidatorIndex())), + zap.Error(err), ) } } From be64b5f47672278bf4763d1eeb1e34cb2e7ad71f Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Wed, 27 Nov 2024 19:07:49 +0300 Subject: [PATCH 4/4] dbft: add commit verification callback It's required by anti-MEV extension to perform state-independent TPKE signature verification. Signed-off-by: Anna Shaleva --- CHANGELOG.md | 1 + config.go | 12 ++++++++++++ dbft.go | 6 ++++++ dbft_test.go | 1 + internal/consensus/consensus.go | 1 + 5 files changed, 21 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51621eb8..17ad9774 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ Behaviour changes: Improvements: * minimum required Go version is 1.22 (#122, #126) * log Commit signature verification error (#134) + * add Commit message verification callback (#134) Bugs fixed: * context-bound PreBlock and PreHeader are not reset properly (#127) diff --git a/config.go b/config.go index 4e9b4804..104d423d 100644 --- a/config.go +++ b/config.go @@ -89,6 +89,10 @@ type Config[H Hash] struct { // Note that PreBlock-dependent PreCommit verification should be performed inside PreBlock.Verify // callback. VerifyPreCommit func(p ConsensusPayload[H]) error + // VerifyCommit performs external Commit verification and returns nil if it's successful. + // Note that Block-dependent Commit verification should be performed inside Block.Verify + // callback. + VerifyCommit func(p ConsensusPayload[H]) error } const defaultSecondsPerBlock = time.Second * 15 @@ -117,6 +121,7 @@ func defaultConfig[H Hash]() *Config[H] { VerifyPrepareRequest: func(ConsensusPayload[H]) error { return nil }, VerifyPrepareResponse: func(ConsensusPayload[H]) error { return nil }, + VerifyCommit: func(ConsensusPayload[H]) error { return nil }, AntiMEVExtensionEnablingHeight: -1, VerifyPreBlock: func(PreBlock[H]) bool { return true }, @@ -401,3 +406,10 @@ func WithVerifyPreCommit[H Hash](f func(preCommit ConsensusPayload[H]) error) fu cfg.VerifyPreCommit = f } } + +// WithVerifyCommit sets VerifyCommit. +func WithVerifyCommit[H Hash](f func(commit ConsensusPayload[H]) error) func(config *Config[H]) { + return func(cfg *Config[H]) { + cfg.VerifyCommit = f + } +} diff --git a/dbft.go b/dbft.go index 6fe2e328..9a1b0748 100644 --- a/dbft.go +++ b/dbft.go @@ -586,6 +586,12 @@ func (d *DBFT[H]) onCommit(msg ConsensusPayload[H]) { } d.CommitPayloads[msg.ValidatorIndex()] = msg if d.ViewNumber == msg.ViewNumber() { + if err := d.VerifyCommit(msg); err != nil { + d.CommitPayloads[msg.ValidatorIndex()] = nil + d.Logger.Warn("invalid Commit", zap.Uint16("from", msg.ValidatorIndex()), zap.String("error", err.Error())) + return + } + d.Logger.Info("received Commit", zap.Uint("validator", uint(msg.ValidatorIndex()))) d.extendTimer(4) header := d.MakeHeader() diff --git a/dbft_test.go b/dbft_test.go index 2fcdea1a..cd0e8f15 100644 --- a/dbft_test.go +++ b/dbft_test.go @@ -1150,6 +1150,7 @@ func (s *testState) getOptions() []func(*dbft.Config[crypto.Uint256]) { dbft.WithNewRecoveryMessage[crypto.Uint256](func() dbft.RecoveryMessage[crypto.Uint256] { return consensus.NewRecoveryMessage(nil) }), + dbft.WithVerifyCommit[crypto.Uint256](func(p dbft.ConsensusPayload[crypto.Uint256]) error { return nil }), } verify := s.verify diff --git a/internal/consensus/consensus.go b/internal/consensus/consensus.go index e0ebdd6d..37653efc 100644 --- a/internal/consensus/consensus.go +++ b/internal/consensus/consensus.go @@ -40,6 +40,7 @@ func New(logger *zap.Logger, key dbft.PrivateKey, pub dbft.PublicKey, dbft.WithGetValidators[crypto.Uint256](getValidators), dbft.WithVerifyPrepareRequest[crypto.Uint256](verifyPayload), dbft.WithVerifyPrepareResponse[crypto.Uint256](verifyPayload), + dbft.WithVerifyCommit[crypto.Uint256](verifyPayload), dbft.WithNewBlockFromContext[crypto.Uint256](newBlockFromContext), dbft.WithNewConsensusPayload[crypto.Uint256](defaultNewConsensusPayload),