chore(code): Some WAL entries fail to decode #840

ancazamfir · 2025-02-07T21:19:25Z

Closes: #XXX
Sometime wal entries fail to decode with errors like this:

2025-02-07T20:30:44.054030Z ERROR node{moniker=test-2}:wal{height=144}: Failed to decode WAL entry 4: failed to fill whole buffer

Looks like for PrevoteTimeLimit and PrecommitTimeLimit we still write something to the WAL (just the tag) and later fail to decode.

Add test that shows the error
Fix the error

Note: A better fix might be to move the entry filtering logic in consensus who would decide which entries should be logged. For timeouts it could check the type here

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

codecov · 2025-02-07T21:40:00Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.73%. Comparing base (614a46c) to head (ea04fba).
Report is 2 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #840      +/-   ##
==========================================
- Coverage   75.79%   75.73%   -0.06%     
==========================================
  Files         169      172       +3     
  Lines       14682    14801     +119     
==========================================
+ Hits        11128    11209      +81     
- Misses       3554     3592      +38

Flag	Coverage Δ
integration	`75.44% <ø> (-0.06%)`	⬇️
mbt	`14.63% <ø> (-6.00%)`	⬇️

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

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

romac · 2025-02-10T11:36:04Z

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

-                // Write timeout
-                encode_timeout(timeout, &mut buf)?;
+                // Write tag and timeout if applicable
+                encode_timeout(Self::TAG_TIMEOUT, timeout, &mut buf)?;


Nice catch! Thanks so much for figuring this out 🙌

Wondering if we should have consensus decide which entries should be logged and have wal write everything it's told to (by consensus).

80afd66 + ea04fba

ancazamfir · 2025-02-10T11:48:43Z

code/crates/starknet/test/src/lib.rs

+                                break 'inner;
+                            }
+                        }
+                        Event::WalReplayError => {


Not sure this is the best place to catch errors (WAL or others) in tests.

Not sure either, but since we already detect errors in the background task, I moved it there.

* Add test that shows: failed to fill whole buffer * Add WAL replay error when unable to decode all entries * Don't add tag w/o value for skipped timers or empty buf to wal * Move new test to wal * Logging cleanup * Fix missing info in logs * Add error detail to WalReplayError event * Add tag to wal only if vote encoding is succesful * Do not instruct WAL to persist step timeouts * Move WalReplayError detection to background task * WAL writes all timeouts it is given, leave choice up to consensus --------- Co-authored-by: Romain Ruetschi <[email protected]>

* feat(test): Add test app based on the example app * Improve logging in tests * Better error handling * Formatting * Fix test suite * Implement `RestreamProposal` in example and test app * Disable `proposal_only` consensus mode tests * Start from latest height in store plus one * Do not run MBT tests in integration test suite * Fix missing logs * Store synced values in database for later retrieval, increase history length to 100 * Formatting * Run integration tests for test app in their own job * Run test app tests with `nextest` for now * Do not fail CI for test app integration test failure * Go back to running tests with maelstrom * Exclude test app from code coverage for now * Ignore unsupported tests * Add back full nodes tests * Wait a bit until starting the first height * Fix off-by-one error in starting height of test and example apps * Fix typo * Slow down test app to ease debugging * Update state.rs Co-authored-by: Anca Zamfir <[email protected]> Signed-off-by: Romain Ruetschi <[email protected]> * feat(code/app/starknet): Adapt Starknet app to latest P2P protos (#819) * Update Starknet protos to their latest version * Adapt Starknet app to new protos * Use height and round as stream id in example app * Use `ConsensusStreamId` proto message for stream id in Starknet app * Fix formatting of stream ids * Update protos for interop * Add `ProposalCommitment` type * Add `BlockInfo` type * Update proposal building and assembly from parts to account for BlockInfo and ProposalCommitment * Update protos * Adapt to latest proto changes * Post-merge fixes * Disable integration tests with proposal mode, keep parts only * Set parts-only in spwan.bash * Add detail to panic when explicit proposal * Fix fmt --------- Co-authored-by: Anca Zamfir <[email protected]> * chore(code): Some WAL entries fail to decode (#840) * Add test that shows: failed to fill whole buffer * Add WAL replay error when unable to decode all entries * Don't add tag w/o value for skipped timers or empty buf to wal * Move new test to wal * Logging cleanup * Fix missing info in logs * Add error detail to WalReplayError event * Add tag to wal only if vote encoding is succesful * Do not instruct WAL to persist step timeouts * Move WalReplayError detection to background task * WAL writes all timeouts it is given, leave choice up to consensus --------- Co-authored-by: Romain Ruetschi <[email protected]> * Post-merge fixes * Refactor parts signature verification * Add parts signature verification to test app * Restream proposal parts when reproposing a value * Formatting * Remove decided proposal from undecided proposals table * Remove duplicated code * Include test app tests in code coverage * Abort whole test on failure * Fix full node test * Show log when request sync value --------- Signed-off-by: Romain Ruetschi <[email protected]> Co-authored-by: Anca Zamfir <[email protected]> Co-authored-by: Anca Zamfir <[email protected]>

…ny app (#836) * feat(test): Add test app based on the example app * Improve logging in tests * Better error handling * Formatting * Fix test suite * Implement `RestreamProposal` in example and test app * Disable `proposal_only` consensus mode tests * Start from latest height in store plus one * Do not run MBT tests in integration test suite * Fix missing logs * Store synced values in database for later retrieval, increase history length to 100 * Formatting * Run integration tests for test app in their own job * Run test app tests with `nextest` for now * Do not fail CI for test app integration test failure * Generalize the test framework to make it work with any app * Fix unit test * Fix bug in config generation * Cleanup * Typo * Cleanup * Fix late starting full node test * Cleanup * Introduce `NodeHandle` abstraction * Cleanup * Wait until the main actor is dead before returning * Go back to running tests with maelstrom * Fix full node tests * Fix config generation in test app tests * Exclude test app from code coverage for now * Ignore unsupported tests * Add back full nodes tests * Remove to initiate logger externally * Wait a bit until starting the first height * Fix off-by-one error in starting height of test and example apps * Fix typo * Slow down test app to ease debugging * Cleanup * Update state.rs Co-authored-by: Anca Zamfir <[email protected]> Signed-off-by: Romain Ruetschi <[email protected]> * feat(code/app/starknet): Adapt Starknet app to latest P2P protos (#819) * Update Starknet protos to their latest version * Adapt Starknet app to new protos * Use height and round as stream id in example app * Use `ConsensusStreamId` proto message for stream id in Starknet app * Fix formatting of stream ids * Update protos for interop * Add `ProposalCommitment` type * Add `BlockInfo` type * Update proposal building and assembly from parts to account for BlockInfo and ProposalCommitment * Update protos * Adapt to latest proto changes * Post-merge fixes * Disable integration tests with proposal mode, keep parts only * Set parts-only in spwan.bash * Add detail to panic when explicit proposal * Fix fmt --------- Co-authored-by: Anca Zamfir <[email protected]> * chore(code): Some WAL entries fail to decode (#840) * Add test that shows: failed to fill whole buffer * Add WAL replay error when unable to decode all entries * Don't add tag w/o value for skipped timers or empty buf to wal * Move new test to wal * Logging cleanup * Fix missing info in logs * Add error detail to WalReplayError event * Add tag to wal only if vote encoding is succesful * Do not instruct WAL to persist step timeouts * Move WalReplayError detection to background task * WAL writes all timeouts it is given, leave choice up to consensus --------- Co-authored-by: Romain Ruetschi <[email protected]> * Post-merge fixes * Refactor parts signature verification * Add parts signature verification to test app * Restream proposal parts when reproposing a value * Formatting * Remove decided proposal from undecided proposals table * Remove duplicated code * Include test app tests in code coverage * Abort whole test on failure * Fix full node test * Show log when request sync value * Make sure Starknet app does not go into round 1 at height 1 * Fix WAL tests for test app * Fix off by one error in logs due to potential race condition in test framework * Clarify that `Default` impl of Height must return the zero height * Require both Starknet and test app tests to pass on CI * Update coverage exclude list * Post-merge fixes --------- Signed-off-by: Romain Ruetschi <[email protected]> Co-authored-by: Anca Zamfir <[email protected]> Co-authored-by: Anca Zamfir <[email protected]>

Add test that shows: failed to fill whole buffer

aebabcb

ancazamfir added 2 commits February 10, 2025 12:13

Add WAL replay error when unable to decode all entries

b4d2a45

Don't add tag w/o value for skipped timers or empty buf to wal

9380e0e

romac reviewed Feb 10, 2025

View reviewed changes

Move new test to wal

aef2879

ancazamfir commented Feb 10, 2025

View reviewed changes

ancazamfir added 4 commits February 10, 2025 13:08

Logging cleanup

bdc9747

Fix missing info in logs

78ba01e

Add error detail to WalReplayError event

6eadd52

Add tag to wal only if vote encoding is succesful

0c4e58e

ancazamfir marked this pull request as ready for review February 10, 2025 13:31

romac added 3 commits February 10, 2025 15:08

Do not instruct WAL to persist step timeouts

80afd66

Move WalReplayError detection to background task

a1508ef

WAL writes all timeouts it is given, leave choice up to consensus

ea04fba

romac approved these changes Feb 10, 2025

View reviewed changes

ancazamfir added this pull request to the merge queue Feb 10, 2025

Merged via the queue into main with commit eaf84cc Feb 10, 2025
22 checks passed

ancazamfir deleted the anca/wal_decode branch February 10, 2025 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(code): Some WAL entries fail to decode #840

chore(code): Some WAL entries fail to decode #840

ancazamfir commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 7, 2025 •

edited

Loading

romac Feb 10, 2025

ancazamfir Feb 10, 2025 •

edited

Loading

romac Feb 10, 2025

ancazamfir Feb 10, 2025

romac Feb 10, 2025

chore(code): Some WAL entries fail to decode #840

chore(code): Some WAL entries fail to decode #840

Conversation

ancazamfir commented Feb 7, 2025 • edited Loading

PR author checklist

For all contributors

For external contributors

codecov bot commented Feb 7, 2025 • edited Loading

Codecov Report

romac Feb 10, 2025

Choose a reason for hiding this comment

ancazamfir Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

romac Feb 10, 2025

Choose a reason for hiding this comment

ancazamfir Feb 10, 2025

Choose a reason for hiding this comment

romac Feb 10, 2025

Choose a reason for hiding this comment

ancazamfir commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 7, 2025 •

edited

Loading

ancazamfir Feb 10, 2025 •

edited

Loading