Repository navigation
Land verified payload stack - #1692
Conversation
Use primitive metric conversion, one bounded collector, direct undo-codec re-exports, and one set-level scan-limit check. Keep decode and scan behavior unchanged.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR introduces the "verified payload stack" — primarily refactoring the historical AssumeUTXO validation path to use BlockPayload instead of (Block, Option<Bytes>), adding a witnessed snapshot activation mint (pending_activation_mint), and wiring a VerifiedSnapshotArtifact seed into the load_verified path.
Overall Assessment: The core changes are well-structured. The lifecycle serialization logic in AssumeUtxoManager, the new write_archive<T> generic refactor, and the V4Witness minting path all look correct. No critical logic errors, security vulnerabilities, or crash scenarios were identified that would block merge.
The code correctly handles:
- The
archivedearly-return instep_historical(durablehistorical_heightis already ≥ the replayed height, so no regression) - Lifecycle lock ordering in
recover_pending→step_historical pending_activation_mintis only populated after the durableValidatingcommit succeedswrite_archivelock-before-write / unlink-after-rename ordering remains intact after the generic refactor
No blocking issues were found in this review.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Stream portable index exports through bounded snapshot pages instead of materializing each family, and probe the restore leftover with one row. Parse each retained RPC body once and cache its transactions. Replace the observer body-interest callback with registration-time BodyInterest, so commit reads one atomic before the pool write lock and no observer code runs under a gateway lock. Wire every caller to the explicit interest and document the contract.
Register the ZMQ sequence observer with BodyInterest::Bodies only when the publisher has a rawtx subscriber. Metadata-only notification setups no longer retain accepted transaction bodies.
Main brings #1684 opt-in Tor/I2P overlay, #1685 public-listener PeerRemote design, plus #1676 descriptor scan and #1677 compact-scan bound. Branch brings state-payload plumbing (BlockPayload wire, BodyInterest admission, index bundle validation). Resolutions: - crates/node/src/config.rs: keep both validations; branch validate_index_bundle plus main p2p.overlay.validate. - crates/p2p/src/listener.rs send_block: main PeerRemote parameter on branch BlockPayload body; single-conversion entry. - crates/p2p/src/listener.rs handle_compact_outcome: keep branch Option follow-up return (single enqueue_owned_body_fetch publisher per branch 5dee503) with main PeerRemote parameter. Main header-carried body_fetch_owned mark not restored: branch sync/headers.rs and inbound.rs intentionally supersede it with one atomic enqueue; restoring would reintroduce the racy double-mark 5dee503 deleted. - aggregate_budget_tests.rs: main PeerEndpoint address with branch is_none assertion matching the Option return. - crates/rpc/tests/core_compat.rs: main ipv4/ipv6/onion/i2p networks assertion (branch had dropped the stale len-3 check).
Main compact-sync tests called record_owned_body_fetch, which the branch replaced with enqueue_owned_body_fetch. The NotFoundFixture fields also kept SocketAddr after current_source and test_addr moved to PeerRemote. - Add the missing SocketAddr import. - Store owner and alternate as PeerRemote. - Mark the owned fetch through enqueue_owned_body_fetch.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7e0bd884f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…time Single positions_are_valid in types.rs backed by one MAX_POSITION_END_BYTES (4 MiB, citing consensus MAX_BLOCK_SERIALIZED_SIZE). Writer, query fallback, export, and restore framing all apply it; the writer refuses bodies past the cap so an admitted row is never corrupt to an exporter.
One stored key can no longer accumulate positions without bound during restore/import: past 131,072 positions the fold fails closed with MutationSizeOverflow instead of growing importer RSS. The bound is twice the transaction capacity of a 4 MiB block, so no honest bundle trips it.
Skipped locator verification is now an explicit NotChecked outcome that refuses instead of reporting Verified, so correctness never depends on gate ordering. stream_import revalidates that the bundle base is still on the active chain before the watermark commit; a tip advance past the base stays valid and backfills above it.
MAX_LIVE_SCRIPT_SIZE is now an alias of bitcoin_rs_script::MAX_SCRIPT_SIZE instead of a duplicated literal, removing the stale node-assert comment. History rows deliberately keep oversized scripts (pinned by live_predicate_matches_utxo_admission_not_history); the funding push site now says so with its consensus-block-size bound.
The Accepted-to-body join moves past the pool guard into a HashMap, keeping admission-lock time out of large-package publication. A commit-ordering lock spans mutation through enqueue so the publish queue order stays the commit order; it drops before the drain so observers can re-enter. A missing body with capture enabled warns and debug-asserts instead of silently dropping the envelope entry.
Keeps main's tree alongside the mempool join-outside-the-lock fix: join_accepted_bodies runs past the pool guard with a HashMap join and visible missing-body failure, while the commit_order lock still spans mutation through enqueue so the publish queue order stays the commit order.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # crates/index/src/portable/export.rs # crates/index/src/portable/restore.rs
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
seed_from_bundle only checked should_stop at entry, so a stop landing mid-stream committed the watermark anyway. Thread a cancelled closure through for_each_coin_raw, read_raw_snapshot_stream, and seed_script_live_stream, polling every 4,096 emits/rows plus a pre-commit check. A trip returns IndexError::Cancelled, mapped from RawSeedError::Cancelled, and the worker keeps artifact plus lease and stalls so the retry reuses the same bundle. Focused regression tests at the traversal and fenced-stream levels.
The validating pass drained the artifact tail with a blocking io::copy that never polled host.cancelled. Replace it with a bounded 4,096-byte chunked copy that polls once per chunk and returns Cancelled at once with no digest comparison, preserving ArtifactHash precedence over framing faults. Adds a focused unit test for full drain hashing, entry cancellation, and a mid-tail trip.
Draft subsystem stack split from PR #1687.
Base: f024fc3
Tip: 69ccdb8
Full repository gate: native-gate-20261027T230010Z-1140364 (scripts/ci-pr.sh all, exit 0).
Scope: state and payload.
This is a verified stack tip. Do not rebase or reparent it
without owner approval.
No better-than-Core claim. No mainnet qualification.
See also: #1687
Sync impact: YES.
SyncChain::commit_windowandconnect_historicalmove from ownedBlockplusBytesto borrowedBlockPayloadslices without changing outcomes, follower order, or settlement. Affected clauses: SYNC-02, SYNC-05, SYNC-06, SYNC-04, and ARCH-07b. Smallest proofs:crates/node/tests/sync_smoke.rs,crates/node/tests/overhaul_durable_head.rs, andchain_effects.rs::committed_connect_settlement_failure.Fix snapshot-seed cancellation before merging; a normal shutdown can otherwise abandon a worker that keeps writing.
Findings
Summary
The PR replaces separate block/body pairs with immutable
BlockPayloadvalues. It also adds witnessed snapshot seeds, portable transaction-index bundles, historical prefix reads, and checkpoint side-branch recovery.notfound.rawtx.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR Raw[Wire or stored bytes] --> Payload[BlockPayload] Local[Locally built block] --> Payload Payload --> Apply[Chainstate commit] Apply --> Followers[Ordered followers] Apply --> Checkpoint[Witnessed checkpoint] Checkpoint --> Seed[ScriptLive seed] Seed --> Index[Index rows and watermark] Index --> Queries[Strict and historical reads]Reviews (5) · Last reviewed commit: "Adapt merged sync tests to branch owners..." · Reviewed by Greptile