From 28758a47687c72dfab018ae05e8a38eb11cdbca2 Mon Sep 17 00:00:00 2001 From: Anna Shaleva Date: Thu, 5 Dec 2024 15:23:50 +0300 Subject: [PATCH] check: prohibit WatchOnly to send commits Problem: Inappropriate message in the WatchOnly node logs with NeoXAMEV extension enabled: ``` 2024-12-05T15:15:54.366+0300 INFO dbft@v0.3.1/dbft.go:545 received PreCommit {"validator": 5} 2024-12-05T15:15:54.366+0300 DEBUG dbft@v0.3.1/check.go:90 can't send commit since self preCommit not yet sent ``` Based on these logs it may seem that WatchOnly is trying (or will try) to send Commit. It must never happen, hence I dived into the library code and discovered the condition that was added mostly for safety and that was planned to be removed in future: https://github.com/nspcc-dev/dbft/blob/7c005d93e4b47106b679e3cc55d182f8167e0e56/check.go#L82-L84 Ref. https://github.com/nspcc-dev/dbft/pull/118#discussion_r1681631842. So the problem is in the invalid documentaiton and in improper log message. It should be noted that for non-WatchOnly CNs we also need to keep and check PreCommitSent requirement because node's Commit must be confirmed by previousely sent PreCommit. Solution: Strictly speaking, this commit almost doesn't change the dBFT behaviour because WatchOnly node is not allowed to send Commit thanks to the condition outlined above. But we'd better adjust the comment and don't confuse the library users with inappropriate message. Signed-off-by: Anna Shaleva --- CHANGELOG.md | 2 ++ check.go | 11 ++++++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e903db59..47068308 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ Behaviour changes: Improvements: Bugs fixed: + * inappropriate log on attempt to construct Commit for anti-MEV enabled WatchOnly + (#139) ## [0.3.1] (29 November 2024) diff --git a/check.go b/check.go index 233877a5..cfc84e46 100644 --- a/check.go +++ b/check.go @@ -79,15 +79,20 @@ func (d *DBFT[H]) checkPreCommit() { d.preBlockProcessed = true } - // Require PreCommit sent by self for reliability. This condition may be removed - // in the future. + // Require PreCommit sent by self for reliability. This condition must not be + // removed because: + // 1) we need to filter out WatchOnly nodes; + // 2) CNs that have not sent PreCommit must not skip this stage (although it's OK + // from the DKG/TPKE side to build final Block based only on other CN's data). if d.PreCommitSent() { d.verifyCommitPayloadsAgainstHeader() d.sendCommit() d.changeTimer(d.SecondsPerBlock) d.checkCommit() } else { - d.Logger.Debug("can't send commit since self preCommit not yet sent") + if !d.Context.WatchOnly() { + d.Logger.Debug("can't send commit since self preCommit not yet sent") + } } }