feat(code): Add polka certificate to VoteSync response #859
Conversation
| ) | ||
| .await?; | ||
| } else { | ||
| for vote in vote_set.votes { |
There was a problem hiding this comment.
@ancazamfir I don't believe we need to process both the votes and the certificate, but perhaps I am mistaken?
| .cloned() | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| if votes.is_empty() { |
There was a problem hiding this comment.
I wonder if this is wrong, in case we have a polka certificate for a previous round?
| self.polka_certificates | ||
| .iter() | ||
| .filter(|c| c.round <= round) | ||
| .last() |
There was a problem hiding this comment.
Here I did something similar as we do for commit certificates but it feels very hacky and I am not convinced it is right. Will check the Quint spec again to see whether or not this is correct.
| /// The value that the Polka is for | ||
| pub value_id: ValueId<Ctx>, | ||
| /// The votes that make up the Polka | ||
| pub votes: Vec<SignedVote<Ctx>>, |
There was a problem hiding this comment.
Right now I am just storing the votes like this, we can switch to an aggregated signature at a later stage if needed.
❌ 10 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
romac
changed the title
|
Ignore the Rust / Standalone CI failure, it is expected because of the debug statements I added in the driver. |
|
Looks like I broke the Starknet tests, which is unexpected. |
romac
force-pushed
the
romac/polka-certificate
branch
from
f6b9dae to
80556c4
Compare
…preceding one fails
| if round_input == RoundInput::NoInput { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| self.apply_input(vote_round, round_input) | ||
| } | ||
|
|
||
| fn store_polka_certificate(&mut self, vote_round: Round, value_id: &ValueId<Ctx>) { |
There was a problem hiding this comment.
Ref:
There was a problem hiding this comment.
TODO: Check that the certificate we have constructed is valid.
There was a problem hiding this comment.
TODO: Rename this method to get_polka_certificate
There was a problem hiding this comment.
TODO: Move this method to the VoteKeeper and only store the certificate in polka_certificates here
| @@ -216,6 +216,41 @@ where | |||
| None | |||
| } | |||
|
|
|||
| pub(crate) fn store_and_multiplex_polka_certificate( | |||
There was a problem hiding this comment.
Ref:
There was a problem hiding this comment.
Looking at the spec again, I believe this method is not correct. Instead of sending a ProposalAndPolkaCurrent to the state machine, we should instead follow the same logic as the spec and override the votes in the vote keeper.
We should add the apply_certificate method to the VoteKeeper struct and implement that logic there.
|
Note: The two relevant integration tests for this PR are:
They can be run with: (Requires |
|
Hey @romac, I think it will be very useful if we add a description to this PR. As from what I understood it is not a bug fix but rather a change that should not only unstuck consensus in case of a Byzantine behavior observed 2nd scenario in #850 but also lead to the consensus decision. I can maybe help with writing this description but I need more context, for example where can I learn how vote sync works and when it is triggered? |
We do not have a spec for this yet, but the tracking issue for it has some background info: #576 Let me know if that helps, otherwise I can provide more details. |
No description provided.