-
Notifications
You must be signed in to change notification settings - Fork 0
Skip dBFT extensible verification during first node sync #298
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
Conversation
Downloader activity should be tracked irrespectively dBFT engine activity. Even nodes with disabled mining process should be able to verify/broadcast dBFT payloads, and thus, should be aware of Downloader ativity. Signed-off-by: Anna Shaleva <[email protected]>
If node is not yet in sync, then engine must not pass dBFT payloads to the dbft library since dbft can't react properly on them, and sometimes even can't validate them properly (due to #296). Note, this commit is relevant only for mining nodes (with dBFT engine started and active). Signed-off-by: Anna Shaleva <[email protected]>
Nodes (both miners and seeds) can't properly validate consensus payloads if they are syncing. This check prevents both type of nodes from consensus payloads validation if the node is in the process of the initial sync. Next possible Downloader activity is not taken into account. Close #296. Signed-off-by: Anna Shaleva <[email protected]>
It's easy to spam the network via syncing node since it's not able to validate consensus messages. Thus, do not relay dBFT extensible if the node isn't able to properly verify it. Signed-off-by: Anna Shaleva <[email protected]>
|
Just as #296 (comment), can we ignore these unverifiable messages based on block height or something else? |
Yes, we actually perform kind of this check in upper-level dBFT message verification: go-ethereum/eth/protocols/dbft/pool.go Lines 84 to 92 in 7717358
But the case is that for consensus messages |
|
I don't know of any specific reason for doing that, likely it just happened to be this way. Unsynchronized node can't check message validity irrespective of this setting. |
txhsl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
I tested on private net by changing CN nodes, it works as expected |
Should be a part of #298. Signed-off-by: Anna Shaleva <[email protected]>
Close #296.
Testnet sync passes as expected, without unwanted peers disconnections for both dBFT-enabled and dBFT-disabled nodes. There are still a couple of "Can't validate payload" messages in the start of the node functioning, but it happens because Downloader does not start its work immediately after the node start.
T4 CN sync logs
T4 seed sync logs
Mainnet sync is also OK, but mainnet nodes don't have this problem because mainnet is still driven by StandBy validators.
Mainnet CN sync log
Mainnet seed sync logs
@txhsl, @chenquanyu, @songb2 welcome to review and test.