Repository navigation
Land verified performance evidence stack - #1701
Merged
Merged
Conversation
Separate window preparation, transition, and commit timing. Add replay benchmarks and document the attribution boundary without claiming an unmeasured speed change. See also: #1607
Measure retained heap bytes for decoded blocks and their owners. Expose staging counters and distinguish retained memory from wire payload size so operators can interpret the measurements. See also: #1572
Record the isolated 48-trial comparison and its failed memory and stall guards. Retained-byte attribution remains diagnostic; no count-horizon policy enters the production path.
Evidence must describe the binary that produced it. Record the actual debug-assertion setting and retain runtime replay checks without a constant assertion that fails strict lint.
The harness records debug assertions in its toolchain identity instead of refusing the build. Keep the storage and peak-memory reset gates unchanged.
Align the execution proof with rustfmt and mark the Linux metric name as code so the native verification gate accepts the test. Runtime behavior and assertions are unchanged.
Identify Linux backing counters without confusing pool-wide, process, allocated, or physical write measurements. Preserve unavailable results for ambiguous attribution.
Drop the 'Anchors: John Carmack, Richard Feynman' line from docs/benchmarks/window-pipeline-attribution.md. No measured value, protocol rule, gate, or blocker changes.
…d mounts as diagnostics
![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 a verified performance and evidence stack (split from #1687), adding the Step-21 block-to-template baseline, Step-8 template-render baseline, Step-24 window attribution benchmarks, and associated infrastructure (evidence identity, EvidenceIdentity, Sha256Hex, benchmark ledger support, window measurement subscriber, and the Python filesystem counter tool for block-device I/O attribution).
Structural and consensus changes
The core chainstate and mempool logic changes look correct:
crates/chainstate/src/connect.rs: TheApplyScratchhelper struct was removed and its fields inlined. This is a straightforward refactor — theraw_txscomputation moved from eager (insideApplyScratch::from_prepared_parts) to lazy (handles.capture_rawtx.then(|| …)) after the UTXO commit, which is the correct place given the comment explaining that serialization should be skipped during IBD.crates/chainstate/src/window.rs: Theflush_groupfunction now has a proper early-return guard for an empty group and adds timing/tracing diagnostics for flush wait. The logic is correct and theflush_wait_usfield name matches whatWindowSpanSubscriberin the bench expects.crates/mempool/src/submit_package.rs:check_structurewas moved fromordered()tosubmit_package_with_times(). See the inline comment for a coupling concern.
Performance and evidence infrastructure
The new bench infrastructure (sync_pipeline.rs additions, evidence.rs, filesystem.py, test_filesystem.py) is well-scoped to the bench binary and does not affect production paths. The EvidenceIdentity struct reads the executable from disk at measurement time, which is correct for capturing binary identity.
One finding posted
A comment was posted on the submit_package.rs diff regarding the coupling between ordered() and check_structure now that the check was moved out of ordered().
No blocking consensus, security, or crash-risk defects were found in the production code paths.
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}
Rebuild foreground and historical batches without bodies the owner already refused, pinning the carried cursor to the lowest excluded height. The remote branch already fixes benchmark filesystem scope.
metaphoric-dev
marked this pull request as ready for review
October 28, 2026 09:12
gosunuts
added a commit
that referenced
this pull request
Oct 28, 2026
Window attribution spans and CLI are implemented in PR #1701 at 994e82a, although absent from the docs-cleanup branch. Record that dependency instead of declaring the infrastructure unimplemented across the project. Scope strict-Rust qualification availability to the current checkout. Compared all 20 open PR heads: no k256 verifier or retired overhaul_consensus_matrix/overhaul_native_crypto targets found. Verified #1701 emits both spans and dispatches the CLI. No sync impact: Markdown only; no authoritative runtime boundary crossed. git diff --check and existing documented-target/link validation passed.
added 3 commits
October 28, 2026 21:37
Merges origin/main at 46de452 (Tor/IVNP overlay connectivity, #1684, atop the #1685 public-listener split) into the performance-evidence lane. Listener (crates/p2p/src/listener.rs, aggregate_budget_tests.rs): main's #1685 addressing design wins - peer_addr stays impl Into<PeerRemote>, test addrs stay PeerEndpoint, no ListenerRole, no rpc-public-bind. The compact-outcome ownership path keeps the branch's enqueue design (Option return, single linearizable enqueue_owned_body_fetch call site, 3-arg send_headers without body_fetch_owned) because the merged sync/inbound tree carries only that API: record_owned_body_fetch and body_fetch_owned exist nowhere in the merged tree, so main's bool/record caller could not resolve. Branch block_not_found/NotFound release path and block_not_found_tests kept; their sync.block_not_found dependency merged cleanly. All other auto-merged hunks (mempool admission/gateway/mutation/orphan, node config/options, sync, tx_relay, rpc compat, docs, Cargo.lock) staged as merged. Discarded: main's bool handle_compact_outcome body and record_owned_body_fetch caller, incompatible with the merged sync ownership API.
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.
Collaborator
Author
|
@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: 994e82acf4
ℹ️ 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".
metaphoric-dev
pushed a commit
that referenced
this pull request
Oct 28, 2026
… cells - window-pipeline-attribution: transition owner is ChainTransition::connect_window (delegates to apply_window_admitted); spans gate #1701 harness validity (panic on missing event), never a production decision. Spans/harness stay labelled implemented-in-#1701, absent-from-#1703; replay stays diagnostic-only on coinbase-only blocks. - lightning-interop-matrix: five LND cells passed -> not exercised; the only record (parent-live-4) is gitignored scratch absent from this checkout (61-dirty tree, diverged runner copy, unverified manifest signature, no Core comparison). Provenance section now calls it an unretained scratch record whose verdict is not shippable. - validation-tooling: LND bullet matches the matrix (not exercised).
added 4 commits
October 28, 2026 22:09
Collaborator
Author
|
@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". |
metaphoric-dev
added a commit
that referenced
this pull request
Oct 28, 2026
Draft subsystem stack split from PR #1687. Base: f024fc3 Tip: 0704754 Full repository gate: native-gate-20261028T001206Z-2627348 (scripts/ci-pr.sh all, exit 0). Scope: docs cleanup (high certainty only; child of 09). 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 ## Cross-PR implementation clarification (7cf8e73) Window attribution is implemented in sibling PR #1701 (audited head 994e82a): chainstate emits transition_held_us and flush_wait_us, and sync_pipeline::main dispatches --window-attribution. Those changes are absent from #1703, so its guide now describes an integration dependency instead of claiming the infrastructure is unimplemented project-wide. No campaign execution or measurement result is inferred. The strict-Rust qualification guide explicitly scopes availability to this checkout. Source/target checks across all 20 open PR heads found no k256 verifier or overhaul_consensus_matrix / overhaul_native_crypto targets. Those qualification gaps remain distinct from #1701's implemented measurement infrastructure. Validation: git diff --check and the documented Cargo target/feature and relative-link checks passed. This clarification changes Markdown only; no additional sync impact. ## Documentation follow-up (c451135) The validation guide now describes the implemented secp256k1-backed native verifier, names runnable regression targets, and marks missing strict-Rust implementation and qualification coverage as BLOCKED. Current index query behavior lives in the index crate guide; the superseded brainstorm and stale task-number comments are removed. Validation on WSL/Linux, Rust 1.99.0: - Five script integration targets: 31 passed. - Four consensus integration targets: 14 passed. - Changed-file rustfmt and git diff --check: passed. - Cargo metadata confirms all five documented regression/benchmark commands name existing package targets and features. Relative Markdown links resolve; source changes are comments only and Cargo manifest semantics are unchanged. - Whole-workspace cargo fmt --all -- --check fails on pre-existing formatting in crates/p2p/src/download_window.rs (lines 104 and 1737), unchanged by this follow-up. - Benchmark campaigns, strict-Rust qualification and full-mainnet replay were not run. The full repository gate recorded above belongs to the earlier stack revision; it does not qualify this new commit. No additional sync impact from this follow-up: only documentation and comments change. It crosses no authoritative sync, validation, commit, persistence or lock boundary; no additional SYNC-* clause is affected. The existing sync-impact declaration below continues to describe the earlier runtime changes. **Sync impact: YES.** Foreground and historical download batches now exclude bodies an owner refused and pin the carried cursor to the lowest excluded height. Affected clauses: SYNC-04 (recovery closure) and SYNC-06 (work drivers). Smallest proof: `crates/p2p/src/sync/tests.rs` notfound and `sync/tests/historical.rs` historical-notfound regressions. <!-- greptile_comment --> <!-- greptile_summary --> <h2><a href="https://app.greptile.com/api/retrigger?id=89464748"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/RetriggerDark.svg?v=2"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=2"><img alt="Retrigger" src="https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=2" align="right"></picture></a><a href="https://app.greptile.com/superpowers/github/gosuda/bitcoin-rs/-/pull/1703?utm_source=greptile_review&utm_medium=github&utm_campaign=view_in_greptile"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewInGreptileDark.svg?v=2"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewInGreptile.svg?v=2"><img alt="View in Greptile" src="https://greptile-static-assets.s3.amazonaws.com/badges/ViewInGreptile.svg?v=2" align="right"></picture></a>Confidence Score: 5/5</h2> <!-- greptile-impact-verdict --> The latest documentation changes appear safe to merge. <details><summary><h3>Summary</h3></summary> The latest change clarifies which branch contains the window benchmark code. - Restarts keep eligible side branches in the header tree. - Peer sync replans blocks after a peer says it cannot provide them. - Compact-block follow-up requests publish ownership when they are sent. - Locator-based header replies now follow the applied chain. - ZMQ publishes accepted transaction bodies on the `rawtx` topic. - `sendrawtransaction` accepts `false` as the default fee limit. - Block responses use one applied-chain view for their chain details. - Checkpoint file access rejects unsafe entries and rename paths. - The Lightning guide and runner describe interoperability checks and their evidence. - Guides distinguish implemented behavior from proposed work and missing evidence. - Supporting build and test updates keep the affected workflows aligned. No new actionable issues found. The previous threads were rechecked; none requires another comment. No tests or benchmark campaigns were run during this review. </details> <!-- greptile_confidence_score:5 --> <sub>Reviews (5) · Last reviewed commit: ["docs: distinguish sibling PR implementat..."](7cf8e73) · [Reviewed by Greptile](https://www.greptile.com/?utm_source=greptile_review&utm_medium=github&utm_campaign=review_public&utm_content=gosuda/bitcoin-rs)</sub> <!-- /greptile_comment -->
added 2 commits
October 28, 2026 23:58
…evidence # Conflicts: # crates/chainstate/src/window.rs # crates/chainstate/tests/unit/apply/observability_tests.rs # crates/p2p/src/block_stager.rs # crates/p2p/src/sync/historical.rs # crates/p2p/src/sync/requests.rs # crates/primitives/src/block.rs # docs/benchmarks/window-pipeline-attribution.md # docs/observability.md
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. 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". |
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
![https://greptile-static-assets.s3.amazonaws.com/badges/RetriggerDark.svg?v=2"><source](https://greptile-static-assets.s3.amazonaws.com/badges/RetriggerDark.svg?v=2"><source){kind=link}
![https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=2"><img](https://greptile-static-assets.s3.amazonaws.com/badges/Retrigger.svg?v=2"><img){kind=link}
![https://greptile-static-assets.s3.amazonaws.com/badges/ViewInGreptileDark.svg?v=2"><source](https://greptile-static-assets.s3.amazonaws.com/badges/ViewInGreptileDark.svg?v=2"><source){kind=link}
![https://greptile-static-assets.s3.amazonaws.com/badges/ViewInGreptile.svg?v=2"><img](https://greptile-static-assets.s3.amazonaws.com/badges/ViewInGreptile.svg?v=2"><img){kind=link}
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.
Draft subsystem stack split from PR #1687.
Base: f024fc3
Tip: 5c39599
Full repository gate: native-gate-20261027T234159Z-2602207 (scripts/ci-pr.sh all, exit 0).
Scope: performance and evidence.
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
The changes appear safe to merge; the previous batch-request blocker is fixed.Summary
This stack adds performance evidence and changes several supporting paths:
Since the last review, block requests now filter unavailable hashes instead of rejecting entire batches. The numbered finding is fixed. The two unnumbered filesystem findings are also fixed: partitions check their parent’s backing devices, and mount-table reads preserve carriage returns. No new actionable issues were found. No performance or mainnet qualification is inferred.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Plan peer batch] --> B[Remove hashes this peer lacks] B --> C{Any entries remain?} C -->|No| D[Try next peer without marking] C -->|Yes| E[Send remaining entries] E --> F[Mark sent entries pending] F --> G[Keep cursor at earliest excluded height]Reviews (3) · Last reviewed commit: "Exclude unavailable blocks per P2P reque..." · Reviewed by Greptile