feat(code): Add polka certificates to VoteSync response #859

romac · 2025-02-18T18:21:28Z

No description provided.

code/crates/core-consensus/src/handle/vote_set.rs

code/crates/core-consensus/src/state.rs

code/crates/core-driver/src/driver.rs

romac · 2025-02-18T18:32:39Z

code/crates/core-types/src/certificate.rs

+    /// The value that the Polka is for
+    pub value_id: ValueId<Ctx>,
+    /// The votes that make up the Polka
+    pub votes: Vec<SignedVote<Ctx>>,


Right now I am just storing the votes like this, we can switch to an aggregated signature at a later stage if needed.

codecov · 2025-02-18T18:59:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.17%. Comparing base (65757e4) to head (ea61aa6).

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@                       Coverage Diff                        @@
##           anca/temp_byzantine_proposer     #859      +/-   ##
================================================================
- Coverage                         83.06%   82.17%   -0.89%     
================================================================
  Files                               188      191       +3     
  Lines                             16219    16601     +382     
================================================================
+ Hits                              13471    13641     +170     
- Misses                             2748     2960     +212

Flag	Coverage Δ
integration	`81.99% <ø> (-0.91%)`	⬇️
mbt	`8.08% <ø> (-0.32%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
core	`∅ <ø> (∅)`
engine	`∅ <ø> (∅)`
app	`∅ <ø> (∅)`
starknet	`∅ <ø> (∅)`

romac · 2025-02-19T13:49:34Z

Ignore the Rust / Standalone CI failure, it is expected because of the debug statements I added in the driver.

romac · 2025-02-19T13:50:25Z

Looks like I broke the Starknet tests, which is unexpected.

…preceding one fails

romac · 2025-02-26T13:24:20Z

code/crates/core-driver/src/driver.rs

        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>) {


Ref:

malachite/specs/consensus/quint/votekeeper.qnt

Line 541 in ad7350e

pure def getCertificate(keeper: Bookkeeper, vkout: VoteKeeperOutput) : Certificate =

TODO: Check that the certificate we have constructed is valid.

TODO: Rename this method to get_polka_certificate

TODO: Move this method to the VoteKeeper and only store the certificate in polka_certificates here

romac · 2025-02-26T13:25:25Z

code/crates/core-driver/src/mux.rs

@@ -216,6 +216,41 @@ where
        None
    }

+    pub(crate) fn store_and_multiplex_polka_certificate(


Ref:

malachite/specs/consensus/quint/votekeeper.qnt

Line 445 in ad7350e

pure def applyCertificate(keeper: Bookkeeper, certificate: Set[Vote], currentRound: Round): { bookkeeper: Bookkeeper, output: VoteKeeperOutput } =

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.

We can do that. But we can also store/ retrieve the certificate as you do here. In this case we also have to change multiplex_proposal() and how we determine polka_current and polka_previous.

romac · 2025-02-26T13:33:57Z

Note: The two relevant integration tests for this PR are:

wal::restart_with_byzantine_proposer_1_request_response
wal::restart_with_byzantine_proposer_2_request_response

They can be run with:

$ cargo nextest run -p informalsystems-malachitebft-test --no-capture wal::restart_with_byzantine_proposer_1_request_response --retries 0
$ cargo nextest run -p informalsystems-malachitebft-test --no-capture wal::restart_with_byzantine_proposer_2_request_response --retries 0

(Requires cargo-nextest, can be installed with cargo install cargo-nextest)

nenadmilosevic95 · 2025-02-26T18:13:39Z

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?

romac · 2025-02-26T18:51:40Z

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.

cason · 2025-02-28T10:31:31Z

Recall that we might need a Polka certificate for a previous round (vr < round_p) in case of line 28.

Bumps [prost](https://github.com/tokio-rs/prost) from 0.13.4 to 0.13.5. - [Release notes](https://github.com/tokio-rs/prost/releases) - [Changelog](https://github.com/tokio-rs/prost/blob/master/CHANGELOG.md) - [Commits](tokio-rs/prost@v0.13.4...v0.13.5) --- updated-dependencies: - dependency-name: prost dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* chore: Add detailed contributing informations * Code review * Fix

…#872) * feat(code): Applications now define their own configuration * Cleanup * Ignore unuspported tests * Fix unit tests * Fix init and testnet commands * Post-merge fixes * Override VoteSync mode from test params for Starknet test app * Update VoteSync tests * Re-instate `distributed-testnet` command for Starknet app * Embed discovery config directly in `MakeConfigSettings` * Cleanup

…e capacity (#878) * fix(code/engine): Use custom version of `OutputPort` with configurable capacity * Disallow use of ractor's output port

romac · 2025-03-05T16:32:22Z

The wal::restart_with_byzantine_proposer_1_request_response test has passed on CI but it does not pass reliably locally. Need to investigate further.

@ancazamfir Can you give it a go when you have time and see if you see the same?

$ cargo nextest run -p informalsystems-malachitebft-test --no-capture --retries 0 wal::restart_with_byzantine_proposer_1_request_response

romac · 2025-03-05T16:35:51Z

It's also surprising that the wal::restart_with_byzantine_proposer_2_rebroadcast test has passed on CI, where I would have expected that we'd need to include the polka certificates in the rebroadcast for it to pass.

ancazamfir · 2025-03-05T20:45:57Z

The wal::restart_with_byzantine_proposer_1_request_response test has passed on CI but it does not pass reliably locally. Need to investigate further.

@ancazamfir Can you give it a go when you have time and see if you see the same?
$ cargo nextest run -p informalsystems-malachitebft-test --no-capture --retries 0 wal::restart_with_byzantine_proposer_1_request_response

It passes for me, it was passing even when CI was failing it. I tried today again 10 times and was about to ask you to try it :)

ancazamfir · 2025-03-05T21:17:49Z

It's also surprising that the wal::restart_with_byzantine_proposer_2_rebroadcast test has passed on CI, where I would have expected that we'd need to include the polka certificates in the rebroadcast for it to pass.

The test is not there yet. There are a few things:

since we fixed the vote equivocation we cannot generate the scenario, we will need the byzantine node to be configured to equivocate on votes in addition to proposals
will also need code changes to store equivocating proposals in the driver (currently we only store them before in the full_proposal module)

code/crates/test/tests/it/wal.rs

romac added the work in progress Work in progress label Feb 18, 2025

romac commented Feb 18, 2025

View reviewed changes

romac mentioned this pull request Feb 19, 2025

code: Vote Sync improvements #857

Open

4 tasks

romac changed the title ~~code: Add polka certificate to VoteSync response~~ feat(code): Add polka certificate to VoteSync response Feb 19, 2025

wip: Add PolkaCertificate

80556c4

romac force-pushed the romac/polka-certificate branch from f6b9dae to 80556c4 Compare February 19, 2025 16:39

Ensure all three types of integration tests are always run even if a …

27c1bc5

…preceding one fails

This was referenced Feb 22, 2025

Consensus stuck in votesync upon restart #850

Open

fix(code/core-consensus): Prevent nodes from broadcasting equivocating votes #864

Merged

romac commented Feb 26, 2025

View reviewed changes

ancazamfir and others added 13 commits March 3, 2025 16:36

Merge branch 'anca/temp_byzantine_proposer' into romac/polka-certificate

1ab7004

Fix compilation error

a2cc60c

Clippy

5b03d82

Merge branch 'anca/temp_byzantine_proposer' into romac/polka-certificate

0de2efa

Merge branch 'anca/temp_byzantine_proposer' into romac/polka-certificate

45fd9f6

chore: Add detailed contributing informations (#875)

616c414

* chore: Add detailed contributing informations * Code review * Fix

Merge branch 'anca/temp_byzantine_proposer' into romac/polka-certificate

e2416c8

fix(code/engine): Use custom version of OutputPort with configurabl…

201eaf8

…e capacity (#878) * fix(code/engine): Use custom version of `OutputPort` with configurable capacity * Disallow use of ractor's output port

Merge branch 'main' into romac/polka-certificate

1891a41

Get polka certificate for previous rounds

77f4c35

Get polka certificates for all rounds up to round included

238ee56

romac added 2 commits March 5, 2025 17:12

Get votes for round and all later rounds

d6e2566

Test app only supports parts-only mode

cecb3e3

romac changed the title ~~feat(code): Add polka certificate to VoteSync response~~ feat(code): Add polka certificates to VoteSync response Mar 5, 2025

Better output when running multiple tests

7e6c49d

ancazamfir reviewed Mar 5, 2025

View reviewed changes

code/crates/test/tests/it/wal.rs Outdated Show resolved Hide resolved

romac added 2 commits March 6, 2025 11:14

Remove restriction on parts-only mode

399a7be

Remove debug statements

ea61aa6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(code): Add polka certificates to VoteSync response #859

feat(code): Add polka certificates to VoteSync response #859

romac commented Feb 18, 2025

romac Feb 18, 2025

codecov bot commented Feb 18, 2025 •

edited

Loading

romac commented Feb 19, 2025

romac commented Feb 19, 2025

romac Feb 26, 2025

romac Feb 26, 2025

romac Feb 26, 2025 •

edited

Loading

romac Feb 26, 2025

romac Feb 26, 2025

romac Feb 26, 2025

ancazamfir Mar 3, 2025

romac commented Feb 26, 2025 •

edited

Loading

nenadmilosevic95 commented Feb 26, 2025

romac commented Feb 26, 2025 •

edited

Loading

cason commented Feb 28, 2025

romac commented Mar 5, 2025 •

edited

Loading

romac commented Mar 5, 2025 •

edited

Loading

ancazamfir commented Mar 5, 2025

ancazamfir commented Mar 5, 2025

feat(code): Add polka certificates to VoteSync response #859

Are you sure you want to change the base?

feat(code): Add polka certificates to VoteSync response #859

Conversation

romac commented Feb 18, 2025

romac Feb 18, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 18, 2025 • edited Loading

Codecov Report

romac commented Feb 19, 2025

romac commented Feb 19, 2025

romac Feb 26, 2025

Choose a reason for hiding this comment

romac Feb 26, 2025

Choose a reason for hiding this comment

romac Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

romac Feb 26, 2025

Choose a reason for hiding this comment

romac Feb 26, 2025

Choose a reason for hiding this comment

romac Feb 26, 2025

Choose a reason for hiding this comment

ancazamfir Mar 3, 2025

Choose a reason for hiding this comment

romac commented Feb 26, 2025 • edited Loading

nenadmilosevic95 commented Feb 26, 2025

romac commented Feb 26, 2025 • edited Loading

cason commented Feb 28, 2025

romac commented Mar 5, 2025 • edited Loading

romac commented Mar 5, 2025 • edited Loading

ancazamfir commented Mar 5, 2025

ancazamfir commented Mar 5, 2025

codecov bot commented Feb 18, 2025 •

edited

Loading

romac Feb 26, 2025 •

edited

Loading

romac commented Feb 26, 2025 •

edited

Loading

romac commented Feb 26, 2025 •

edited

Loading

romac commented Mar 5, 2025 •

edited

Loading

romac commented Mar 5, 2025 •

edited

Loading