fix(code/sync): Buffer full sync response for higher heights, value and certificate#1149
Merged
fix(code/sync): Buffer full sync response for higher heights, value and certificate#1149
Conversation
7 tasks
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1149 +/- ##
==========================================
+ Coverage 75.12% 75.13% +0.01%
==========================================
Files 170 170
Lines 18133 18166 +33
Branches 18133 18166 +33
==========================================
+ Hits 13622 13648 +26
- Misses 3579 3584 +5
- Partials 932 934 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
romac
reviewed
Jul 30, 2025
Contributor
|
Looks great! Let's also add an entry in RELEASE_NOTES.md and one in BREAKING_CHANGES for the renamed input and the new effect. |
ancazamfir
requested review from
nenadmilosevic95 and
romac
and removed request for
adizere
hvanz
added a commit
that referenced
this pull request
Jul 31, 2025
… from InvalidValue and ValueProcessingError Required to simplify merging #1149: fix(code/sync): Buffer full sync response for higher heights, value and certificate
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Aug 11, 2025
* feat(sync/proto): Extend value request/response proto messages to support a range of values, while staying backwards compatible - Updated `ValueRequest` and `ValueResponse` to handle ranges of heights instead of single heights. - Modified sync request and response encoding/decoding to accommodate new structure. - Adjusted related processing logic to iterate over values based on the specified range. * feat(sync/network): Add v2 sync protocol - Extend PeerConnected event to include the peer's list of supported StreamProtocols - Update sync state `peers` to keep track of the protocol version supported by each connected peer * feat(sync/config): Add batch_size field to value_sync config * feat(sync)!: Update app interface to support retrieving a batch of decided values instead of one - Renamed `GetDecidedValue` to `GetDecidedValues`, and `GotDecidedValue` to `GotDecidedValues` in `HostMsg`, `sync::Effect`, `sync::Msg` and `sync::Input`. * fix(sync): Update borsh serialization for ValueRequest and ValueResponse to handle ranges * feat(sync): Implement value batching logic - sync state: Added `PendingRequests` struct for managing pending requests and their state. - Updated `SyncRequestTimedOut` to include `request_id`. * refactor(sync): use `is_none_or` when filtering peers * fix(config): Add batch_size option to config.toml * fix: updating sync_height when on_started_height * refactor(sync): Add request_id to `InvalidValue` and `ValueProcessingError` messages This is needed to remove height_states in PendingRequests, and allow the changes in the following commit. * refactor(sync): Simplify pending request management * fix: the reset_height test was failing because syncing was not triggered on a ResetHeight event * feat(tests): Add test for a node starting late with parallel requests and batching * chore: Update BREAKING_CHANGES.md * fix: add missing fields to TestParams * feat(test): add test to starknet * refactor: improve log message * fix: picking v1 peers by filtering per minimum height * fix: filtering peers + add unit tests - Filtering on a range not provided by the peers was returning a non-empty list. * feat(tests): update parameters for late start tests with batching; fix test params to include parallel request and batching config - tests take ~40 seconds on a laptop, so had to increase timeout duration from 30 seconds to 60 seconds * refactor: simplify PeerConnected event handling and remove unused parameters - Updated the `PeerConnected` event to only include the `PeerId`, removing the unnecessary `StreamProtocol` parameter. - Adjusted related code across multiple files to reflect this change, including updates to the `Status` struct and filtering logic in the `State` class. - Cleaned up the `BREAKING_CHANGES.md` to remove redundant entries regarding enum changes. * fix: accept as valid responses with one value, for peers not supporting batching * refactor(tests): adjust voting power and timeout duration in late start tests with batching * Buffer full sync response for higher heights, value and certificate * refactor(tests): one peer filtering test with structured test cases * Rename ValueResponse to SyncValueResponse * Update release notes and breaking changes * fix(code): Update consensus queue capacity calculation to include `sync.batch_size` Without this fix, the queue capacity was overflowing. Also, reduce the default batch_size from 1000 to 100. * refactor(code)!: Restore `GetDecidedValues` to `GetDecidedValue` and fetch values from app one by one With this change we leave the host API unchanged. This optimization could be added later. * refactor(code): Simplify sync message handling by removing request_id from InvalidValue and ValueProcessingError Required to simplify merging #1149: fix(code/sync): Buffer full sync response for higher heights, value and certificate * Make better defaults * Add failing test for restart height * Formatting * Update default params in `spawn-tmux.bash` script * refactor: fix comments on valid value * fix comment Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Signed-off-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> * fix comment Signed-off-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> * On restart height: - empty state.msg_buffer - empty state.consensus.input_queue * clear pending requests on reset height * refactor: restore BTreeSet for network peers instead of BTreeMap, which now is not used - This code comes from the first implementation and now is not used. - Removed unused Default implementation for Status struct. * Prevent boolean blindness by using an enum instead of a bool for denoting restarts --------- Signed-off-by: Hernán Vanzetto <15466498+hvanz@users.noreply.github.com> Co-authored-by: Anca Zamfir <zamfiranca@gmail.com> Co-authored-by: Romain Ruetschi <romain@informal.systems> Co-authored-by: nenadmilosevic95 <50905385+nenadmilosevic95@users.noreply.github.com> Co-authored-by: Nenad Milosevic <nenadmilosevic@Nenads-MacBook-Pro.local>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #1139
Part of #1136
PR author checklist
For all contributors
RELEASE_NOTES.mdif the change warrants itBREAKING_CHANGES.mdif the change warrants itFor external contributors