Aura: Report malice on sibling blocks from the same validator#11160
Conversation
|
It looks like @afck signed our Contributor License Agreement. 👍 Many thanks, Parity Technologies CLA Bot |
|
failing RPC test strikes again |
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
So the code looks good to me (modulo minor nitpick) but I do not understand the full background&context to the change. Not sure if it's obvious to an expert Aura code reader but I for one would greatly appreciate some more info (perhaps as module-level docs, or if too trivial for that, here in the PR description).
| // Report malice if the validator produced other sibling blocks in the same step. | ||
| let received_step_key = (step, *header.author()); | ||
| let new_hash = header.hash(); | ||
| if self.received_step_hashes.read().get(&received_step_key).into_iter().any(|h| *h != new_hash) { |
There was a problem hiding this comment.
I must be missing something, self.received_step_hashes.read().get(&received_step_key) returns a single H256 yes? Why not if self.received_step_hashes.read().get(&received_step_key) != Some(&new_hash)?
There was a problem hiding this comment.
a != Some(b) is true if a is None.
a.into_iter().any(|h| h != b) is false if a is None.
We could use a.map_or(false, |h| h != b). Not sure if that's more readable. After all, we want to say something like: "If the map already contains a hash, and that's different from new_hash, report the block author."
There was a problem hiding this comment.
IMHO, map_or is more readable in this case
| } | ||
|
|
||
| // Remove older step hash records. | ||
| const SIBLING_MALICE_DETECTION_PERIOD: u64 = 100; |
There was a problem hiding this comment.
It'd be nice to see a comment on why 100 was chosen.
It would be really great to get the opinion of one of those! As far as I understand, this PR fixes an omission: A validator does get reported as malicious if they create a block whose step is not greater than its parent's step. However, they do not get reported if they create two sibling blocks in the same step, which shouldn't be allowed in Aura either. There's no good reason for the number 100, I think: It just needs to be large enough so that it's likely that any single-slot sibling block pair that could still do damage to consensus would be detected and reported. And not large enough to waste too much memory. (In that respect, 100 seems like a conservative choice to me.) I'll add some documentation and comments, as far as I can. |
|
I think it might make sense to use the number of validators N instead of 100 here, or maybe 2 N: That would mean we are always able to detect multiple blocks per step in the current and the previous full cycle of steps. After a full cycle, blocks are likely to be finalized, since every validator has already built on top of them, and an attack would not be effective. If you're okay with that, I'll change the |
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
LGTM, and thank you for the doc-additions, good stuff, much appreciated.
|
Great, thank you! |
|
Oh missed that, sure that’s not a bad idea. |
This was originally written by @vkomenda, then squashed for easier rebasing on master. Cleanup of `received_step_hashes` was moved to `verify_block_family`, since `on_prepare_block` does not exist on master, and a unit test was added. Original commit messages: added the map of received block header hashes do not return an error and remove older received block records optimised older record removal block hash comparison optimisation and the weak client ref fix SIBLING_MALICE_DETECTION_PERIOD constant review comments using step numbers instead of block numbers
Co-Authored-By: David <dvdplm@gmail.com>
afck
force-pushed
the
poanetwork-sibling-blocks
branch
from
8581b8c to
53c320c
Compare
|
Another runner system failure: |
|
Best merge before the beast wakes up. ;) |
ordian
added
A8-looksgood 🦄
* master: Type annotation for next_key() matching of json filter options (#11192) Crypto primitives removed from ethkey (#11174) Made ecrecover implementation trait public (#11188) Remove unused macro_use. (#11191) [dependencies]: jsonrpc `14.0.1` (#11183) [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179) [dependencies] bump rand 0.7 (#11022) [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) TxPermissions ver 3: gas price & data (#11170) [ethash] chainspec validate `ecip1017EraRounds` non-zero (#11123) util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) Aura: Report malice on sibling blocks from the same validator (#11160)
* master: Pause pruning while snapshotting (#11178) Type annotation for next_key() matching of json filter options (#11192) Crypto primitives removed from ethkey (#11174) Made ecrecover implementation trait public (#11188) Remove unused macro_use. (#11191) [dependencies]: jsonrpc `14.0.1` (#11183) [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179) [dependencies] bump rand 0.7 (#11022) [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) TxPermissions ver 3: gas price & data (#11170) [ethash] chainspec validate `ecip1017EraRounds` non-zero (#11123) util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) Aura: Report malice on sibling blocks from the same validator (#11160)
7 participants
Calls
report_maliciousif a validator creates multiple sibling blocks in the same step.This was originally written by @vkomenda (poanetwork#165), then squashed for easier rebasing on master. Cleanup of
received_step_hasheswas moved toverify_block_family, sinceon_prepare_blockdoes not exist on master, and a unit test was added.Original commit messages: