Repository navigation
refactor(p2p): split InitialBroadcastSource out of ValidatedPoolReader - #11973
Merged
pierugo-dfinity merged 2 commits intoOct 29, 2026
Merged
Conversation
`ValidatedPoolReader` had two methods with disjoint users: P2P lookups (`FetchArtifact`'s RPC handler and the stripped-message pools) only call `get`, while `create_artifact_handler` only calls `get_all_for_initial_broadcast`. Every implementor had to provide both, so several returned empty iterators or `unimplemented!()` for the method nobody called on them. Move `get_all_for_initial_broadcast` into a new `InitialBroadcastSource` trait and require it only in `create_artifact_handler`. `create_ingress_handlers` used neither method, so it drops the bound altogether, and the ingress pool, the stripped consensus pool wrapper, the mock and the bench fakes no longer implement the unused method. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pierugo-dfinity
changed the title
InitialBroadcastSource out of ValidatedPoolReader`
pierugo-dfinity
changed the title
InitialBroadcastSource out of ValidatedPoolReader`InitialBroadcastSource out of ValidatedPoolReader
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues block the separation of startup broadcasting from artifact lookup.
No open issues 🎉
What changed in this PR
Separates startup broadcasting from artifact lookup in the P2P interfaces, allowing pools and test helpers to implement only the capabilities they use. Existing lookup and broadcast behavior is preserved.
Changes:
- Extracts
InitialBroadcastSourcefromValidatedPoolReaderand updates artifact-manager bounds. - Moves existing startup-broadcast implementations into the new trait.
- Removes unused broadcast stubs and the obsolete ingress test.
| File | Description |
|---|---|
rs/p2p/test_utils/src/mocks.rs |
Removes the broadcast method from the lookup mock. |
rs/p2p/test_utils/src/consensus.rs |
Implements the new broadcast trait for TestConsensus. |
rs/p2p/artifact_manager/src/lib.rs |
Requires the broadcast trait where needed and drops the unused ingress lookup bound. |
rs/p2p/artifact_downloader/src/fetch_stripped_artifact/assembler.rs |
Removes the unused broadcast stub from the consensus wrapper. |
rs/p2p/artifact_downloader/benches/assembler.rs |
Removes unused broadcast stubs from benchmark pools. |
rs/interfaces/src/p2p/consensus.rs |
Defines separate lookup and startup-broadcast traits. |
rs/artifact_pool/src/ingress_pool.rs |
Removes the empty broadcast implementation and its test. |
rs/artifact_pool/src/idkg_pool.rs |
Moves startup broadcasting into the new trait. |
rs/artifact_pool/src/dkg_pool.rs |
Moves startup broadcasting into the new trait. |
rs/artifact_pool/src/consensus_pool.rs |
Moves startup broadcasting into the new trait. |
rs/artifact_pool/src/certification_pool.rs |
Moves startup broadcasting into the new trait. |
rs/artifact_pool/src/canister_http_pool.rs |
Moves startup broadcasting into the new trait. |
Review effort: Balanced
|
✅ No security or compliance issues detected. Reviewed everything up to 7deab30. Security OverviewDetected Code Changes
|
alin-at-dfinity
approved these changes
Oct 29, 2026
pierugo-dfinity
deleted the
pierugo/p2p/split-initial-broadcast-source
branch
October 29, 2026 14:15
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
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.
Why
ValidatedPoolReader<T>had two methods with disjoint users:getis used only by P2P lookups:FetchArtifact's RPC handler and the stripped-messagePools(which serve peers and fill in stripped block proposals).get_all_for_initial_broadcastis used only bycreate_artifact_handlerinic-artifact-manager, when it starts.Every implementor still had to provide both, so several returned empty iterators or
unimplemented!()for the method nobody called on them (the ingress pool, the stripped consensus pool wrapper, the mock and the bench fakes).What
ValidatedPoolReader<T>with onlyget, and moveget_all_for_initial_broadcastinto a newInitialBroadcastSource<T>trait inrs/interfaces/src/p2p/consensus.rs.create_artifact_handlernow requiresInitialBroadcastSourceinstead ofValidatedPoolReader.create_ingress_handlersused neither method (it passes no initial artifacts), so it drops the bound altogether.TestConsensus) implement both traits, with unchanged method bodies.ConsensusPoolWrapper,MockValidatedPoolReaderand the bench fakes only implementget. The ingress pool test that only asserted the removed method returned nothing is deleted.No behavior change.
Testing
cargo check --all-targets --all-features,cargo fmt,ci/scripts/rust-lint.sh(clippy clean for all affected crates).bazel testof the 118 tests within twordepshops of the changed files: all pass.🤖 Generated with Claude Code