Enable downloading of transaction sets in parallel with early SCP stages - #5209
Conversation
285df2f to
1692131
Compare
|
I just rebased this on master and force pushed up so we can get a CI run. Ignore the terrible branch name; this is built on top of protocol 26, not 25. |
cf854d3 to
0564c22
Compare
2af1efe to
c68cf84
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR introduces support for parallel transaction-set downloading in SCP, including CAP-0083 empty-tx-set values, and extends tests/metrics/configuration to validate and observe the new behavior.
Changes:
- Extend
SCPDriver/SCP protocols to distinguish fully-validated vs structurally-valid values and to support parallel tx-set download readiness. - Add empty-tx-set value handling (hash sentinel, creation/validation, replacement on timeout) across Herder/SCP/Ballot/Nomination.
- Add config knobs, overlay fetch wait-time reporting, and substantial new tests + metrics for the new flow.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/util/ProtocolVersion.h | Defines protocol gating constant for empty-tx-set values. |
| src/scp/test/SCPUnitTests.cpp | Updates test driver interface conformance for new SCPDriver methods. |
| src/scp/test/SCPTests.cpp | Adds test hooks for structurally-valid values, tx-set download timing, and new CAP tests. |
| src/scp/SCPDriver.h | Adds new driver APIs for download-wait/timeout, envelope readiness, empty-tx-set support, and new validation levels. |
| src/scp/NominationProtocol.cpp | Allows nomination flow to consider structurally-valid values. |
| src/scp/BallotProtocol.h | Renames/clarifies statement validation + adds empty-tx-set replacement and confirm-commit invariant check. |
| src/scp/BallotProtocol.cpp | Implements structurally-valid envelope handling, timeout-to-empty-tx-set replacement, and confirm-commit invariant enforcement. |
| src/protocol-curr/xdr | Updates protocol XDR submodule pointer. |
| src/overlay/Peer.cpp | Adjusts getTxSet handling to new TxSetResult type. |
| src/overlay/ItemFetcher.h | Adds API to query fetch waiting time for an item. |
| src/overlay/ItemFetcher.cpp | Implements fetch waiting time lookup. |
| src/main/Config.h | Adds experimental parallel tx-set download flag and timeout config. |
| src/main/Config.cpp | Implements defaults and config parsing for new parallel download settings. |
| src/ledger/test/LedgerCloseMetaStreamTests.cpp | Enables parallel tx-set download in test configuration. |
| src/ledger/LedgerManagerImpl.cpp | Allows EMPTY_TX_SET_HASH through txset hash mismatch checks. |
| src/herder/test/PendingEnvelopesTests.cpp | Expands tests for envelope readiness w/ parallel download and empty-tx-set gating. |
| src/herder/test/HerderTests.cpp | Updates tests for new TxSetResult and adds validation coverage for missing tx sets. |
| src/herder/TxSetFrame.h | Adds makeEmpty(previousLedgerHash, previousLedgerVersion) overload. |
| src/herder/TxSetFrame.cpp | Implements new makeEmpty overload while preserving existing API. |
| src/herder/PendingEnvelopes.h | Moves txset access to TxSetResult and adds readiness/wait-time helpers. |
| src/herder/PendingEnvelopes.cpp | Adds EMPTY_TX_SET_HASH handling, wrapper update hook, readiness logic, and waiting-time plumbing. |
| src/herder/LedgerCloseData.cpp | Allows EMPTY_TX_SET_HASH in constructor invariant and improves StellarValue string formatting. |
| src/herder/HerderUtils.h | Adds throwing toStellarValueOrThrow helper. |
| src/herder/HerderUtils.cpp | Implements throwing conversion with diagnostics. |
| src/herder/HerderSCPDriver.h | Adds empty-tx-set APIs, readiness, onTxSetReceived wrapper updates, and metrics hooks. |
| src/herder/HerderSCPDriver.cpp | Implements wrapper registries, readiness logic, empty-tx-set value creation, and tx-set-block metrics. |
| src/herder/HerderImpl.h | Changes getTxSet return type to TxSetResult and updates comment. |
| src/herder/HerderImpl.cpp | Handles EMPTY_TX_SET_HASH on externalization and updates txset/signature logic for new value types. |
| src/herder/Herder.h | Introduces EmptyTxSet/TxSetResult and EMPTY_TX_SET_HASH sentinel. |
| src/herder/Herder.cpp | Defines EMPTY_TX_SET_HASH constant. |
| src/Makefile.am | Adds CAP_0083 define plumbing for XDR generation. |
| docs/stellar-core_example.cfg | Documents new config settings. |
| docs/metrics.md | Documents new empty-tx-set and blocked-on-txset metrics. |
| configure.ac | Adds conditional for enabling CAP_0083 builds. |
| common.mk | Adds CAP_0083 define plumbing for compilation. |
Comments suppressed due to low confidence (4)
src/scp/BallotProtocol.cpp:1
case SCPDriver::kStructurallyValidValue:currently falls through todefault:without an explicitbreak/annotation. This can trigger implicit-fallthrough warnings and obscures intent. Add an explicitbreak;at the end of thekStructurallyValidValuecase (or mark it[[fallthrough]]if intentional, though abreakseems correct here since the case already completed its checks).
// Copyright 2014 Stellar Development Foundation and contributors. Licensed
src/scp/test/SCPUnitTests.cpp:1
- These non-void overrides call
releaseAssert(false)but do not return a value, which can fail compilation under-Werror=return-typeor similar settings. Add an explicit return after the assertion (e.g.,return {};/return false;) to satisfy the compiler even in non-asserting builds/configurations.
src/scp/test/SCPUnitTests.cpp:1 - These non-void overrides call
releaseAssert(false)but do not return a value, which can fail compilation under-Werror=return-typeor similar settings. Add an explicit return after the assertion (e.g.,return {};/return false;) to satisfy the compiler even in non-asserting builds/configurations.
src/herder/PendingEnvelopes.cpp:1 - This log message is now potentially misleading: the predicate can drop envelopes not only for "value not signed" but also for disallowed empty-tx-set values (and potentially other unsupported types). Update the message to reflect the real reason (e.g., "unsupported or unsigned StellarValue type") so operators can diagnose configuration/protocol gating correctly.
#include "PendingEnvelopes.h"
marta-lokhova
left a comment
There was a problem hiding this comment.
Looks good to me overall, thanks for all the cleanups! I just left a couple of clarifying questions. I think we also need to rebase the change.
ebdbe5a to
fdde3ab
Compare
marta-lokhova
left a comment
There was a problem hiding this comment.
Change looks good to me! The remaining questions I have are around test plan and invariance.
bc94f35 to
37e9d85
Compare
Head branch was pushed to by a user without write access
2f35aa0 to
3813e64
Compare
|
The failing test is a binary fuse filter test that is also failing on |
This change adds support for CAP-0083 empty-tx-set values, as well as a new flag `EXPERMENTAL_PARALLEL_TX_SET_DOWNLOAD` that allows validators to download transaction sets in parallel with early SCP rounds.
b4a81f9 to
6bd15f5
Compare
This PR adds support for CAP-0083 and enables downloading of transactions sets in parallel with SCP up until setting
cduring balloting.