Repository navigation
Land verified RPC stack - #1693
Conversation
Closes #1599 item 1. API-02 already claimed that envelopes, batches, notifications, and error ordering follow the pinned Core 31.1 contract, but the transport accepted any jsonrpc value as legacy and passed any params shape to dispatch. - Add JsonRpcVersion::classify, mirroring Core's JSONRPCRequest::parse (src/rpc/request.cpp:206-260): a present jsonrpc field must be a string and must be "1.0" (kept as legacy for compatibility) or "2.0"; a present params field must be an array, an object, or null; method must be present and a string. A violation is -32600 with Core's message and the method never runs. - Map legacy HTTP statuses as Core does (src/httprpc.cpp:41-56): -32600 -> 400, -32601 -> 404, every other code -> 500. v2 dispatch errors stay inside HTTP 200. An envelope violation is detected before the claimed version is trusted, so it takes the legacy mapping in either dialect. - A batch now always answers HTTP 200 and carries per-member errors in its array (httprpc.cpp:200-201); an all-notification batch still answers 204. - Add the missing Bad Request and Not Found reason phrases. Tests: 11 new crates/rpc/src/server.rs cases cover each rule and were confirmed to fail before the change for the intended reason, plus the listener-level crates/rpc/tests/auth.rs case now pins 404 for a legacy unknown method instead of the previous 500. The pre-commit hook's p2p profile hit listener::writer_shutdown_tests::writer_exit_shuts_down_the_reader_socket_clone, a socket-timing flake outside this change (crates/rpc only); it passes on rerun and in the five consecutive runs used to confirm the flake.
…laims Addresses the review of #1643: - `RpcError::InvalidRequest` rendered an `invalid request: ` prefix, so the envelope errors did not carry Core's message text. The payload is now rendered verbatim, and the tests assert the exact strings Core produces (`jsonrpc field must be a string`, `JSON-RPC version not supported`, `Params must be an array or object`). - Add `a_rejected_envelope_never_dispatches_the_method`: it first shows the probe method (`getrpcinfo` without a configured log path) fails with `-32603` when dispatched, then that three malformed envelopes produce `-32600` instead — so the contract's "the method never runs" now has an observable assertion. - Add `an_empty_batch_is_rejected_as_an_invalid_request` and state that behavior as local (the pinned Core source does not specify an empty batch), rather than as parity. - API-02 now says an explicit-null `jsonrpc` is legacy, matching the parser, and qualifies the batch 200 rule as applying to non-empty batches.
…bution Addresses the review of #1643: - Add `an_all_notification_batch_answers_204`, which API-02 now cites for the 204 claim (previously the clause had no clause-local proof). - The empty-batch test comment attributed the rejection to Core; the contract already records that the pinned Core source does not specify an empty batch, so the comment now describes `-32600` as local behavior.
Core's parser assigns the JSON-RPC version before it validates method and params (request.cpp:206-260), so a v2 request that fails a later envelope check still replies in the v2 envelope — but takes HTTP 400 from the outer error path (httprpc.cpp:189-194) rather than the dialect's 200. Adopt that mapping and align the remaining envelope details with the pinned Core source: - an explicit "method":null is "Missing method" (Core checks null before the string check), - absent or explicit-null params dispatch as [] so no-argument methods apply their defaults, - a rejected envelope provably never executes the method, now covered by a setnetworkactive dispatch probe. Also extract the server test module into src/server/tests.rs per the source-organization policy.
…y-batch cases - v2 requests refused on method/params validation answer HTTP 400 with the v2 envelope kept (the jsonrpc field itself was already accepted); a rejected jsonrpc field still falls back to the legacy envelope. - Explicit "method":null is "Missing method", matching Core's isNull()-before-isStr() order (request.cpp:228-232). - A top-level scalar/array-non-object is -32700 "Top-level object parse error" at HTTP 500 with a null id (httprpc.cpp final else). - An empty batch answers HTTP 200 [] (Core's batch reply path). - Keep the historical 'invalid request: ' prefix on the malformed-HTTP body; update API-02 to the corrected status/dialect mapping.
sats_from_btc took an f64, so values like 1e-9 rounded through binary floating point before the satoshi check. Parse the JSON number or string as exact decimal text, expand e/E notation with integer shifts, and normalize trailing zeros the way Core ParseFixedPoint does before the eight-decimal precision check. 1e-8 is one satoshi; 1e-9 is Invalid amount. Every amount caller now shares this one parser. Also forward maxfeerate into both package passes, clamp signed modified fees at zero before the fee-rate cap so a negative prioritisation delta can never become u64::MAX, and keep the single structural gate in submit_package_with_times.
Core ParseFixedPoint requires a digit on each side of a decimal point, but the exact parser accepted "1." and ".1". Require the integer part, and a non-empty fraction when a point is present, so malformed amounts fail closed instead of silently taking a value Core refuses.
effective_includes now credits each accepted member with the wtxids that actually subsidize it: self plus descendants in the package, so a bumped parent reports [parent, child] while the paying child reports itself. The residency recheck distinguishes the removal cause: a resident txid with a different witness reports DifferentWitness, and a fully absent member reports MissingInputs rather than a capacity eviction. check_structure classifies duplicate members before the topology guard, so [tx, tx] reports PackageDuplicates instead of a topology refusal. The submitpackage handler bounds maxburnamount at the monetary ceiling and stops echoing raw hex in the decode error. Review: PR #1667 submit_package.rs:441
POL-03 names the pre-script check_package_max_feerate exception, and POL-02, POL-05, MPL-04 and the embedding citation gain clause-local proofs for the RPC package surface. Review: PR #1667
effective_fee_rate now sums modified fees and vsize over the same members each row reports in effective_includes: the bumped parent reports the package rate, the paying child its own. A failed modified-fee lookup nulls the rates instead of falling back to base fees, matching the old aggregate's fail-closed behavior. The CPFP test asserts the package rate on the parent row and the child's own rate on the child row. Review: PR #1667 submit_package.rs:485
…ember Core 31.1 atomically commits a package as one subpackage: every member reports the full package's aggregate effective-feerate and the complete member set in effective-includes, including the paying child. The branch priced each member's own subpackage instead, diverging on the partial- acceptance harness row. Classify unordered duplicate pairs as topology violations before the structural verdicts, matching Core's -25 'package topology disallowed' for [parent,parent] submissions; the dedicated duplicates branch was unreachable for unordered pairs. Gate: ci-pr.sh fmt + clippy passed in the pre-commit run; all test profiles re-run green on this tree (rpc no-default-features, binary rocksdb/fjall/redb harness incl. submitpackage parity cases, workspace kernel-free 1365, rpc zmq 641); cargo deny clean (advisory fetch needed GIT_CONFIG_GLOBAL=/dev/null to bypass the git-manager rewrite).
Callers had to implement BIP337 contextual bit packing and signature recovery themselves. One pure codec behind a runtime opt-in now serves compressrawtransaction and decompressrawtransaction, resolving prevouts and flat output indexes through one bounded RPC resolver over the existing chain and body owners. The codec records the five reference readings where the draft disagrees with itself (Core VARINT over the 7-bit prose, the missing pkh row, the closed signature set, the reference minimum-height rule, and compressed outpoints only with confirmed context). All budgets are charged before allocation; oversize, structural, and budget failures keep distinct error codes. The four published vectors restore byte-for-byte, and the checked-in fixture rows are derived from real mainnet bodies at heights 833265 through 833280; an ignored truth test re-derives every field from those bodies. Measured on the vectors: 52 to 67 percent of raw size, encode 1.5 to 3.8 us, decode 1.1 to 314 us by script type.
scantxoutset rejected ranged and multipath descriptors, forcing wallets to pre-expand them client-side, and scanobject_descriptor discarded the range it had parsed. The existing descriptor owner now expands ranges with aggregate preflight against the remaining 1,024-script and 1 MiB budgets before deriving anything, checks cancellation before every derivation, expands index-major/branch-inner, and keeps first-wins attribution for duplicate scripts. Fixed descriptors validate and ignore the range; the whitelist, checksum, private-key, and network refusals are unchanged.
getrpcinfo always answered an empty active_commands array because nothing tracked in-flight calls. Handler::dispatch now registers each call after registry resolution and blocking-quota acquisition, so unknown or refused methods never appear; an RAII guard removes exactly its own entry on every exit path, and getrpcinfo renders method plus elapsed microseconds oldest first, including itself. The gettxoutproof fallback scan gains record and body-byte counters. The candidate bounds (10,000 records, 64 MiB of bodies) are not adopted: the scan runs forward from genesis, so a late-window success at a 972,861 tip must visit about 973k records and read bodies orders of magnitude past 64 MiB (the 16 sampled mainnet bodies at 833265-833280 average 1.73 MB). That fails the four-times headroom gate, so the current scan stays and the bound remains blocked on representative evidence for a scan order or index change that makes late-window cost small. Also restores the server tests the envelope candidate's test-module extraction dropped (accept loop, head-auth, and drain suites) and corrects the README HTTP mapping.
Use the complete heading fragments owned by the API contract. Regenerate the reference from its registry so all four links resolve consistently.
Pin the Core RPC endpoint so an IPv4 bind collision cannot leave an IPv6-only process waiting on the wrong service. Require positive child bind evidence before adopting an RPC response. Keep the existing bounded port retry and readiness budgets. The readiness regressions and real decoder comparison pass in the 102-test correction union with strict lint.
Use checked subtraction for decimal precision so extreme negative exponents return InvalidParams instead of overflowing in overflow-checking builds. Cover the exact 1.1e-2147483647 regression.
…free recovery, base-fee cap, package floor, burn guard, hashtx Core VARINT spans ten bytes for u64 amounts while u32-capped readers keep five; P2PKH compression requires an exactly round-tripping pubkey and falls back otherwise; decompression recovery runs outside the stable chainstate lock with a tip-move recheck; single-tx maxfeerate gates on the base fee in submit and preview; native packages reject a below-floor child on its parent's fee while CPFP roots still ride the aggregate; sendrawtransaction honors maxburnamount and rejects unknown extra params; hashtx publishes on accepted mempool admission only, with focused regressions.
Use contextual test failures, simplify command iteration, and remove stale lint expectations without changing assertions.
Separate precision, malformed-input, and ceiling assertions into focused tests while preserving all amount checks.
Use one saturating integer helper, one ZMQ endpoint validator, and direct typed ScriptPubKey construction without changing response shapes.
Require exact push reconstruction for specialized P2PKH encoding and document actual batch HTTP status behavior.
Emit rawtx, hashtx, and sequence notifications from the Accepted observer arm with subscriber coverage and no duplicate publication.
There was a problem hiding this comment.
Review Summary
This PR introduces the verified RPC stack, covering JSON-RPC envelope handling, sendrawtransaction improvements (burn guard, named params, LND compatibility), an exact decimal amount parser replacing floating-point, BIP337 compressed transaction RPCs, a gettxoutproof txindex fast-path, and server/surface/routing infrastructure.
Issues Found
1. handle_json — close flag overwrite (crates/rpc/src/server.rs): response.close = close (assigning the registry-computed blocking flag) uses = instead of |=. While handle_decoded_json always returns close: false today, this unconditionally overwrites any future close: true the inner function might produce.
2. CallResponse::http_status — v2 dispatch error returning -32600 at HTTP 400 (crates/rpc/src/server.rs): The V2 branch in error_status_for_code maps INVALID_REQUEST => 400. A v2 dispatch error that happens to produce code -32600 (e.g., RpcError::InvalidRequest) would get HTTP 400 from the dispatch path rather than 200. Verify this matches the pinned Core 31.1 behavior for such edge cases.
3. optional_max_feerate — negative-check gap (crates/rpc/src/handlers/tx.rs): The three-branch negativity pre-check (as_str, as_raw_number, as_f64) could silently skip the guard for a numeric value not matched by any branch. The sats_from_btc function does handle negative strings internally, but sats_from_btc rejects them as InvalidParams with a generic message rather than the more specific "maxfeerate must be non-negative".
4. txout_typed — ? removal (crates/rpc/src/handlers/tx.rs): script_pub_key_typed was changed to return a plain value (not Result), making the ? removal correct. No issue here — note for reviewers that this was an intentional infallibility change.
The most critical functional concern is item 1 (the |= vs = assignment). The others are either defensive verification requests or minor edge cases.
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.
Compute the close decision once in the decoded-envelope handler so batch and notification drains cannot discard it. Prefer coded statuses before the envelope-error arm and give undecodable bodies no blocking method. Parse a retained block body once, cache its materialized transactions, and charge their bytes to the body budget. Reject P2TR compression when a non-empty scriptSig cannot round-trip.
Replace the observer body-interest callback with BodyInterest across this branch mempool gateway and every call site, so commit reads one atomic before the pool write lock. Add the lock-order regression and the negative-integer feerate pin.
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.
Account for the materialized transaction structure, input and output vectors, script buffers, and per-item witness vectors when deciding body eviction. Keep the serialized body length as the pre-parse cap.
Merges origin/main (46de452: #1684 Tor/I2P overlay connectivity plus #1685 public-listener work) into the 03-rpc lane (registration-time observer interest, ZMQ body interest on rawtx, retained RPC allocations). Listener conflicts (crates/p2p/src/listener.rs, aggregate_budget_tests.rs): resolved by composing main's PeerEndpoint/PeerRemote address generalization (impl Into<PeerRemote> params, into() conversion, PeerEndpoint test addr) with the branch Option/enqueue_owned_body_fetch ownership protocol that the merged tree implements (3-arg send_headers, 3-field InboundHeaders, sync enqueue API). Neither bare side compiles: main's bool signature against the branch body (Some/None returns, if-let-Some caller) is a type error, and the branch signature without main's into() line misuses the generalized generic param. No ListenerRole or rpc-public-bind content exists anywhere in the tree, so nothing of that design was restored. Discarded alternative (documented): restoring main's bool/fetch_issued/record_owned_body_fetch callee end-to-end would require rewriting auto-merged files outside the conflicted set (sync.rs method, send_headers arity, InboundHeaders constructors, staged sync tests that call the enqueue API) and would drop the lane's body-interest plumbing; left for Main to revisit if desired.
|
@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. |
Merges origin/main (fa187f9: #1676 ranged/multipath scantxoutset scans plus #1677 compact-scan bound near the applied tip) into the 03-rpc lane (observer interest, ZMQ body interest, retained RPC allocations, submitpackage). scantxoutset conflicts (chain.rs, util.rs, registry.rs, bip-support.md, external-api.md, rpc-reference.md): branch and main convergently implemented bounded wildcard/multipath expansion, so main's #1676 two-phase implementation wins (count all expansions including duplicates before any public-key derivation, then expand with dedup and the 1 MiB script cap). Adopted main's ScanDescriptor/scan_descriptor/expand/scan_key_script, parse flow, error strings, registry wording, API-35 docs, and proof-test references, all verified present in the merged tree. Deleted the branch 2-arg scan_descriptor shim (name collision with main's 3-arg form) and retargeted its one retained caller plus the branch fixed-form test to scan_descriptor_range, preserving their assertions. rpc-reference.md splits the hunk: main's scantxoutset row with the branch submitpackage rows, which main did not touch and which remain implemented (registry plus handlers/tx/submit_package.rs).
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbc3911b45
ℹ️ 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".
Resolve 6 conflicted paths, preserving the branch RPC surface and main's docs/zmq contract changes: - crates/rpc/README.md, docs/contracts/external-api.md: keep branch JSON-RPC envelope/batch wording, which matches the merged handle_decoded_json (empty batch answers HTTP 200 with []; batch replies always ride HTTP 200). Main's rejection/HTTP-500/max-status wording described pre-branch behavior. - crates/rpc/src/zmq.rs: take main's inline publish_hashtx plus Core comment and assert message (behavior-identical to the branch helper). - crates/rpc/src/context.rs: keep merged top import (BodyInterest already imported once); drop duplicated hunk. - crates/rpc/tests/policy_contract.rs: keep branch nesting; the wants_accepted_transactions override stays gone (method removed from MempoolObserver) per main. - docs/rpc-reference.md: recompute row counts for the merged table (branch moved submitpackage, main moved dumptxoutset out of Unimplemented; branch added 2 Extension rows): Deviation 33, Extension 5, Unimplemented 83, total 178.
…ry inputs context.rs: replace body_tx deep-clone with body_tx_ref borrow plus body_txout TxOut-only clone; resolve_prevout and outpoint_at use it. compressed_tx.rs: build one SighashCache over the immutable skeleton in decompress_finalize and pass it into recover_pubkey; Recovery no longer carries the transaction. Tests: shared-cache two-input (P2PKH plus P2WPKH) recovery and round trip; production-resolver multi-output prevout/outpoint_at coverage.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c8c0a5bb7
ℹ️ 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".
A compressed request cycling five retained block heights thrashed the four-body cache: every input paid a full body read, parse, materialization, and txid pass inside the stable-chainstate exclusion. Root fix, retention unchanged at four bodies: - compressed_tx: resolve flat-output indices (compress) and outpoints (decompress_prepare) grouped by required height in first-appearance order, so each body loads once; per-input error mapping, proved-absent degradation, and cancellation polling preserved. - context: RpcPrevoutResolver enforces cumulative body-load (Loads) and byte (LoadBytes) budgets that fail closed before body I/O/parsing, backstopping height-unknown prevout paths. Regression tests with more than four heights: five-height cycling loads each body once across compress and decompress, and ten distinct heights fail closed with LimitExceeded after bounded work.
…edence (#1712) A submitted member can exceed `maxfeerate` by one satoshi while its displayed integer sat/kvB rate still equals the limit. RPC package underpayment can also be reported as a later cluster/dust failure. Preserve only these remaining fixes from #1667 after the scoped submitpackage implementation landed in #1693: - Compare the current signed `i128` modified fee exactly against `max_rate * vsize / 1000`. The nonnegative `u64 * u32` limit fits `i128`; this avoids signed-fee multiplication overflow and works even when no `u64` rate quote exists. This implements the strict rational comparison for integer fees. - For RPC package submission only, report aggregate `MinRelayFeeNotMet` after TRUC checks and before cluster/ephemeral-spend checks. Other planner errors and native/peer submission keep their existing precedence. This is a fresh adaptation to current main, not a copy of #1667's unsigned/GetFee implementation. Pinned Core 31.1 directly compares `CFeeRate(modified_fee, vsize)` to the caller's rate in [single/package submission](https://github.com/bitcoin/bitcoin/blob/9be056a8a72b624dae9623b2f7bded92c2a21c91/src/validation.cpp#L1251), using [FeeRateCompare cross-products](https://github.com/bitcoin/bitcoin/blob/9be056a8a72b624dae9623b2f7bded92c2a21c91/src/util/feefrac.h#L146). It does not apply GetFee's one-satoshi minimum to this comparison. The [aggregate floor precedes cluster/dust checks](https://github.com/bitcoin/bitcoin/blob/9be056a8a72b624dae9623b2f7bded92c2a21c91/src/validation.cpp#L1350). Existing non-RPC fee-rounding behavior is outside this change. Regression evidence: - Both public regression tests failed before the fix: the 1,341-vB member with modified fee 1,342 sat was accepted under a 1,000-sat/kvB cap; the underfunded pair reported PackageCluster instead of MinRelayFeeNotMet. - New tests cover cap equality and ±1-sat boundaries, a sub-one-satoshi fractional allowance, explicit zero disabling, signed extremes/missing rate quotes, floor versus cluster/dust errors, unchanged capacity-error precedence, and refusal without membership/sequence/estimator/observer changes. - `cargo test -p bitcoin-rs-mempool --lib package`: **44 passed**, including current RPC/native/peer package and snapshot CPFP proofs. - `cargo clippy -p bitcoin-rs-mempool --lib --tests -- -D warnings`: passed. - `cargo fmt --all -- --check` and `git diff --check`: passed. No sync impact: this changes only policy decisions over captured mempool admission facts, before the existing stamped mutation boundary. It does not cross authoritative sync, chain validation, durable chainstate commit, retention, or lock-order boundaries; no SYNC-* clause is changed. No schema, protocol, counter, dependency, or RPC surface expansion. The Core evidence is source comparison plus local behavioral tests; a new Core process campaign was not run.
Draft subsystem stack split from PR #1687.
Base: f024fc3
Tip: 0416119
Full repository gate: native-gate-20261027T201054Z-310371 (scripts/ci-pr.sh all, exit 0).
Scope: RPC.
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 code fixes the earlier cache and ZMQ findings, but the required synchronization-impact declaration must be added before merging.
Findings
Summary
This stack adds optional BIP337 transaction compression, scoped
submitpackage, ranged descriptor scans, RPC compatibility updates, and ordered ZMQ admission notices.scantxoutsetaccepts ranged descriptors and keeps its scan work bounded.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Compressed transaction RPC] --> B[Capture stable chain view] B --> C[Parse bytes and resolve spent outputs] C --> D[Release chain lock] D --> E[Recover signatures] E --> F[Recheck chain view and return]Reviews (5) · Last reviewed commit: "Charge retained RPC transaction allocati..." · Reviewed by Greptile
Sync impact: YES.
enqueue_owned_body_fetchchanges how block sync sends and records body requests. Affected clauses: SYNC-04 (recovery closure) and SYNC-06 (work drivers). Smallest proof:crates/p2p/src/sync/tests.rsnotfound and owned-body-fetch regressions.