Feature/drynet sync - #73
Conversation
- Introduced `p2p_magic` field in `Config` for P2P message-start overrides. - Implemented `invalidate_block` function in `reorg.rs` to manage chain state. - Updated RPC handlers to support `invalidateblock` method. - Added tests for P2P magic overrides and validation requirements.
…egration - Implement `invalidateblock` functionality in RPC and related tests. - Update Docker configuration for enforcer service and networking. - Add new environment variables for REST and ZMQ settings. - Modify `.env.example` and `.gitignore` for new paths and configurations. - Document changes in `CONCEPTS.md` and `rest-interface.md`.
…tation - Refactor .env.example to set drynet4 as the default network. - Update CONCEPTS.md and README.md to clarify drynet4 network settings and behavior. - Implement network selection logic in config.rs to handle drynet4 specifics. - Add integration tests for drynet4 network configuration. - Remove outdated enforcer Dockerfile and docker-compose files; replace with updated versions for drynet4. - Introduce new documentation on network selection and P2P identity management.
|
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
WalkthroughThis PR adds Drynet4 and configurable P2P identity handling, node-owned block invalidation through RPC, version-aware JSON-RPC responses, and a BIP300/301 enforcer Compose deployment. It also updates deployment documentation and persistent data handling. ChangesNode and deployment integration
Sequence Diagram(s)sequenceDiagram
participant RPCClient
participant RPCHandler
participant RpcChainControl
participant ReorgSubsystem
participant BlockTree
RPCClient->>RPCHandler: invalidateblock(hash)
RPCHandler->>RpcChainControl: invalidate_block(hash)
RpcChainControl->>ReorgSubsystem: invalidate_block(hash)
ReorgSubsystem->>BlockTree: tip_after_invalidation(root)
ReorgSubsystem->>ReorgSubsystem: execute disconnect/connect plan
ReorgSubsystem-->>RPCHandler: success or mapped error
RPCHandler-->>RPCClient: null or JSON-RPC error
Possibly related PRs
🚥 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: 6
🧹 Nitpick comments (7)
crates/rpc/tests/handler_smoke.rs (1)
133-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo of the four
invalidateblockbranches are untested.You cover success and
UnknownBlock. You skippedChainControlError::Genesisand thechain_control == Nonepath that returnsMethodDisabled. Those are exactly the branches an operator hits by accident, and the stub you already wrote makes both tests three lines each.♻️ Proposed additional tests
#[test] fn invalidateblock_rejects_genesis() { let ctx = Context::new().with_chain_control(Arc::new(RecordingChainControl { called: Arc::new(AtomicBool::new(false)), result: Err(ChainControlError::Genesis), })); let handler = Handler::new(Arc::new(ctx)); let hash = Hash256::from_le_bytes(&[9_u8; 32]).to_string_be(); handler .dispatch("invalidateblock", &json!([hash])) .expect_err("genesis must fail"); } #[test] fn invalidateblock_without_chain_control_is_disabled() { let handler = Handler::new(Arc::new(Context::new())); let hash = Hash256::from_le_bytes(&[9_u8; 32]).to_string_be(); handler .dispatch("invalidateblock", &json!([hash])) .expect_err("missing chain control must fail"); }🤖 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/tests/handler_smoke.rs` around lines 133 - 178, Add tests covering the remaining invalidateblock branches: use RecordingChainControl with ChainControlError::Genesis and assert dispatch fails, then create a Handler from Context::new() without chain control and assert dispatch fails with MethodDisabled. Follow the existing hash construction and dispatch patterns in the invalidateblock tests.crates/rpc/src/handlers.rs (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
invalidateblockships withoutreconsiderblock.Core always exposes the pair. You just gave operators a one-way door: invalidate a block by mistake and the only recovery is nuking chain state and resyncing. That is a lousy deal for a consensus-affecting RPC.
Add a
reconsiderblockarm and the matchingChainControl::reconsider_blockmethod, or document loudly that invalidation is permanent for this node.Do you want me to open an issue to track
reconsiderblock?🤖 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/handlers.rs` at line 56, Add the missing reconsiderblock RPC dispatch alongside invalidateblock in the handler match, and implement the corresponding ChainControl::reconsider_block method using the existing chain-control and RPC patterns. Ensure operators can reverse a prior block invalidation without resetting or resynchronizing chain state.crates/node/tests/config_layered.rs (1)
490-499: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer a literal address over
localhost.
localhostresolution depends on the host's/etc/hostsand resolver configuration, and it can return an IPv6 address first. The assertions survive that, but the lookup itself is avoidable. Use127.0.0.1:18444if the intent is only to prove that the environment layer parses a peer list.If the intent is specifically to prove hostname resolution, say so in a comment so nobody "simplifies" it later.
🤖 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/node/tests/config_layered.rs` around lines 490 - 499, Update the BITCOIN_RS_CONNECT value in the Config::from_layered_sources test to use the literal address 127.0.0.1:18444, preserving the existing assertions. Only retain localhost if the test explicitly documents that it is validating hostname resolution.crates/node/src/run.rs (1)
79-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFlattening
Fatalinto a generic string throws away the only signal that matters.
ReorgError::FatalandMarkerStuck-driven failures mean the chainstate is torn and apply admission was closed permanently. Here they collapse intoChainControlError::Failed(other.to_string()), indistinguishable from a routineMissingBody. The operator gets one RPC error string and no log line saying the node just stopped accepting blocks.Log the error before mapping, at a level that matches the damage.
🩺 Log before mapping
crate::reorg::invalidate_block(&self.handles, hash).map_err(|error| match error { crate::reorg::ReorgError::UnknownBlock(_) => { bitcoin_rs_rpc::ChainControlError::UnknownBlock } crate::reorg::ReorgError::CannotInvalidateGenesis => { bitcoin_rs_rpc::ChainControlError::Genesis } + other @ crate::reorg::ReorgError::Fatal(_) => { + tracing::error!(%other, "invalidateblock left the chainstate inconsistent"); + bitcoin_rs_rpc::ChainControlError::Failed(other.to_string()) + } other => { + tracing::warn!(%other, "invalidateblock failed"); bitcoin_rs_rpc::ChainControlError::Failed(other.to_string()) } })🤖 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/node/src/run.rs` around lines 79 - 87, Update the error handling around crate::reorg::invalidate_block in the enclosing method to log failures before mapping them, using an error-level log for ReorgError::Fatal and MarkerStuck-driven failures to clearly signal that apply admission is permanently closed, while retaining appropriate lower severity for routine errors. Preserve the existing ChainControlError mapping after logging.crates/node/src/apply.rs (2)
9146-9152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis assertion cannot fail for the reason you want it to fail.
The
recv_timeout(100ms)returningTimeoutproves only that the contender did not report within 100 ms. It cannot distinguish "blocked on the chain transition" from "scheduled late". The negative direction is safe, so the test never flakes, but it also passes if the serialization guarantee is removed and the contender is merely slow.Strengthen it: have the contender record a timestamp when it acquires the transition, and assert that timestamp is after the barrier release. That makes the ordering claim, rather than a scheduling guess.
🤖 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/node/src/apply.rs` around lines 9146 - 9152, Strengthen the concurrency test around the acquired_rx assertion by having the competing transition record its acquisition timestamp, then release the preload barrier and assert that acquisition occurred afterward. Replace the recv_timeout-based absence check with this explicit ordering validation, while preserving the test’s existing transition setup and synchronization.
9063-9095: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winA
Barrierwith no timeout turns a regression into a hung CI job.
BlockingBodyStore::load_block_bodyblocks onentered.wait()only whenblock_onceis still true. The test thread then callsstore.entered.wait()at Line 9131. If a future change makesinvalidate_blockserve the disconnect body from somewhere other than the body store,load_block_bodyis never called, and both threads park on the barrier forever.std::sync::Barrierhas no timeout, so the failure mode is a stuck job, not a red test.Use a channel pair with
recv_timeouton the test side so the same regression fails loudly.Also applies to: 9128-9131
🤖 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/node/src/apply.rs` around lines 9063 - 9095, Replace the Barrier-based coordination in BlockingBodyStore and its test with a channel pair: have load_block_body notify the test when entered and wait for the release signal, while the test-side wait uses recv_timeout. Preserve the one-time blocking behavior controlled by block_once, and make a missing body-store call fail promptly instead of hanging.crates/chain/src/tree.rs (1)
675-707: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffRebuilding the entire child adjacency on every plan call is lazy.
invalidation_planallocatesVec<Vec<NodeId>>with oneVecper slab slot, then avec![false; node_count], on every call. On a mainnet-sized tree that is roughly a million heap headers per invocation. Andinvalidate_blockincrates/node/src/reorg.rscalls this three times per attempt:tip_after_invalidationunder the read lock, again under the write lock, theninvalidate_subtree.Nobody invalidates blocks in a loop, so this is not a fire. It is still three full O(n) allocations to answer "which subtree hangs off this node", and the second call happens while holding the block-tree write lock.
Use a flat CSR-style adjacency (one counts pass, one offsets array, one children array), or return the plan from the preview and let
invalidate_subtreeaccept it.🤖 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/chain/src/tree.rs` around lines 675 - 707, Refactor invalidation_plan so it no longer allocates a Vec<Vec<NodeId>> and visited bitmap for every call, and avoid rebuilding the subtree plan three times from invalidate_block. Prefer a flat CSR-style child adjacency using a counting pass, offsets, and one children array, or reuse the preview plan by passing it into invalidate_subtree; preserve the existing invalidation result and root handling.
🔇 Additional comments (24)
.gitignore (1)
17-18: LGTM!README.md (1)
39-58: LGTM!Also applies to: 72-75
docs/rest-interface.md (1)
57-61: LGTM!docs/solutions/architecture-patterns/network-selection-keeps-p2p-identity-atomic.md (1)
1-41: LGTM!tools/bip300301-enforcer/.env.example (1)
1-2: LGTM!Also applies to: 15-16
tools/bip300301-enforcer/Dockerfile.enforcer (1)
1-39: LGTM!tools/bip300301-enforcer/docker-compose.yaml (1)
73-76: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Send a valid RPC request in the enforcer health check.
This command sends an empty POST with no
Content-Type. It does not create a validGetChainInforequest. Generated Connect handlers support JSON requests, but require a declared codec and request payload. (pkg.go.dev)Send
Content-Type: application/jsonand{}. Otherwise, the container can remain unhealthy after a successful startup.Proposed fix
curl --fail --silent --show-error -X POST -H 'Content-Type: application/json' + --data '{}' http://127.0.0.1:50051/cusf.mainchain.v1.ValidatorService/GetChainInfocrates/rpc/src/context.rs (1)
222-244: LGTM!Also applies to: 298-299, 379-379, 446-446, 466-472
crates/rpc/src/lib.rs (1)
29-30: LGTM!crates/rpc/src/handlers/chain.rs (1)
12-12: LGTM!Also applies to: 590-604
crates/rpc/src/server.rs (1)
8-8: LGTM!Also applies to: 147-149, 303-382, 432-472, 569-644
crates/rpc/tests/auth.rs (1)
13-13: LGTM!Also applies to: 29-126
crates/node/src/config.rs (3)
19-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.The preset enum is fine. The TOML alias surface is not.
NetworkSelectionderivesDeserializewithrename_all = "lowercase", butConfigLayer.network(Line 672) does not use a custom deserializer. So a TOML file gets exactlymainnet|testnet3|testnet4|signet|regtest|drynet4, while CLI and environment go throughparse_network_selectionand also acceptmain,bitcoin,test, andtestnet. Two parsers, two vocabularies, one config key. That is how you get bug reports.Either drop the aliases or wire the TOML field through the same parser.
♻️ Route TOML through the single parser
/// Select the Bitcoin or fork network, including its P2P bootstrap profile. #[arg(long, value_parser = parse_network_selection)] + #[serde(default, deserialize_with = "deserialize_optional_network_selection")] pub(crate) network: Option<NetworkSelection>,fn deserialize_optional_network_selection<'de, D>( deserializer: D, ) -> core::result::Result<Option<NetworkSelection>, D::Error> where D: serde::Deserializer<'de>, { let raw = Option::<String>::deserialize(deserializer)?; raw.as_deref() .map(parse_network_selection) .transpose() .map_err(serde::de::Error::custom) }
674-676: 🎯 Functional Correctness | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that a TOML file without
p2p_magicstill parses.
deserialize_withon a field disables serde's implicit "missing field meansNone" behaviour forOption<T>. Without#[serde(default)]on the field or on the container, serde's generated code returns a missing-field error for every TOML config that omitsp2p_magic. That would break every existing config file, which is a somewhat conspicuous regression.The
ConfigLayerstruct attributes are not in the provided range, so confirm the container carries#[serde(default)].🛡️ Field-level guard if the container has no default
/// Override the four P2P message-start bytes for a fork network. #[arg(long = "p2p-magic", value_parser = parse_p2p_magic)] - #[serde(deserialize_with = "deserialize_optional_p2p_magic")] + #[serde(default, deserialize_with = "deserialize_optional_p2p_magic")] pub(crate) p2p_magic: Option<[u8; 4]>,Also applies to: 1009-1021
370-383: LGTM!Also applies to: 435-439, 952-964
crates/node/src/bitcoin_conf_compat.rs (1)
13-13: LGTM!crates/node/tests/config_layered.rs (1)
99-122: LGTM!Also applies to: 158-190
crates/node/src/run.rs (1)
199-199: LGTM!Also applies to: 285-285, 643-645
crates/chain/src/tree.rs (1)
630-673: LGTM!Also applies to: 1922-1926
crates/node/src/reorg.rs (2)
66-75: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
⚠️ Unverified finding
Sandbox verification was unavailable.You discard the invalidated hashes and the connected count. Both exist for a reason.
BlockTree::invalidate_subtreedocuments its return value explicitly: "callers use them to purge bounded body and download state after releasing their chain-transition witness." Line 66 throws thatVec<Hash256>away. Line 74 throws away the committed-connect count thatswitch_to_branchfeeds to itsconnected_bodycallback to retire staging and download-window entries.So after an RPC
invalidateblock, the stager and the download window still own entries for a subtree the node just declared invalid.ConnectFailedin this same file goes to the trouble of plumbinginvalidatedout for exactly this cleanup, and the new path silently skips it. Either purge here or return the hashes soRpcChainControlcan.🧹 Keep the cleanup inputs
- tree.invalidate_subtree(root).map_err(ReorgError::Plan)?; + let invalidated = tree.invalidate_subtree(root).map_err(ReorgError::Plan)?; let tip = tree.tip().ok_or(ReorgError::NoValidTip)?; handles.chain_tip.store(Some(tip.clone())); handles.assume_valid_gate.evaluate(&tree); - tip.tip_id + (tip.tip_id, invalidated) }; - debug_assert_eq!(published_target, target); - - let (_, outcome) = execute_loaded_plan(handles, &disconnect, &connect, &transition); - return outcome; + let (published_target, invalidated) = published_target; + debug_assert_eq!(published_target, target); + + let (connected, outcome) = execute_loaded_plan(handles, &disconnect, &connect, &transition); + drop(transition); + // Purge staged bodies and download ownership for the invalid subtree, and + // retire the entries for every connect that committed. + purge_invalidated(handles, &invalidated); + for body in &connect[..connected] { + retire_connected(handles, body.hash); + } + return outcome;
85-93: LGTM!Also applies to: 267-278, 280-335
crates/node/src/apply.rs (1)
8926-8982: LGTM!Also applies to: 8984-9028, 9030-9061
docs/solutions/architecture-patterns/node-reorg-execution-design.md (1)
40-45: LGTM!Also applies to: 158-158
CONCEPTS.md (1)
40-41: LGTM!Also applies to: 183-196
🤖 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/node/src/config.rs`:
- Around line 626-641: The network-selection behavior must be documented and
confirmed as intentional: update apply_network_selection in
crates/node/src/config.rs to preserve its current reset behavior, then update
CONCEPTS.md lines 42-43 to define “later overrides” as the same or a
higher-precedence layer and list rpc_bind, p2p_listen, dns_seeds_enabled,
connect, and p2p_magic as keys reset by a selection.
- Around line 905-911: Make connect configuration retain the unresolved peer
host and defer DNS resolution to the retryable P2P dial path: update
parse_connect_addr in crates/node/src/config.rs#L905-L911, store the
DRYNET4_CONNECT host string at crates/node/src/config.rs#L638, and remove the
network-resolving value_parser from the connect argument at
crates/node/src/config.rs#L704-L708. Update
crates/node/tests/config_layered.rs#L124-L156 to assert the stored host string
so tests run without DNS access.
In `@crates/node/src/reorg.rs`:
- Around line 27-45: Bound the retry loop in the reorganization flow around
begin_chain_transition and current_reorg_plan with a finite attempt count,
returning a dedicated ReorgError variant such as RaceRetriesExhausted when the
limit is reached. Preserve the existing retry behavior for transient root/target
races, but ensure exhaustion exits the RPC instead of looping indefinitely.
In `@crates/rpc/src/server.rs`:
- Around line 182-192: Replace the no-body branch in the response handling flow
with a dedicated no-content writer that emits only the 204 status, Connection
header, and terminating blank line, preserving keep-alive behavior and flushing
the stream. Do not use write_status for 204 responses; add or update the
relevant auth.rs tests to verify content-length and content-type headers are
absent.
- Around line 346-351: Update the error-status logic around error_status to
accept the RPC error code and map RPC_METHOD_NOT_FOUND to 404,
RPC_INVALID_REQUEST to 400, and all other legacy errors to 500. Ensure batch
responses with JSON bodies, including error-containing and empty batches,
consistently return HTTP 200, while preserving the existing V2 behavior.
In `@tools/bip300301-enforcer/docker-compose.yaml`:
- Around line 15-16: Remove the default “password” fallback from
BITCOIN_RS_RPC_PASSWORD in the Compose environment and enforcer command,
requiring the variable during interpolation while leaving BITCOIN_RS_RPC_USER
behavior unchanged.
---
Nitpick comments:
In `@crates/chain/src/tree.rs`:
- Around line 675-707: Refactor invalidation_plan so it no longer allocates a
Vec<Vec<NodeId>> and visited bitmap for every call, and avoid rebuilding the
subtree plan three times from invalidate_block. Prefer a flat CSR-style child
adjacency using a counting pass, offsets, and one children array, or reuse the
preview plan by passing it into invalidate_subtree; preserve the existing
invalidation result and root handling.
In `@crates/node/src/apply.rs`:
- Around line 9146-9152: Strengthen the concurrency test around the acquired_rx
assertion by having the competing transition record its acquisition timestamp,
then release the preload barrier and assert that acquisition occurred afterward.
Replace the recv_timeout-based absence check with this explicit ordering
validation, while preserving the test’s existing transition setup and
synchronization.
- Around line 9063-9095: Replace the Barrier-based coordination in
BlockingBodyStore and its test with a channel pair: have load_block_body notify
the test when entered and wait for the release signal, while the test-side wait
uses recv_timeout. Preserve the one-time blocking behavior controlled by
block_once, and make a missing body-store call fail promptly instead of hanging.
In `@crates/node/src/run.rs`:
- Around line 79-87: Update the error handling around
crate::reorg::invalidate_block in the enclosing method to log failures before
mapping them, using an error-level log for ReorgError::Fatal and
MarkerStuck-driven failures to clearly signal that apply admission is
permanently closed, while retaining appropriate lower severity for routine
errors. Preserve the existing ChainControlError mapping after logging.
In `@crates/node/tests/config_layered.rs`:
- Around line 490-499: Update the BITCOIN_RS_CONNECT value in the
Config::from_layered_sources test to use the literal address 127.0.0.1:18444,
preserving the existing assertions. Only retain localhost if the test explicitly
documents that it is validating hostname resolution.
In `@crates/rpc/src/handlers.rs`:
- Line 56: Add the missing reconsiderblock RPC dispatch alongside
invalidateblock in the handler match, and implement the corresponding
ChainControl::reconsider_block method using the existing chain-control and RPC
patterns. Ensure operators can reverse a prior block invalidation without
resetting or resynchronizing chain state.
In `@crates/rpc/tests/handler_smoke.rs`:
- Around line 133-178: Add tests covering the remaining invalidateblock
branches: use RecordingChainControl with ChainControlError::Genesis and assert
dispatch fails, then create a Handler from Context::new() without chain control
and assert dispatch fails with MethodDisabled. Follow the existing hash
construction and dispatch patterns in the invalidateblock tests.
🪄 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: 6c4425a0-aaa3-4c68-8b0e-adf9fb6ae2c9
📒 Files selected for processing (26)
.gitignoreCONCEPTS.mdREADME.mdcrash-1efd99623c772a1dd3aca1178fdafc93c933dec0crates/chain/src/tree.rscrates/node/src/apply.rscrates/node/src/bitcoin_conf_compat.rscrates/node/src/config.rscrates/node/src/reorg.rscrates/node/src/run.rscrates/node/tests/config_layered.rscrates/rpc/src/context.rscrates/rpc/src/error.rscrates/rpc/src/handlers.rscrates/rpc/src/handlers/chain.rscrates/rpc/src/lib.rscrates/rpc/src/server.rscrates/rpc/tests/auth.rscrates/rpc/tests/handler_smoke.rsdocker-compose.yamldocs/rest-interface.mddocs/solutions/architecture-patterns/network-selection-keeps-p2p-identity-atomic.mddocs/solutions/architecture-patterns/node-reorg-execution-design.mdtools/bip300301-enforcer/.env.exampletools/bip300301-enforcer/Dockerfile.enforcertools/bip300301-enforcer/docker-compose.yaml
💤 Files with no reviewable changes (3)
- docker-compose.yaml
- crates/rpc/src/error.rs
- crash-1efd99623c772a1dd3aca1178fdafc93c933dec0
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test
🧰 Additional context used
🪛 Betterleaks (1.7.3)
tools/bip300301-enforcer/docker-compose.yaml
[high] 31-32: Discovered a potential basic authorization token provided in a curl command, which could compromise the curl accessed resource.
(curl-auth-user)
🔍 Remote MCP Github Grep
Relevant review context
-
Bitcoin Core’s
invalidateblock:- Rejects unknown blocks before mutation.
- Rejects genesis invalidation.
- Serializes invalidation under a chainstate mutex, then activates the best remaining chain.
-
Bitcoin Core JSON-RPC 2.0 behavior:
- Application-level RPC errors use HTTP 200; HTTP errors are reserved for actual transport/request failures.
- Notifications execute without a response and use HTTP 204.
- Notification-only batches also return HTTP 204.
-
Bitcoin Core supports
-zmqpubsequence; it uses a PUB socket, and sequence-notification high-water marks must be non-negative.
| fn apply_network_selection(&mut self, selection: NetworkSelection) -> Result<()> { | ||
| let network = selection.consensus_network(); | ||
| self.network = network; | ||
| self.p2p_magic = None; | ||
| self.rpc_bind = SocketAddr::from(([127, 0, 0, 1], network.default_rpc_port())); | ||
| self.p2p_listen = vec![SocketAddr::from(([0, 0, 0, 0], network.default_p2p_port()))]; | ||
| self.dns_seeds_enabled = true; | ||
| self.connect.clear(); | ||
|
|
||
| if selection == NetworkSelection::Drynet4 { | ||
| self.p2p_magic = Some(DRYNET4_P2P_MAGIC); | ||
| self.dns_seeds_enabled = false; | ||
| self.connect = vec![parse_connect_addr(DRYNET4_CONNECT).map_err(anyhow::Error::msg)?]; | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Network selection resets low-level keys across configuration layers, and the documentation does not say so. apply_network_selection rewrites rpc_bind, p2p_listen, dns_seeds_enabled, connect, and p2p_magic against the already-merged configuration, so a selection in a higher-precedence layer discards those keys from every lower-precedence layer.
crates/node/src/config.rs#L626-L641: confirm this is intended, or restrict the reset to keys that no earlier layer set explicitly.CONCEPTS.md#L42-L43: state that "later overrides" means the same or a higher-precedence layer, and list the five keys that a selection resets.
📍 Affects 2 files
crates/node/src/config.rs#L626-L641(this comment)CONCEPTS.md#L42-L43
🤖 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/node/src/config.rs` around lines 626 - 641, The network-selection
behavior must be documented and confirmed as intentional: update
apply_network_selection in crates/node/src/config.rs to preserve its current
reset behavior, then update CONCEPTS.md lines 42-43 to define “later overrides”
as the same or a higher-precedence layer and list rpc_bind, p2p_listen,
dns_seeds_enabled, connect, and p2p_magic as keys reset by a selection.
| let transition = handles | ||
| .begin_chain_transition() | ||
| .map_err(|source| ReorgError::Unavailable(Box::new(source)))?; | ||
|
|
||
| loop { | ||
| let (root, target) = { | ||
| let tree = handles.block_tree.read(); | ||
| let root = tree.lookup(hash).ok_or(ReorgError::UnknownBlock(hash))?; | ||
| if tree.node(root).map_err(ReorgError::Plan)?.height == 0 { | ||
| return Err(ReorgError::CannotInvalidateGenesis); | ||
| } | ||
| let target = tree | ||
| .tip_after_invalidation(root) | ||
| .map_err(ReorgError::Plan)? | ||
| .ok_or(ReorgError::NoValidTip)?; | ||
| (root, target) | ||
| }; | ||
|
|
||
| let plan = current_reorg_plan(handles, target)?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
An unbounded retry loop that holds the chain-transition witness the whole time.
The transition is acquired at Line 27, outside the loop. Every continue at Line 64 re-runs the read-side plan, re-runs current_reorg_plan, and reloads every disconnect and connect body from storage, all while the transition lock stays held and the entire apply pipeline waits behind it.
The retry triggers when root or target moved between the optimistic read and the write lock. Header insertion is what moves them, and header insertion does not appear to take the transition. So a node ingesting a fast header stream can spin here, redoing O(tree) planning and body I/O per iteration, with no bound, no backoff, and no log line.
Bound the attempts and fail the RPC when the bound is reached. A retried invalidateblock is cheap; a wedged apply pipeline is not.
🛑 Bound the retries
+ const MAX_INVALIDATE_ATTEMPTS: u32 = 8;
+ let mut attempts = 0_u32;
loop {
+ attempts += 1;
+ if attempts > MAX_INVALIDATE_ATTEMPTS {
+ return Err(ReorgError::Plan(
+ bitcoin_rs_chain::ChainError::UnknownNode { id: /* target */ },
+ ));
+ }Replace the placeholder with a dedicated ReorgError variant, for example RaceRetriesExhausted, so the RPC layer can report a retryable condition rather than a planning fault.
Also applies to: 56-65
🤖 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/node/src/reorg.rs` around lines 27 - 45, Bound the retry loop in the
reorganization flow around begin_chain_transition and current_reorg_plan with a
finite attempt count, returning a dedicated ReorgError variant such as
RaceRetriesExhausted when the limit is reached. Preserve the existing retry
behavior for transient root/target races, but ensure exhaustion exits the RPC
instead of looping indefinitely.
| if let Some(body) = response.body.as_ref() { | ||
| write_json( | ||
| reader.get_mut(), | ||
| response.status, | ||
| response.reason, | ||
| body, | ||
| keep_alive, | ||
| )?; | ||
| } else { | ||
| write_status(reader.get_mut(), 204, "No Content", b"", keep_alive)?; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The 204 path emits Content-Length: 0 and a Content-Type. That is an HTTP framing violation.
write_status unconditionally writes Content-Type: application/json and Content-Length. RFC 9110 forbids Content-Length on a 204 response. You are sending a content-type header for a response that has no content. Strict clients and intermediaries reject or mis-frame this, and on a keep-alive connection that desyncs the pipeline for every following request.
Notifications are the whole point of this change. Do not ship them with a malformed response line.
🐛 Proposed fix: dedicated no-content writer
Add a writer that omits both headers:
fn write_no_content(stream: &mut TcpStream, keep_alive: bool) -> io::Result<()> {
let connection = if keep_alive { "keep-alive" } else { "close" };
write!(
stream,
"HTTP/1.1 204 No Content\r\nConnection: {connection}\r\n\r\n"
)?;
stream.flush()
}Then use it on the no-body path:
} else {
- write_status(reader.get_mut(), 204, "No Content", b"", keep_alive)?;
+ write_no_content(reader.get_mut(), keep_alive)?;
}Extend the auth.rs 204 tests to assert that content-length and content-type are absent from the response headers.
🤖 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 182 - 192, Replace the no-body branch
in the response handling flow with a dedicated no-content writer that emits only
the 204 status, Connection header, and terminating blank line, preserving
keep-alive behavior and flushing the stream. Do not use write_status for 204
responses; add or update the relevant auth.rs tests to verify content-length and
content-type headers are absent.
| const fn error_status(self) -> u16 { | ||
| match self { | ||
| Self::Legacy => 500, | ||
| Self::V2 => 200, | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Bitcoin Core JSON-RPC 1.1 vs 2.0 HTTP status codes 404 500 doc/JSON-RPC-interface.md
💡 Result:
In Bitcoin Core, the behavior of HTTP status codes in the JSON-RPC interface depends on whether the request is processed as legacy JSON-RPC v1.1 or the newer JSON-RPC v2.0 [1][2]. The distinction is documented in doc/JSON-RPC-interface.md [1][3]. JSON-RPC v1.1 Behavior For legacy v1.1 requests (or if the "jsonrpc": "2.0" marker is absent), the interface uses HTTP status codes to communicate both transport-level issues and application-level RPC errors [1][4]. If an RPC error occurs—such as invalid parameters or a method not found—the server returns a non-200 HTTP status code (e.g., 404 for method not found or 400 for invalid requests) [1][5]. JSON-RPC v2.0 Behavior For JSON-RPC v2.0 requests (identified by the presence of "jsonrpc": "2.0" in the request body), the interface is designed to always return HTTP 200 (OK), unless an actual HTTP server-level error occurs, such as a request parsing error or an endpoint not found [1][2]. In this mode, RPC-level errors (like method not found) are returned within the JSON-RPC response body alongside an HTTP 200 status, separating the transport layer from the RPC application layer [1][4]. Key HTTP status codes used by the Bitcoin Core RPC interface include [6][7]: - 200 (HTTP_OK): Success. - 204 (HTTP_NO_CONTENT): Returned for JSON-RPC 2.0 notifications that exclude the "id" field [1][5]. - 400 (HTTP_BAD_REQUEST): Used for invalid requests [6][7]. - 404 (HTTP_NOT_FOUND): Used when a method is not found [6][7]. - 500 (HTTP_INTERNAL_SERVER_ERROR): Used for internal server issues [6][7]. This design shift ensures that v2.0 clients do not need to parse HTTP error headers to interpret JSON-RPC error objects [1][4].
Citations:
- 1: https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md
- 2: https://github.com/bitcoin/bitcoin/blob/v28.1/doc/JSON-RPC-interface.md
- 3: https://github.com/bitcoin/bitcoin/blob/35e6444fdc4068adc79082648f9889ad593e623b/doc/JSON-RPC-interface.md
- 4: https://github.com/bitcoin/bitcoin/blob/master/src/httprpc.cpp
- 5: https://doxygen.bitcoincore.org/httprpc_8cpp_source.html
- 6: https://doxygen.bitcoincore.org/rpc_2protocol_8h.html
- 7: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/protocol.h
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- server.rs relevant definitions and call sites ---'
sed -n '330,475p' crates/rpc/src/server.rs
printf '%s\n' '--- status helpers and tests ---'
sed -n '475,660p' crates/rpc/src/server.rs
printf '%s\n' '--- repository references to error_status and legacy status handling ---'
rg -n -C 3 'error_status|HTTP_NOT_FOUND|Method not found|Invalid request|jsonrpc.*2\.0|status.*batch' cratesRepository: gosuda/bitcoin-rs
Length of output: 1920
🔍 Additional research query:
Search Bitcoin Core's official doc/JSON-RPC-interface.mdandsrc/httprpc.cpp for the exact legacy JSON-RPC HTTP status mapping and batch-response status behavior.
💡 Result:
Legacy JSON-RPC HTTP status mapping
Bitcoin Core’s legacy (v1.1) behavior is:
- Default RPC error:
500 Internal Server Error RPC_INVALID_REQUEST:400 Bad RequestRPC_METHOD_NOT_FOUND:404 Not Found- Successful requests:
200 OK
The documentation summarizes legacy behavior as HTTP 200 unless an RPC error occurs.
Batch-response status behavior
In src/httprpc.cpp:
- Batch members are processed individually.
- Errors are embedded in the batch response; batches do not change the HTTP status and return
200 OK. - Notification responses are omitted.
- A non-empty batch containing only notifications returns
204 No Content. - An empty batch returns an empty JSON array with
200 OK, preserving backward compatibility.
🔍 Additional research query:
Search GitHub Grep for gosuda/bitcoin-rs crates/rpc/src/server.rs, including error_status, batch status aggregation, and empty-batch handling.
💡 Result:
No relevant context found from external knowledge sources.
Fix legacy and batch HTTP status handling.
Map legacy RPC errors by code: RPC_METHOD_NOT_FOUND to 404, RPC_INVALID_REQUEST to 400, and other errors to 500. Return HTTP 200 for batches with a JSON body, including batches containing errors and empty batches.
🤖 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 346 - 351, Update the error-status
logic around error_status to accept the RPC error code and map
RPC_METHOD_NOT_FOUND to 404, RPC_INVALID_REQUEST to 400, and all other legacy
errors to 500. Ensure batch responses with JSON bodies, including
error-containing and empty batches, consistently return HTTP 200, while
preserving the existing V2 behavior.
Source: MCP tools
| BITCOIN_RS_RPC_USER: "${BITCOIN_RS_RPC_USER:-bitcoin-rs}" | ||
| BITCOIN_RS_RPC_PASSWORD: "${BITCOIN_RS_RPC_PASSWORD:-password}" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the shipped RPC password fallback.
Line 16 starts the node with the known password password when .env is absent. Any local process can then authenticate to the loopback RPC endpoint and invoke chain-control methods.
Require BITCOIN_RS_RPC_PASSWORD during Compose interpolation. Remove its fallback from the enforcer command too.
Proposed fix
- BITCOIN_RS_RPC_PASSWORD: "${BITCOIN_RS_RPC_PASSWORD:-password}"
+ BITCOIN_RS_RPC_PASSWORD: "${BITCOIN_RS_RPC_PASSWORD:?Set BITCOIN_RS_RPC_PASSWORD in .env}"
- - --node-rpc-pass=${BITCOIN_RS_RPC_PASSWORD:-password}
+ - --node-rpc-pass=${BITCOIN_RS_RPC_PASSWORD:?Set BITCOIN_RS_RPC_PASSWORD in .env}🤖 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 `@tools/bip300301-enforcer/docker-compose.yaml` around lines 15 - 16, Remove
the default “password” fallback from BITCOIN_RS_RPC_PASSWORD in the Compose
environment and enforcer command, requiring the variable during interpolation
while leaving BITCOIN_RS_RPC_USER behavior unchanged.
…rformance and memory efficiency
… variables for txindex and electrum_bind
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/node/src/config.rs (1)
628-640: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDerive the default data directory from the network selection.
apply_network_selectionchanges P2P identity but leavesdata_dirunchanged.Config::default_for_networkalso uses.bitcoin-rsfor every network. Therefore,--network drynet4can open mainnet runtime state unless the operator manually setsdata_dir.
crates/node/src/config.rs#L628-L640: assign a Drynet4-specific default data directory before same-layer explicitdata_dirsettings apply.docs/solutions/architecture-patterns/network-selection-keeps-p2p-identity-atomic.md#L45-L46: keep this claim only after the configuration behavior enforces it.🤖 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/node/src/config.rs` around lines 628 - 640, Update Config::apply_network_selection to assign the Drynet4-specific default data_dir when selection is NetworkSelection::Drynet4, while preserving explicit same-layer data_dir settings applied afterward; retain the existing default directory for other networks. In docs/solutions/architecture-patterns/network-selection-keeps-p2p-identity-atomic.md lines 45-46, keep the claim unchanged because it is corrected by the configuration change and requires no direct code change.
🤖 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/node/src/config.rs`:
- Around line 909-923: Update parse_connect_endpoint to reject port 0 after
parsing the port, while preserving valid nonzero ports. Add equivalent
validation for TOML ConfigLayer.connect values, which bypass this parser, and
ensure the fixed-peer dialing path never passes zero-port endpoints to
ToSocketAddrs. Add regression coverage for CLI, TOML, and BITCOIN_RS_CONNECT
configuration sources.
---
Outside diff comments:
In `@crates/node/src/config.rs`:
- Around line 628-640: Update Config::apply_network_selection to assign the
Drynet4-specific default data_dir when selection is NetworkSelection::Drynet4,
while preserving explicit same-layer data_dir settings applied afterward; retain
the existing default directory for other networks. In
docs/solutions/architecture-patterns/network-selection-keeps-p2p-identity-atomic.md
lines 45-46, keep the claim unchanged because it is corrected by the
configuration change and requires no direct code change.
🪄 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: 1db9da69-99b4-4e28-8f9c-ae9833402af6
📒 Files selected for processing (5)
CONCEPTS.mdcrates/node/src/config.rscrates/node/src/run.rscrates/node/tests/config_layered.rsdocs/solutions/architecture-patterns/network-selection-keeps-p2p-identity-atomic.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/node/tests/config_layered.rs
- CONCEPTS.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: bench-smoke
- GitHub Check: test
- GitHub Check: clippy
🧰 Additional context used
🔍 Remote MCP Github Grep
Relevant review context
- Bitcoin Core’s
invalidateblockrejects unknown hashes with “Block not found,” invalidates the selected block and descendants, then activates the best remaining chain. - Bitcoin Core explicitly refuses to invalidate the genesis block and serializes invalidation against chainstate transitions.
- Bitcoin Core’s JSON-RPC behavior supports HTTP 204 for notifications and notification-only batches; ordinary requests use HTTP 200.
- Bitcoin Core’s functional tests verify that invalidation can switch to an alternate chain and that repeated invalidation moves the active tip backward as expected.
🔇 Additional comments (3)
crates/node/src/config.rs (1)
119-152: LGTM!Also applies to: 373-442, 500-626, 765-800
crates/node/src/run.rs (1)
4-4: LGTM!Also applies to: 75-87, 532-575, 921-921
docs/solutions/architecture-patterns/network-selection-keeps-p2p-identity-atomic.md (1)
1-44: LGTM!
| fn parse_connect_endpoint(value: &str) -> std::result::Result<String, String> { | ||
| let value = value.trim(); | ||
| if value.parse::<SocketAddr>().is_ok() { | ||
| return Ok(value.to_owned()); | ||
| } | ||
| let Some((host, port)) = value.rsplit_once(':') else { | ||
| return Err(format!("connect peer `{value}` must include a port")); | ||
| }; | ||
| if host.is_empty() { | ||
| return Err(format!("connect peer `{value}` has an empty hostname")); | ||
| } | ||
| port.parse::<u16>() | ||
| .map_err(|error| format!("connect peer `{value}` has an invalid port: {error}"))?; | ||
| Ok(value.to_owned()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Rust standard library documentation: does std::net::SocketAddr parsing accept a port of zero, and does u16 parsing accept "0"?
💡 Result:
Yes, both std::net::SocketAddr parsing and u16 parsing accept zero in Rust. 1. std::net::SocketAddr Parsing: The FromStr implementation for SocketAddr (and SocketAddrV4/SocketAddrV6) parses a port as a decimal integer [1][2][3][4]. This implementation accepts a port of zero [1][2]. In networking, a port of zero is a valid u16 value, and Rust's standard library does not impose additional restrictions during string parsing that would exclude it [1][2]. 2. u16 Parsing: The primitive type u16 provides from_str_radix (and FromStr which uses radix 10 by default) [5][6][7]. These methods correctly parse the string "0" as the integer value 0, as it is a valid member of the u16 range (0 to 65,535) [5][6][8].
Citations:
- 1: https://doc.rust-lang.org/std/net/enum.SocketAddr.html
- 2: https://doc.rust-lang.org/stable/src/core/net/socket_addr.rs.html
- 3: https://doc.rust-lang.org/std/net/struct.SocketAddrV4.html
- 4: https://doc.rust-lang.org/stable/src/core/net/parser.rs.html
- 5: https://doc.rust-lang.org/1.74.0/std/primitive.u16.html
- 6: https://dev-doc.rust-lang.org/beta/core/primitive.u16.html
- 7: https://doc.rust-lang.org/1.69.0/std/primitive.u16.html
- 8: https://doc.rust-lang.org/std/primitive.u16.html
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate files ---'
git ls-files 'crates/node/src/config.rs' '*config*' '*network*' | head -80
printf '%s\n' '--- config outline ---'
ast-grep outline crates/node/src/config.rs | sed -n '1,180p'
printf '%s\n' '--- endpoint references ---'
rg -n -C 4 'parse_connect_endpoint|connect|fixed.?peer|fixed_peers|port.*0|default_for_network|apply_network_selection' crates/node/src config* 2>/dev/null | head -320
printf '%s\n' '--- target implementation ---'
sed -n '850,970p' crates/node/src/config.rsRepository: gosuda/bitcoin-rs
Length of output: 29604
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- validation and layering ---'
sed -n '330,430p' crates/node/src/config.rs
sed -n '500,655p' crates/node/src/config.rs
printf '%s\n' '--- all fixed-peer consumers ---'
rg -n -C 6 '\bconnect\b|fixed outbound|fixed peer|DNS seed' crates/node/src crates/node/tests --glob '*.rs' | grep -v -E 'zmq|import.rs' | head -300
printf '%s\n' '--- configuration tests ---'
rg -n -C 5 'connect|BITCOIN_RS_CONNECT|toml|environment|env|invalid port|hostname' crates/node/tests/config_layered.rs crates/node/src/config.rs | tail -320
printf '%s\n' '--- repository policy and dependency context ---'
rg -n -C 3 'connect peer|fixed outbound|port zero|port 0|outbound peer|dial' README.md docs crates --glob '*.md' --glob '*.rs' | head -240Repository: gosuda/bitcoin-rs
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- ConfigLayer declarations and TOML deserialization ---'
sed -n '660,770p' crates/node/src/config.rs
printf '%s\n' '--- Exact fixed-peer consumers ---'
rg -n -C 8 'config\.connect|self\.config\.connect|\.connect\.iter|connect\.iter|connect\.as_slice|connect\]' crates --glob '*.rs' | head -240
printf '%s\n' '--- Existing config test structure ---'
sed -n '1,220p' crates/node/tests/config_layered.rs
sed -n '440,520p' crates/node/tests/config_layered.rs
printf '%s\n' '--- Lightweight behavioral probe for both parser branches ---'
python3 - <<'PY'
import ipaddress
def socket_addr_like(value):
host, sep, port = value.rsplit(":", 1) if ":" in value else ("", "", "")
if not sep or ":" in host:
return False
try:
ipaddress.ip_address(host)
return 0 <= int(port) <= 65535
except ValueError:
return False
def parse_connect_endpoint_like(value):
value = value.strip()
if socket_addr_like(value):
return ("ok", value, "SocketAddr branch")
if ":" not in value:
return ("err", value, "missing port")
host, port = value.rsplit(":", 1)
if not host:
return ("err", value, "empty hostname")
try:
parsed = int(port)
if not 0 <= parsed <= 65535:
raise ValueError
except ValueError:
return ("err", value, "invalid port")
return ("ok", value, "hostname branch")
for endpoint in ("127.0.0.1:0", "localhost:0", "127.0.0.1:1", "localhost:65535"):
print(endpoint, "=>", parse_connect_endpoint_like(endpoint))
PYRepository: gosuda/bitcoin-rs
Length of output: 23243
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- fixed-peer bootstrap path ---'
rg -n -C 12 'spawn_fixed_peer_bootstrap|config\(\)\.connect|to_socket_addrs|SocketAddr|connect\(' crates/node/src/run.rs crates/node/src --glob '*.rs' | grep -E 'run.rs|bootstrap|fixed|config\(\)\.connect|to_socket_addrs|SocketAddr' | head -260
printf '%s\n' '--- TOML connect field handling ---'
rg -n -C 3 'connect\s*=' crates/node/tests crates/node/src docs README.md --glob '*.rs' --glob '*.md' | head -180
printf '%s\n' '--- corrected parser-branch probe ---'
python3 - <<'PY'
import ipaddress
def socket_addr_like(value):
if ":" not in value:
return False
host, port = value.rsplit(":", 1)
try:
ipaddress.ip_address(host.strip("[]"))
return 0 <= int(port) <= 65535
except ValueError:
return False
def parse_connect_endpoint_like(value):
value = value.strip()
if socket_addr_like(value):
return ("ok", "SocketAddr branch")
if ":" not in value:
return ("err", "missing port")
host, port = value.rsplit(":", 1)
if not host:
return ("err", "empty hostname")
try:
parsed = int(port)
if not 0 <= parsed <= 65535:
raise ValueError
except ValueError:
return ("err", "invalid port")
return ("ok", "hostname branch")
for endpoint in ("127.0.0.1:0", "localhost:0", "127.0.0.1:1", "localhost:65535"):
print(f"{endpoint} => {parse_connect_endpoint_like(endpoint)}")
PYRepository: gosuda/bitcoin-rs
Length of output: 26248
Reject port 0 for every fixed-peer configuration source.
parse_connect_endpoint accepts 127.0.0.1:0 and host:0. The fixed-peer worker then passes these endpoints to ToSocketAddrs. Reject port zero before dialing. Apply the check to TOML too; ConfigLayer.connect bypasses parse_connect_endpoint during deserialization. Add CLI, TOML, and BITCOIN_RS_CONNECT regression tests.
🤖 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/node/src/config.rs` around lines 909 - 923, Update
parse_connect_endpoint to reject port 0 after parsing the port, while preserving
valid nonzero ports. Add equivalent validation for TOML ConfigLayer.connect
values, which bypass this parser, and ensure the fixed-peer dialing path never
passes zero-port endpoints to ToSocketAddrs. Add regression coverage for CLI,
TOML, and BITCOIN_RS_CONNECT configuration sources.
1 participant
Enable bitcoin-rs to sync with the ecash drynet4 network and run alongside the BIP300/301 enforcer.