Skip to content

Commit

Permalink
check: prohibit WatchOnly to send commits
Browse files Browse the repository at this point in the history
Problem:

Inappropriate message in the WatchOnly node logs with NeoXAMEV extension
enabled:
```
2024-12-05T15:15:54.366+0300	INFO	[email protected]/dbft.go:545	received PreCommit	{"validator": 5}
2024-12-05T15:15:54.366+0300	DEBUG	[email protected]/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. #118 (comment).

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 <[email protected]>
  • Loading branch information
AnnaShaleva committed Dec 5, 2024
1 parent 7c005d9 commit 28758a4
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 3 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
11 changes: 8 additions & 3 deletions check.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}

Check warning on line 95 in check.go

View check run for this annotation

Codecov / codecov/patch

check.go#L93-L95

Added lines #L93 - L95 were not covered by tests
}
}

Expand Down

0 comments on commit 28758a4

Please sign in to comment.