[Consensus] verify block timestamp invariant and genesis ancestors #16439
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
mwtian
force-pushed
the
consensus-verify
branch
from
e6c9655 to
97e3478
Compare
mwtian
changed the title
mwtian
force-pushed
the
consensus-verify
branch
from
97e3478 to
631ee32
Compare
mwtian
force-pushed
the
consensus-verify
branch
from
631ee32 to
64c2c21
Compare
mwtian
changed the title
mwtian
force-pushed
the
consensus-verify
branch
from
75e3a85 to
474f02f
Compare
| @@ -98,7 +152,7 @@ impl BlockManager { | |||
| /// Tries to accept the provided block. To accept a block its ancestors must have been already successfully accepted. If | |||
| /// block is accepted then Some result is returned. None is returned when either the block is suspended or the block | |||
| /// has been already accepted before. | |||
| fn try_accept_block(&mut self, block: VerifiedBlock) -> Option<VerifiedBlock> { | |||
| fn try_accept_one_block(&mut self, block: VerifiedBlock) -> Option<VerifiedBlock> { | |||
There was a problem hiding this comment.
Why the name change? try_accept_block already reads as it will accept one block.
There was a problem hiding this comment.
I want to make it clearer that children blocks will not be unsuspended in try_accept_block(), unlike try_accept_blocks(). I can definitely revert if this is not helping.
|
|
||
| // Accept the state in DAG here so the next block to be processed can find in DAG/store any accepted blocks. | ||
| // TODO: report blocks_to_reject to peers. |
There was a problem hiding this comment.
Is the purpose of reporting these blocks to peers as a method for reporting suspected byzantine behavior. The peers would have to revalidate these blocks anyways right?
There was a problem hiding this comment.
Yes, and these are not just suspected but verifiable byzantine behaviors, because of block signature. Only the block without a rejected ancestor can be reported though.
| if !committee.is_valid_index(ancestor.author) { | ||
| return Err(ConsensusError::InvalidAuthorityIndex { | ||
| index: ancestor.author, | ||
| max: committee.size() - 1, | ||
| }); | ||
| } | ||
| if ancestor.author == block.author() { | ||
| own_ancestor = true; | ||
| if (i == 0 && ancestor.author != block.author()) |
There was a problem hiding this comment.
ooc why is it important that the block.author has its ancestor as the first ancestor in the list?
There was a problem hiding this comment.
Each block is required to include the latest own ancestor. It seems good to have a convention on where it is placed. In future we may run more validations on the consistency between a block and its own ancestor. Instead of a separate field, own ancestor is stored at the 1st element of the ancestors Vec.
|
Merging. Will address any comment via followup PRs. |
@mwtian apologies for not having a look on this yet, I'll leave any comments in the weekend so you can address in follow up PRs as you said |
…ystenLabs#16439) ## Description Verify that max ancestors timestamp <= block timestamp. Since this check requires all ancestors block to be available, it needs to be done after a block has complete causal history but before the block is accepted. Also, - Verify genesis ancestors are valid. - Refactor genesis block creation. ## Test Plan Unit test. --- If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section. ### Type of Change (Check all that apply) - [ ] protocol change - [ ] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes
Description
Verify that max ancestors timestamp <= block timestamp.
Since this check requires all ancestors block to be available, it needs to be done after a block has complete causal history but before the block is accepted.
Also,
Test Plan
Unit test.
If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.
Type of Change (Check all that apply)
Release notes