feat(code/engine): Store proposed values in WAL #896

romac · 2025-03-07T16:05:06Z

Closes: #897

PR author checklist

For all contributors

Reference GitHub issue
Ensure PR title follows the conventional commits spec

For external contributors

Maintainers of Malachite are allowed to push changes to the branch

…if available

codecov · 2025-03-07T16:26:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.00%. Comparing base (f77bdb3) to head (f631c01).
Report is 3 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@     Coverage Diff     @@
##   main   #896   +/-   ##
===========================
===========================

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

…osals are built (#945) * Make `new_prevote`, `new_precommit` and `new_proposal` methods of `Context` instead of static functions * Stuff middleware in the `TestContext` instead of static variable * Remove unused dep * Add way to access middleware from TestContext

…`Propose` input

… by us instead of performing `GetValue` effect

code/crates/engine/src/util/events.rs

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

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

ancazamfir · 2025-04-04T07:08:27Z

Sorry for the stale review, forgot to publish couple of days ago. Will close the ones resolved.

logged.

full proposal.

nenadmilosevic95

I made a full pass! It looks good to me! Great job @romac ! I left some comments!

As a side not, I am more convinced that different modes parts-only and others should not be part of consensus-core, and that we need to rethink these designs, if for nothing but just for code clarity. IMHO, these modes are complicating the code.

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

code/crates/engine/src/wal/entry.rs

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

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

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

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

code/crates/engine/src/consensus.rs

…efore starting round 0

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

code/crates/engine/src/wal/entry.rs

code/crates/engine/src/consensus.rs

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

code/crates/engine/src/consensus.rs

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

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

code/crates/engine/src/wal/entry.rs

Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

ancazamfir · 2025-04-07T16:05:58Z

As a side not, I am more convinced that different modes parts-only and others should not be part of consensus-core, and that we need to rethink these designs, if for nothing but just for code clarity. IMHO, these modes are complicating the code.

We can look into it but here is a brief overview and history:

we have started with proposal-and-parts mode, it was not configurable as it was the only one. To handle it we added the full_proposal.rs where we mux the ProposedValue from the app and the Proposal wire message. When we have both we call the driver and then the state machine
then starknet model came with parts-only and, from conversations with farcaster, it transpired that a proposal-only mode would have been useful (had it been ready). So we added these two modes
it is the application that configures consensus with the mode that best fits, we can make recommendations
the full_proposer in core-consensus, core-driver, core_votekeeper and core-state-machine are and will stay mode agnostic
the mode is handled in the core-consensus/src/handle/.. event or message handlers and there are a couple of early checks for misconfiguration in the engine

We have draft PRs: #942 and (not directly related) #943 along your comments (and some raised during the review of ADR003). Mainly trying to move the checks in the engine and get rid of proposal parts. But we do have some concerns and we can discuss in the future.

nenadmilosevic95

Made another pass! LGTM! Great job!

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

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

code/crates/engine/src/wal/entry.rs

Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Romain Ruetschi <github@romac.me>

* feat(code/engine): Store locally proposed value in WAL and re-use it if available * Cleanup * Merge all three WalAppend effects into one * Cleanup * Add multi-round test with timeouts * Cleanup * Add a middleware to the test context to override vote building * Less noisy logs in test app * Add concept of middleware to change from the tests how votes and proposals are built (#945) * Make `new_prevote`, `new_precommit` and `new_proposal` methods of `Context` instead of static functions * Stuff middleware in the `TestContext` instead of static variable * Remove unused dep * Add way to access middleware from TestContext * Deduplicate timeout elapsed code * Replay proposed values before other entries * Split `StartHeight` into `PrepareHeight` and `StartHeight` * Add check that we are in the right state before starting a height * Actually replay the proposed values * Cleanup * Store proposed values for other rounds * Better naming * Replay locally proposed value as an `ProposedValue` input instead of `Propose` input * Fix unused variable warning * Replay proposals * On GetValue output from the driver, re-use any full proposal proposed by us instead of performing `GetValue` effect * Remove outdated check in Starknet tests * Cleanup * Simplify WAL replay * Store all proposed values (local or not) in the WAL * Replay the whole WAL in one go after `StartHeight` * The application will get a `GetValue` effect during WAL replay, but its response will be ignored if we already had a proposed value in the WAL. * Re-enable checks in byzantine WAL tests * Cleanup * Cleanup protos * Replay ProposedValue exactly as stored in the WAL * Log proposed values to WAL only if not already seen and therefore logged. * Fix issue with last commit, store proposed values before getting the full proposal. * Fix comments * Ensure all timeouts can be decoded * Re-add check that was previously removed when we were replaying WAL before starting round 0 * Merge `WalReplay` events into one * Review comments * Apply suggestions from code review Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com> * Add comments to handler functions * Add comments to on_propose handler * Add encode and decode functions for all wal entries * Apply suggestion from code review Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Romain Ruetschi <github@romac.me> --------- Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com> Signed-off-by: Romain Ruetschi <github@romac.me> Co-authored-by: Anca Zamfir <zamfiranca@gmail.com> Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com> Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com>

romac added 4 commits March 7, 2025 13:24

feat(code/engine): Store locally proposed value in WAL and re-use it …

4da7504

…if available

Cleanup

4065d06

Merge all three WalAppend effects into one

6b70046

Cleanup

b041f19

romac added 9 commits March 27, 2025 15:21

Merge branch 'main' into romac/wal-proposed-value

bf848d0

Add multi-round test with timeouts

9dc0042

Cleanup

4f201b2

Add a middleware to the test context to override vote building

580a450

Less noisy logs in test app

d0eb574

Merge remote-tracking branch 'origin/main' into romac/wal-proposed-value

8e6dd5c

Merge remote-tracking branch 'origin/main' into romac/wal-proposed-value

fb33d28

Deduplicate timeout elapsed code

1fe7b38

romac mentioned this pull request Mar 28, 2025

chore(test): Replay WAL before start round #898

Closed

3 tasks

romac added 8 commits March 28, 2025 15:04

Replay proposed values before other entries

a9f73f0

Split StartHeight into PrepareHeight and StartHeight

091c277

Add check that we are in the right state before starting a height

cf49f0e

Actually replay the proposed values

fd34681

Cleanup

2649113

Store proposed values for other rounds

f2462b0

Better naming

254b9c5

Replay locally proposed value as an ProposedValue input instead of …

e0ccb97

…`Propose` input

romac mentioned this pull request Mar 31, 2025

code: Tests for multi-rounds unstarted WAL replay #946

Closed

3 tasks

Fix unused variable warning

434cc37

romac mentioned this pull request Mar 31, 2025

feat(code/test): Middleware for testing #948

Merged

3 tasks

romac added 4 commits April 1, 2025 17:22

Merge branch 'main' into romac/wal-proposed-value

0267c25

Replay proposals

b142a49

On GetValue output from the driver, re-use any full proposal proposed…

8416236

… by us instead of performing `GetValue` effect

Remove outdated check in Starknet tests

62019c9

Replay ProposedValue exactly as stored in the WAL

d426a5f

ancazamfir reviewed Apr 4, 2025

View reviewed changes

code/crates/engine/src/util/events.rs Outdated Show resolved Hide resolved

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

code/crates/core-consensus/src/handle/proposed_value.rs Outdated Show resolved Hide resolved

ancazamfir added 2 commits April 4, 2025 09:53

Log proposed values to WAL only if not already seen and therefore

cb696f6

logged.

Fix issue with last commit, store proposed values before getting the

1868db0

full proposal.

nenadmilosevic95 reviewed Apr 7, 2025

View reviewed changes

romac added 3 commits April 7, 2025 11:05

Fix comments

66e7ca5

Ensure all timeouts can be decoded

25c37a6

Re-add check that was previously removed when we were replaying WAL b…

130008a

…efore starting round 0

romac commented Apr 7, 2025

View reviewed changes

romac and others added 7 commits April 7, 2025 12:23

Merge WalReplay events into one

8f9679f

Review comments

f3e06a9

Apply suggestions from code review

f92b07f

Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

Apply suggestions from code review

adecaf7

Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

Add comments to handler functions

9d7ce77

Add comments to on_propose handler

6035541

Add encode and decode functions for all wal entries

a960ae4

ancazamfir mentioned this pull request Apr 7, 2025

code: Improvements to WAL, consensus modes and driver #962

Closed

4 tasks

Merge branch 'main' into romac/wal-proposed-value

31a926f

romac requested a review from ancazamfir April 8, 2025 07:12

nenadmilosevic95 reviewed Apr 8, 2025

View reviewed changes

code/crates/core-consensus/src/handle/proposal.rs Outdated Show resolved Hide resolved

code/crates/core-consensus/src/handle/propose.rs Show resolved Hide resolved

code/crates/engine/src/wal/entry.rs Show resolved Hide resolved

Apply suggestion from code review

f631c01

Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Romain Ruetschi <github@romac.me>

ancazamfir approved these changes Apr 8, 2025

View reviewed changes

romac added this pull request to the merge queue Apr 8, 2025

Merged via the queue into main with commit f02d3f6 Apr 8, 2025
22 checks passed

romac deleted the romac/wal-proposed-value branch April 8, 2025 08:01

ancazamfir mentioned this pull request Apr 8, 2025

code: Proposer should always produce a Proposal output #935

Open

feat(code/engine): Store proposed values in WAL #896

feat(code/engine): Store proposed values in WAL #896

Uh oh!

Conversation

romac commented Mar 7, 2025 • edited by ancazamfir Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR author checklist

For all contributors

For external contributors

Uh oh!

codecov bot commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ancazamfir commented Apr 4, 2025

Uh oh!

nenadmilosevic95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ancazamfir commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nenadmilosevic95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

romac commented Mar 7, 2025 •

edited by ancazamfir

Loading

codecov bot commented Mar 7, 2025 •

edited

Loading

ancazamfir commented Apr 7, 2025 •

edited

Loading