Skip to content
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

Buffer votes for blocks we don't know about #718

Merged
merged 2 commits into from
Apr 16, 2024
Merged

Conversation

JamesHinshelwood
Copy link
Contributor

@JamesHinshelwood JamesHinshelwood commented Feb 13, 2024

Votes can arrive for blocks we aren't yet aware of in (at least) two circumstances:

  • If we disconnect and reconnect, we will download the blocks we missed, but nodes might send us their votes for the next block before we've received the missed blocks.
  • Due to network latency, a vote for a block proposal could arrive before the block proposal itself.

Before this commit, the node ignores these votes and the network eventually recovers via a new view. However, this slows things down so we should recover faster if possible. Instead, we store votes for unknown blocks in memory and replay them if the block later becomes known to us.

The implementation is fairly straight forward but there are a few caveats and TODOs:

  • The return type of Consensus::proposal is now more complicated, as it doesn't just return a Message::Vote any more. If the proposal results in some buffered votes being replayed and those votes form a supermajority, then the node can immediately propose the next block.
  • There's nothing which limits the memory usage of buffered votes. A malicious node is perfectly able to send us loads of votes with non-existant block hashes, which we will store forever. I've raised Limit the amount of memory used by buffered votes #719 to resolve this.
  • When applying buffered votes as a result of a proposal, they take priority over our own vote. This means we lose out on the cosigner reward. Prioritise our own vote before buffered votes #720
  • The unreliability test could be improved to be more efficient and to assert a stricter condition on the network - Improve unreliable::blocks_are_produced_while_a_node_restarts test #721.

@JamesHinshelwood JamesHinshelwood marked this pull request as ready for review February 13, 2024 15:52
@JamesHinshelwood JamesHinshelwood marked this pull request as draft February 13, 2024 15:52
@JamesHinshelwood JamesHinshelwood changed the base branch from sync-off-by-one to main February 13, 2024 15:52
@JamesHinshelwood JamesHinshelwood force-pushed the buffer-votes branch 2 times, most recently from 52bca93 to 29c17e1 Compare February 13, 2024 16:09
zilliqa/src/consensus.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@theo-zil theo-zil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly one question about messages sent after processing blocks during sync, and a couple of non-blocking minor nits/questions, otherwise all looks good

zilliqa/src/consensus.rs Show resolved Hide resolved
zilliqa/src/consensus.rs Outdated Show resolved Hide resolved
zilliqa/tests/it/main.rs Show resolved Hide resolved
zilliqa/tests/it/unreliable.rs Show resolved Hide resolved
zilliqa/src/consensus.rs Outdated Show resolved Hide resolved
Votes can arrive for blocks we aren't yet aware of in (at least)
two circumstances:
* If we disconnect and reconnect, we will download the blocks we
missed, but nodes might send us their votes for the next block
before we've received the missed blocks.
* Due to network latency, a vote for a block proposal could arrive
before the block proposal itself.

Before this commit, the node ignores these votes and the network
eventually recovers via a new view. However, this slows things
down so we should recover faster if possible. Instead, we store
votes for unknown blocks in memory and replay them if the block
later becomes known to us.

The implementation is fairly straight forward but there are a few
caveats and TODOs:
* The return type of `Consensus::proposal` is now more
complicated, as it doesn't just return a `Message::Vote` any
more. If the proposal results in some buffered votes being
replayed and those votes form a supermajority, then the node can
immediately propose the next block.
* There's nothing which limits the memory usage of buffered votes.
A malicious node is perfectly able to send us loads of votes with
non-existant block hashes, which we will store forever. I've
raised #719 to resolve this.
* When applying buffered votes as a result of a proposal, they
take priority over our own vote. This means we lose out on the
cosigner reward. #720
* The unreliability test could be improved to be more efficient
and to assert a stricter condition on the network - #721.
@JamesHinshelwood JamesHinshelwood added this pull request to the merge queue Apr 16, 2024
Merged via the queue into main with commit 8701fad Apr 16, 2024
4 checks passed
@JamesHinshelwood JamesHinshelwood deleted the buffer-votes branch April 16, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants