Skip to content

refactor(rpc): remove redundant lookup and gettxout conversion layers - #1682

Open
gosunuts wants to merge 4 commits into
mainfrom
refactor/rpc-redundant-projections
Open

gosunuts wants to merge 4 commits into
mainfrom
refactor/rpc-redundant-projections

Conversation

@gosunuts

@gosunuts gosunuts commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

This scoped cleanup removes confirmed lookup and conversion redundancy from #1624 while retaining the separate Core, REST and Esplora wire contracts.

  • Remove the private record_for_hash entrypoint and the forwarding block_by_hash body. The existing public block_by_hash now directly owns tree-authoritative identity resolution and log/body metadata enrichment. Update RPC tests and REST callers; Esplora already calls this public entrypoint. The index change only corrects a comment naming the removed helper.
  • Reuse block_hash_at_height_in_view for fresh height lookups, removing the duplicate tip/genesis/cache branches. A fresh call still captures one publication; retained-view callers still use their original publication. Extend the existing no-tip lookup test to cover hash lookup and its distinct genesis fallback.
  • Delete script_pub_key_typed and replace txout_typed with direct txout_json construction. Previously, gettxout rendered script JSON, deserialized it into ScriptPubKey, serialized the whole typed response, and recursively removed null fields. It now embeds the existing script renderer's JSON directly, preserving the existing sat_to_btc amount conversion. Keep the pinned DTO as a test oracle across every script fixture plus malformed push ASM, amount boundaries, coinbase flags and omitted optional fields.

The concrete block path remains Context tree/log resolution -> getblock, REST block/header routes, or Esplora required_block_record -> each format's renderer. Header-only lookup remains separate because detailed getblock must avoid legacy body metadata reads before acquiring the bounded owner permit. Captured AppliedView reads, stable chainstate barriers, reorg/generation fences and request-local Esplora prevout reuse remain intact. Core typed/null-removal helpers still serve other methods with distinct omission rules. REST block envelopes and Esplora script/transaction projections retain their independent schemas. Registry dispatch retains method lookup, blocking admission and parameter/error policy; HTTP surface routing retains framing/authentication ownership. No measured allocation or latency improvement is claimed.

No sync impact: this changes read-only RPC lookup/presentation paths, not an authoritative synchronization, validation or commit path. It preserves the existing tree/log/body capability boundary, tip capture count and stable-read lock lifetime; no SYNC-* transition or persistence format changes. The RPC unit suite exercises captured tips, concurrent swaps, side branches, pruned/unavailable data and existing fencing/error behavior; this is boundary-local evidence, not a whole-node convergence claim.

Validation on WSL/Linux:

  • cargo fmt --all -- --check and git diff --check: passed.
  • cargo test --locked --ignore-rust-version -p bitcoin-rs-rpc --no-default-features --lib --test core_compat --test core_parity --test transaction_methods: 492 + 18 + 15 + 13 passed. Final unit suite rerun after the last test edits: 492 passed.
  • cargo test --locked --ignore-rust-version -p bitcoin-rs --no-default-features --features fjall --test overhaul_process_harness script_decode_cases::block_rest_and_utxo_consumers_share_core_output_scripts -- --exact: passed against the pinned Core 31.1 process.
  • cargo clippy --locked --ignore-rust-version -p bitcoin-rs-rpc --no-default-features --lib --tests --no-deps -- -D warnings -A clippy::duration_suboptimal_units: passed.

Local verification limitation: installed Rust is 1.95, while the repository declares 1.99, so Cargo checks used --ignore-rust-version. Unsuppressed Clippy fails on existing duration_suboptimal_units warnings in utxo/src/scan.rs:21 and rpc/src/server.rs:22; these unrelated lines are unchanged. Full toolchain qualification and broader process/crash/contention/interoperability campaigns remain with their existing CI lanes.

Refs #1624.

RetriggerView in GreptileConfidence Score: 5/5

The PR appears safe to merge; no actionable regression was found.

Summary

This PR removes forwarding and conversion layers from read-only RPC paths.

  • Block lookups resolve records through one tree-first entrypoint.
  • Height-to-hash lookups use one captured chain view.
  • gettxout builds its response from the script JSON directly.

No actionable issues were found. Tests were not run during this review. The previous-review comparison was not used because the target merge bases differ; the full PR diff was reviewed instead.

Reviews (4) · Last reviewed commit: "Merge branch 'main' into refactor/rpc-re..." · Reviewed by Greptile

Resolve block records directly through block_by_hash and share captured-view height lookup logic. Render gettxout with the existing script JSON owner instead of deserializing and reserializing its output through a DTO.

Refs #1624

@amazon-q-developer amazon-q-developer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR refactors three RPC lookup/projection layers: consolidates record_for_hash/block_by_hash into a single public block_by_hash, eliminates the duplicate block_hash_at_height forwarding function, and replaces the txout_typed/script_pub_key_typed conversion chain with direct JSON construction for gettxout.

Summary of findings: After reviewing all 6 changed files, the refactoring is functionally correct. The rename from record_for_hash to block_by_hash is complete and consistent across all callers. The block_hash_at_height simplification correctly delegates to block_hash_at_height_in_view with a fresh applied_view() capture — the new test at lines 1471–1476 in context.rs specifically covers the genesis fallback case that the old inline code handled. The txout_json direct JSON construction preserves the previous output schema as verified by the comprehensive new test gettxout_preserves_script_schema_and_omits_unset_fields.

No merge-blocking defects were found in the changed code.


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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 6 files

Turn on auto-fix | Re-trigger cubic

Upgrade only ctutils from 0.4.3 to 0.4.4 in the workspace and fuzz lockfiles. This fixes RUSTSEC-2026-0385 without weakening the cargo-deny advisory gate.
@gormes-agent

gormes-agent Bot commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

Approve (comment only, not a formal review, approval or merge) — reviewed head 31be7046fb57f763b7ec5c1634828b25c3dd4b2e against base fe00e2bca6a5ff6d3c32fd38d1b5863bd8e1a5c8.

Verdict

No blocking findings. The refactor is behavior-preserving for the reviewed paths.

Prior finding resolution

  • The earlier ctutils 0.4.4 lockfile conflict with main (fix(deps): update ctutils to 0.4.5 #1678 moved to 0.4.5) is resolved by the merge commit: Cargo.lock and fuzz/Cargo.lock are now identical to the base, and git merge-tree against the base reports 0 conflicts.
  • fb0d0168 is unchanged (range-diff), and the crates/ diff against the new base is line-for-line identical to the one reviewed against 8fb1f30b. The four new main commits only touch lockfiles.

What was checked (source trace)

  • record_for_hash → block_by_hash: all callers updated (REST block/part routes, header_chain_context, block_by_height_in_view, tests); tree-authoritative identity with log body-metadata enrichment is unchanged. No stale references to record_for_hash, script_pub_key_typed or txout_typed remain.
  • block_hash_at_height now delegates to block_hash_at_height_in_view(&self.applied_view(), ..): still exactly one applied_tip load per fresh call, with the same tip / genesis / log-fallback branches.
  • gettxout: top-level field order and sat_to_btc conversion are preserved; the renderer emits no nulls, so dropping the null-removal pass does not change output.

Non-blocking notes

  • Key order inside scriptPubKey changes from the DTO order (asm, desc, hex, type, address) to the renderer order (asm, type, hex, desc, address). This now matches getrawtransaction/REST and is semantically equivalent JSON, but byte-exact consumers would notice. (Neither order matches Core's ScriptToUniv order.) Source-only, assuming sonic_rs preserves insertion order.
  • The runtime DTO round-trip that previously rejected renderer/DTO mismatches is gone; the pinned DTO is now a test oracle (gettxout_preserves_script_schema_and_omits_unset_fields). Reasonable trade-off, but future renderer changes depend on that test's coverage.

Verification and limits

  • CI check-runs at 31be7046: ci-gate, test-crates, clippy, fmt, deny, drift, CodeQL, script-tools succeeded; test-workspace, test-binary, kernel-consensus, full-node-compile were skipped. Check-run status only; job logs were not inspected.
  • Local (Rust 1.99.1, --locked, -p bitcoin-rs-rpc --no-default-features --lib, debug info/incremental off): 32 targeted tests passed (gettxout*, block_by_hash*, block_by_height*, header_hex*, record_at*, gettxoutproof*, gettxoutsetinfo*). The full 492-test lib suite, integration targets and clippy were not run locally.
  • No performance claim is made or evaluated.

@metaphoric-dev

Copy link
Copy Markdown
Collaborator

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 26, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-26T02:05:49.695443Z 6334446 Manual request
ℹ️ 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" or "@codex security review".

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

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: 6334446609

ℹ️ 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".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants