Repository navigation
Land verified index and snapshot stack - #1696
Conversation
Use one historical error mapper and one complete-scan admission path while preserving every error variant.
Use one strict BodyReader resolver for export and restore, rejecting embedded serializations and incomplete bundle positions without changing the format.
![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 lands the verified index and snapshot stack, introducing ~21k lines of new code primarily in crates/index. The new subsystem implements portable bundle export/restore for the TxLookup family, a reconciliation state machine, and associated storage operations.
Assessment: The implementation is well-engineered and ready to merge.
After a deep review of the core new subsystem — the bundle pipeline (export.rs, restore.rs, artifact.rs, manifest.rs, chain.rs), the reconciliation state machine (reconciliation.rs), and startup/namespace management (startup.rs) — no blocking defects were found.
Key correctness properties verified:
- Tamper resistance: The two-pass (validate → apply) restore pipeline re-hashes the artifact file before the final watermark commit, so a file altered between validation and apply is refused before readiness is stamped.
- Atomicity: Rows without a
TxLookupwatermark stay invisible to queries; readiness appears in one final fenced commit that writes both rows and watermark atomically. - Partial-import cleanup:
discard_partial_rowscorrectly escalates toIndexBundleError::Indexon reset failure, preventing partial rows from mixing with derived rows. - Path traversal prevention:
artifact.nameis validated against the exact constantBundleFamily::TxLookup.file_name()invalidate(), refusing names like"../escape.dat". Thecreate_newfile creation provides the actual atomic protection against races. - Locator verification: The strict top-level-span rule (
is_top_level_span) correctly prevents embedded transaction serializations (inside scripts/witnesses) from resolving locators. - Namespace poisoning (IDX-07): Abandoned opens (timeout/stop) poison their namespace key for the process lifetime, preventing a detached open thread from racing with a new worker on the same store.
No blocking issues identified. The code ships with thorough unit tests for every described invariant.
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}
Port the bounded page API and export stream, clear interrupted imports with one bounded probe, and buffer UTXO seed reads. The remote branch already retains parsed bundle block spans.
Merge origin/main (Tor/I2P overlay #1684, public listener #1685) into the index-snapshot lane. Branch intent preserved: BlockPayload unification, index bundle snapshot config/tests, and layered config coverage. Resolutions: - crates/node/src/config.rs: union; keep validate_index_bundle (branch) and p2p.overlay.validate (main), one call each. - crates/p2p/src/listener.rs send_block: main PeerRemote param (impl Into<PeerRemote> + into()) with branch BlockPayload body; all nine merged call sites pass payload, so main-only (block, serialized) would break every caller and the already-merged forward_block payload form. - crates/p2p/src/listener.rs handle_compact_outcome: main PeerRemote param with branch Option<(Hash256, Message)> flow; the merged caller in process_compact_wire_message destructures Some((hash, follow_up)), so main-only bool would break it. No ListenerRole restored. - crates/p2p/src/listener/aggregate_budget_tests.rs: main PeerEndpoint addr with branch .is_none() assertion. - crates/rpc/tests/core_compat.rs: main ipv4/ipv6/onion/i2p networks pin (replaces stale len==3 the branch had dropped); branch BlockPayload test changes kept via automerge.
Merge origin/main at fa187f9 (scan ranged and multipath public descriptors #1676; compact-block scan scoping #1677) into the index-snapshot lane. Clean automerge; prior main-merge resolution (2609ba4: overlay validators, PeerRemote/BlockPayload listener hybrid, PeerEndpoint tests, four-network RPC pin) preserved untouched.
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: 37460754ee
ℹ️ 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".
…ount cache Stored locator order is txid order, so consecutive records usually name different heights and a one-body memo reloaded and reparsed a block per record. The LRU keeps recently used parsed bodies resident across interleaved lookups (8 bodies, ~32 MiB worst case). Counts observed on load are retained after eviction so the completeness pass reuses the first parse instead of re-reading every body.
Each page is scanned through a fresh snapshot that must still show the captured base watermark and floor, then dropped before body I/O. A long export no longer pins one KV snapshot across the whole family; a moved watermark aborts stale before any manifest is published.
Block headers are shared identity kept when any capability cursor remains, so a restore onto a node with other capabilities rewrote every covered header. A point lookup per header skips present ones with no extra memory; overwriting stays idempotent for absent ones.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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". |
Resolutions (branch LRU/paged fixes kept; main contracts kept): export.rs keeps branch per-page short-lived snapshots plus height-grouped LRU staging and adds main ExportTooLarge cap; drops main shared-snapshot helper and paged tests (superseded; same bounded-paging behavior). restore.rs keeps branch BodyReader LRU for all body I/O plus main height-grouped pending drain, RestoreTooLarge cap, and mismatch-before-incomplete fault order; drops main ParsedBlock-direct paths (equivalent strict-span rule and metadata reuse via BodyReader). snapshot.rs combines branch bounded 64KiB buffering with main below-tee layering; keeps main VARINT ordering.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cc9ca1087
ℹ️ 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".
Export sorted only within each 1,024-row page, so one block's transactions scattered across pages and the 8-body LRU reloaded whole bodies about 1.86M times for 4,096 blocks. The export now stages every stored row into an anonymous temp file, sorts the staging file by (height, sequence), resolves the height-ordered rows so each covered body is read and parsed once, then re-sorts the resolved rows by sequence and emits them in stored order. The artifact keeps its strictly ascending record order while body I/O stays height-local. Bounded pages, bounded sort runs, and one row held at a time keep export memory flat in family size; per-page snapshot watermark/floor checks and the end-to-end fenced watermark compare are unchanged, and cancellation is polled every 4,096 scanned, resolved, or emitted rows. Adds a regression test over 16 blocks and multiple pages proving each block body is loaded exactly once and the bundle restores like a backfilled index.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf642edd25
ℹ️ 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".
# Conflicts: # crates/index/src/portable/chain.rs # crates/index/src/portable/export.rs # crates/index/src/portable/restore.rs # crates/utxo/src/snapshot.rs
|
@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". |
## Summary Follow-up to #1696 for two review findings that arrived after the merge. - `external_sort`, run generation, merge, and copy now poll the cancellation probe every bounded chunk, so shutdown does not wait for a multi-million-row sort. - Enumeration now validates positions and charges the shared 4,194,304-position budget before writing or sorting, returning `ExportTooLarge` before temporary-storage growth. ## Verification - `cargo test -p bitcoin-rs-index --lib export::tests --locked` (9 passed) - `cargo test -p bitcoin-rs-index --lib portable --locked` (26 passed) - `cargo test -p bitcoin-rs-index --features fjall --test portable_bundle --locked` (9 passed) - `cargo check --workspace --all-targets --locked` ## Synchronization impact No authoritative sync, validation, or commit path changes. The changes are limited to index bundle export cancellation and staging. No `SYNC-*` clause is affected.
Draft subsystem stack split from PR #1687.
Base: f024fc3
Tip: 80be603
Full repository gate: native-gate-20261027T194739Z-194405 (scripts/ci-pr.sh all, exit 0).
Scope: index and snapshots (portable bundles, restore, query).
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
Do not merge until the observer lock regression is fixed and the explicit repository requirements are met.
Findings
Summary
Adds portable transaction-index bundles, snapshot-backed
ScriptLiveseeding, and covered-prefix queries for embedders.TxLookupbundles between nodes.ScriptLiveseeds from a verified snapshot and catches up from its height.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Checkpoint or snapshot activation] --> B[Witnessed snapshot file] B --> C[Worker seed slot] C --> D[Write live rows in bounded batches] D --> E[Check complete snapshot digest] E --> F[Commit live watermark] F --> G[Reconcile with active chain] H[Portable bundle] --> I[Check records against local blocks] I --> J[Import rows and commit transaction watermark] J --> G G --> K[Strict or covered-prefix queries]Reviews (4) · Last reviewed commit: "Adapt merged sync tests to branch owners..." · Reviewed by Greptile
Synchronization impact
(a) This PR does not touch an authoritative synchronization, validation, or commit path. It changes index bundle export and restore caching and paging, and index-side seed traversal cancellation. It consumes existing chain views and does not alter scheduling, validation, or commit ownership.
(b) No
SYNC-*clause is affected.(c) The smallest existing behavioral proof is
cargo test -p bitcoin-rs-index --features fjall --test portable_bundle(8 tests) andcargo test -p bitcoin-rs-index --lib portable(25 tests). These exercise export and restore against the current chain view and do not cross a synchronization contract boundary.