rpc: Core-compatible REST gateway so a remote bip300301_enforcer can use bitcoin-rs as its mainchain node - #49
Conversation
Original prompt from a
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds an optional Bitcoin Core-compatible REST gateway. Configuration supports ChangesREST gateway
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rpc/src/rest.rs`:
- Around line 92-101: Update header_records in crates/rpc/src/rest.rs:92-101 so
side-branch hashes where active_hash != Some(hash) return the Core-compatible
empty result instead of the requested header, and add a regression test covering
that case. Update docs/rest-interface.md:23-25 to remove the statement that
side-branch hashes return their own header.
In `@crates/rpc/src/server.rs`:
- Around line 209-228: Update the request parsing flow around request_target and
keep_alive to parse and validate the HTTP version, then derive persistence
according to HTTP semantics: HTTP/1.1 stays open by default unless Connection:
close is present, while explicit Connection: keep-alive enables persistence
where applicable. Add a socket test sending two HTTP/1.1 GET requests without a
Connection header and verify both are handled on the same connection.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42695073-2811-48a0-8e19-1e1beb57bab7
📒 Files selected for processing (11)
CONCEPTS.mdcrates/node/src/bitcoin_conf_compat.rscrates/node/src/config.rscrates/node/src/run.rscrates/rpc/src/handlers.rscrates/rpc/src/lib.rscrates/rpc/src/rest.rscrates/rpc/src/server.rscrates/rpc/tests/auth.rsdocs/README.mddocs/rest-interface.md
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: test
- GitHub Check: bench-smoke
🧰 Additional context used
🔍 Remote MCP Github Grep
Relevant compatibility context
- Bitcoin Core’s current REST docs use
GET /rest/headers/<BLOCK-HASH>.<bin|hex|json>?count=<COUNT>. The PR implements the older/rest/headers/<COUNT>/<BLOCK-HASH>...form, which Bitcoin Core documents as deprecated since v24.0. Verify this is the exact format required bybip300301_enforcer. - Bitcoin Core documents that header queries return empty when the hash is not on the active chain. The PR summary says side-branch hashes return their own header, which differs from Core behavior and may affect compatibility.
- Core’s REST documentation states
/rest/chaininfo.jsonsupports JSON output only, matching the PR’s endpoint scope.
🔇 Additional comments (11)
crates/node/src/bitcoin_conf_compat.rs (1)
57-57: LGTM!Also applies to: 143-145
crates/node/src/config.rs (1)
97-98: LGTM!Also applies to: 176-176, 237-237, 447-449, 576-578, 669-669
crates/node/src/run.rs (1)
629-629: LGTM!crates/rpc/src/handlers.rs (1)
32-36: LGTM!crates/rpc/src/lib.rs (1)
22-23: LGTM!crates/rpc/src/rest.rs (1)
1-90: LGTM!Also applies to: 111-203
crates/rpc/src/server.rs (1)
31-32: LGTM!Also applies to: 43-52, 116-167, 271-289, 344-365, 385-405
crates/rpc/tests/auth.rs (1)
5-70: LGTM!Also applies to: 113-197
CONCEPTS.md (1)
5-11: LGTM!docs/README.md (1)
10-11: LGTM!docs/rest-interface.md (1)
1-22: LGTM!Also applies to: 27-37
| let request_target = request_line.trim_end_matches(['\r', '\n']); | ||
| let mut request_parts = request_target.split_whitespace(); | ||
| let Some(method) = request_parts.next() else { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "invalid request line", | ||
| )); | ||
| }; | ||
| let Some(path) = request_parts.next() else { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "invalid request line", | ||
| )); | ||
| }; | ||
| if !matches!(method, "POST" | "GET") { | ||
| return Err(io::Error::new( | ||
| io::ErrorKind::InvalidData, | ||
| "invalid request method", | ||
| )); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Honor HTTP/1.1 persistent connections by default.
Line 232 defaults keep_alive to false. Line 267 enables it only for Connection: keep-alive. HTTP/1.1 keeps the connection open unless Connection: close is present.
Parse and validate the HTTP version. Derive persistence from the version and Connection header. Add a socket test with two HTTP/1.1 GET requests that omit Connection.
Also applies to: 232-232, 266-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/rpc/src/server.rs` around lines 209 - 228, Update the request parsing
flow around request_target and keep_alive to parse and validate the HTTP
version, then derive persistence according to HTTP semantics: HTTP/1.1 stays
open by default unless Connection: close is present, while explicit Connection:
keep-alive enables persistence where applicable. Add a socket test sending two
HTTP/1.1 GET requests without a Connection header and verify both are handled on
the same connection.
There was a problem hiding this comment.
Not taking this here. Requiring an explicit Connection: keep-alive is the server's pre-existing behavior for JSON-RPC POST; this PR only lets GET inherit it. Making HTTP/1.1 persistence the default means changing the shared request parser, which changes behavior for every existing JSON-RPC client, so it belongs in its own change rather than riding along with a REST addition.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
|
@codex review |
![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: f3b17e568a
ℹ️ 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".
| for height in start.height..=start.height.saturating_add(count.saturating_sub(1)) { | ||
| let Some(record) = ctx.block_by_height(height) else { | ||
| break; |
There was a problem hiding this comment.
If an active-chain reorg occurs after the initial block_by_height(start.height) check, each subsequent call reloads the current applied tip independently. The loop can therefore return a replacement-branch record at the starting height instead of the requested hash, or splice records from different branches into one response, causing the enforcer to receive a non-contiguous header sequence. Traverse under one tip/tree snapshot or verify every returned header links to the preceding record.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixing. The walk re-reads the active chain per height, so a reorg landing mid-walk can splice records from two branches into one response. Adding linkage verification: each subsequent record's previousblockhash must equal the previous record's hash, and the walk stops at the first break rather than emitting a spliced chain.
| "BITCOIN_RS_DATA_DIR" => layer.data_dir = Some(PathBuf::from(value)), | ||
| "BITCOIN_RS_STORAGE_BACKEND" => layer.storage_backend = Some(value.to_owned()), | ||
| "BITCOIN_RS_RPC_BIND" => layer.rpc_bind = Some(value.parse()?), | ||
| "BITCOIN_RS_REST" => layer.rest = Some(value.parse()?), |
There was a problem hiding this comment.
When REST is enabled through the environment using the Core-style value BITCOIN_RS_REST=1 (or the project's supported yes/on forms), bool::from_str rejects the value and node startup fails. The adjacent boolean environment settings use parse_bool, which explicitly accepts these forms; REST should use the same parser so layered configuration behaves consistently.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already fixed in dae9f8f — BITCOIN_RS_REST now goes through the same parse_bool helper as its sibling boolean env vars, so 1/0/yes/no work. This one was not theoretical: BITCOIN_RS_REST=1 aborted the node at startup, which a runtime run against a regtest node caught.
Runtime verification — REST gateway on a live regtest nodeBuilt with Actual responses (unauthenticated)
The run found one real bug and several Core divergences, all fixed in Pre-existing, out of scope here: |
|
@devin fix that as another pr |
|
@codex review |
|
@coderabbitai ultrareview |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/rpc/src/rest.rs`:
- Around line 61-67: Update route_headers to validate the file extension, then
call parse_count before Hash256::from_str so invalid count responses take
precedence when both inputs are invalid. Preserve the existing hash validation
afterward, and add a regression test covering an invalid hash with an invalid
count.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4a191d2-0ac9-4131-bab6-c8a2c1dac0b7
📒 Files selected for processing (11)
CONCEPTS.mdcrates/node/src/bitcoin_conf_compat.rscrates/node/src/config.rscrates/node/src/run.rscrates/rpc/src/handlers.rscrates/rpc/src/lib.rscrates/rpc/src/rest.rscrates/rpc/src/server.rscrates/rpc/tests/auth.rsdocs/README.mddocs/rest-interface.md
📜 Review details
🧰 Additional context used
🔍 Remote MCP Github Grep
Relevant review context
- Bitcoin Core’s current REST implementation uses
count=5by default and rejects values outside1..=2000with HTTP 400, matching the PR’s documented header semantics. - Core accepts the modern
/rest/headers/<hash>.<ext>?count=<count>form and also retains a deprecated/rest/headers/<count>/<hash>form. The PR summary only mentions the modern form; compatibility with the deprecated form is not established. - Core validates the hash after count validation and returns HTTP 400 for malformed hashes.
- Bitcoin Core registers
-zmqpubsequence=<address>and creates a correspondingpubsequencenotifier, supporting the PR’s ZMQ documentation. - Multiple clients independently consume
/rest/chaininfo.json, confirming the endpoint path is commonly relied upon.
🔇 Additional comments (11)
crates/node/src/bitcoin_conf_compat.rs (1)
57-57: LGTM!Also applies to: 143-145
crates/node/src/config.rs (1)
97-98: LGTM!Also applies to: 176-176, 237-237, 434-434, 448-450, 577-579, 670-670
crates/node/src/run.rs (1)
629-629: LGTM!crates/rpc/src/handlers.rs (1)
32-37: LGTM!crates/rpc/src/lib.rs (1)
22-23: LGTM!crates/rpc/src/rest.rs (1)
1-60: LGTM!Also applies to: 69-489
crates/rpc/src/server.rs (1)
31-32: LGTM!Also applies to: 43-51, 116-122, 136-136, 153-181, 189-190, 202-228, 271-285, 344-366, 385-405
crates/rpc/tests/auth.rs (1)
5-12: LGTM!Also applies to: 29-88, 114-120, 131-137, 139-197
CONCEPTS.md (1)
5-11: LGTM!docs/README.md (1)
10-11: LGTM!docs/rest-interface.md (1)
1-47: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b999e8136
ℹ️ 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".
| for height in | ||
| start_height.saturating_add(1)..=start_height.saturating_add(count.saturating_sub(1)) | ||
| { | ||
| let Some(record) = ctx.block_by_height(height) else { |
There was a problem hiding this comment.
When a node has accumulated many applied records and a client requests a large header batch (up to the supported count=2000), every block_by_height call reaches record_for_hash, which linearly scans Context::blocks for the matching payload. Since apply appends one record per block, serving headers near a million-block tip becomes O(chain length × count)—potentially billions of comparisons per unauthenticated request—so the advertised enforcer sync can time out and consume the bounded RPC workers. Read the headers directly from one block-tree/applied-tip snapshot or otherwise index cache records by hash instead of repeating this scan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, and this is worth more than the traversal itself - thanks. block_by_height goes through record_for_hash, which linearly scans the push-only Context::blocks vector (one entry per applied block, never trimmed) and can additionally hit block_body_metadata on disk, so count=2000 is up to 2000 x O(chain length) comparisons plus disk lookups per unauthenticated request, on the bounded RPC worker pool.
Fixing it by serving headers from the block tree - which is where headers actually live - under a single block_tree read lock: resolve the requested hash to a node, decide active-chain membership from the tip in that same snapshot, then resolve the last wanted height once and collect its ancestors back to the start via parent pointers rather than doing a per-height lookup in a loop (node_at_height_from walks back from the tip when the active-height index isn't trusted, which would just move the O(depth) cost). The block-record cache is then only consulted for the cache-only fallback, which is one lookup rather than count of them.
A side effect worth noting: one ancestor walk inside one locked snapshot is structurally single-branch, so the explicit prev_blockhash linkage check added in a2bc4ef for the earlier multi-snapshot traversal becomes redundant. Observable behavior is unchanged - side-branch hash returns its own header, unknown-but-well-formed hash still returns the empty 200, and the JSON field set stays exactly the eight fields the enforcer reads.
Runtime re-verification against Bitcoin Core 27.0 (regtest, 6100→6102 blocks)Rebuilt the reference environment from scratch: Core 27.0 regtest peer with Header serving after the tree-snapshot rewrite — 0 diffs vs Core, and bounded by the applied tip
Mid-sync (headers arrive before blocks are applied), with each request bracketed by a chaininfo read before and after so the concurrently-advancing tip cannot produce a false result: With 6100 headers known but only ~512–1152 blocks applied, Error precedence, count range, query params
Latency and reorgPer-request on one keep-alive connection, 6100-block chain: (The ~44 ms floor on small bodies is TCP delayed-ACK; a new connection costs a constant ~96 ms for every endpoint including POST JSON-RPC, which is pre-existing and unrelated to REST.) Reorg via No duplicated or spliced heights. The run isolated four remaining Core divergences, all now being fixed in this PR: a bare Not verified: non-regtest networks, TLS/proxy fronting, concurrent load. |
|
@devin Should support ZMQ too. See https://github.com/LayerTwo-Labs/bip300301_enforcer and get full compatibility. Do your best. |
|
@devin Be modular. Don't naively copy bitcoin-core's full surface. |
|
On it. Mapping exactly what the enforcer subscribes to and what our publisher already emits before I write anything — I'll come back with the gap and the plan rather than starting from Core's ZMQ surface. Doing it as its own PR so this one stays a REST change. |
|
Agreed — the target is the enforcer's actual consumers, not Core's notification surface. Concretely that means the topics it subscribes to and the frame layout it parses, behind our existing publisher configuration, with nothing added just because bitcoind has it. Same principle as this PR, where |
|
@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". |
1 participant
Summary
Adds the minimal Bitcoin Core-compatible REST surface an unmodified
bip300301_enforcerconsumes, so it can be pointed at bitcoin-rs instead of bitcoind. No BIP300/301 consensus logic is ported.The enforcer's REST client dictates the shape: it builds its base URL as
http://{--node-rpc-addr}(app/main.rs), sends noAuthorizationheader, and only callsGET /rest/chaininfo.jsonandGET /rest/headers/{hash}.json?count=N(lib/validator/main_rest_client.rs); it reads any 404 on/rest/*as "REST not enabled". So REST is served on the existing JSON-RPC listener/port, unauthenticated, off by default behind Core'srest=1— not a second listener — and 404 is reserved for exactly the cases that detection is meant to catch (gateway disabled, unknown route, missing output format).Endpoints:
chaininfo.json(reusesgetblockchaininfo),headers/{hash}.{json,hex,bin}. Status semantics follow Core:countdefaults to 5 and must be in1..=2000(out of range → 400, no silent clamping), unknown query parameters and valueless keys are ignored, a well-formed hash that is not on the active chain →200with an empty body, a bad hash or unsupported extension →400 Invalid hash: <hash>(hash only, no extension), a missing extension →404 output format not found. Header JSON fields match the enforcer's deserializer, includingbitsas unprefixed hex soCompactTarget::from_unprefixed_hexround-trips./rest/block/*,/rest/tx/*,/rest/mempool/*,/rest/blockhashbyheight/*are deliberately not implemented — the enforcer gets raw blocks viagetblockverbosity 0 over JSON-RPC.Server change is confined to
serve_connection, which previously hard-rejected any non-POSTrequest line:Header traversal mirrors Core's
rest_headers, whose loop condition isActiveChain().Contains(pindex): the requested hash must be positively on the applied chain, otherwise the result is the empty200. That covers side branches, orphans, and headers already known to the tree but not yet applied — during header-first sync a header above the applied tip yields[], and a walk that reaches the applied tip stops there rather than continuing up the header chain. Oneblock_treeread snapshot per request serves the whole walk via parent pointers (resolve the hash once, resolve the terminal height once, walk ancestors, reverse), so a largecountno longer costs a fullContext::blocksscan per header —count=2000on a 6100-block chain went from seconds to ~5 ms — and the walk cannot splice records from two branches mid-reorg. The linkage check is kept as a defense against an internally inconsistent tree and truncates the tail at the first broken link.Two known gaps for enforcer operators, documented in
docs/rest-interface.mdrather than papered over:getnetworkinforeports version10000(major 1) which the enforcer's version gate rejects, so it must run with--bitcoin-core-skip-version-check; and bitcoin-rs publishes no ZMQpubsequencetopic, so the enforcer's mempool sync needs an explicit--node-zmq-addr-sequenceor a no-mempool/bounded mode.pubsequencecan follow separately.Coverage: unit tests for routing/count boundaries/error precedence (
countis validated before the hash, as Core does)/active-chain rules/JSON field shapes, socket-level tests incrates/rpc/tests/auth.rs(REST 200 without credentials, 404 when disabled, POST JSON-RPC still 401/200 with and without auth, non-/rest/GET → 404, keep-alive serving two REST requests on one connection), plus two rounds of runtime verification against a live regtest node peered to Bitcoin Core 27.0 (see PR comments) which is what surfaced theBITCOIN_RS_REST=1env-parsing crash, the header-cache scan cost, the applied-vs-header tip boundary, and the Core status/message divergences fixed here.Link to Devin session: https://app.devin.ai/sessions/2cf54a23e1494fd080fa7541dadf06ff
Requested by: @metaphorics