Repository navigation
Conversation
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR implements BIP434 peer feature negotiation: protocol version bump to 70017, a new Feature message variant with codec, FSM-level receive-window enforcement (post-version, pre-remote-verack), and a matching send-window guard (sent_verack flag in Peer). The architectural changes are coherent — the outbound handshake now sends only VERSION before learning the remote version, feature messages are blocked from the async post-handshake writer queue, and duplicate/unknown feature IDs are ignored without state retention.
One confirmed defect requiring a fix:
A truncated feature payload in decode_feature returns PeerError::Protocol (a local/non-punitive error class) for a remotely-sourced structural violation. Every other comparable decode error in this file uses PeerError::Misbehavior, which is the correct type for remote violations — it flows through PeerError::inbound() → Incoming(...) → is_misbehavior() == true, affecting reconnect policy appropriately. Using Protocol here means a peer that sends a truncated feature payload is never penalized.
The connection.rs comment I initially added has been reviewed more carefully — the credit is correctly dropped (released) when enqueue returns early with OutboundRefusal::Closed, as confirmed by the test at line 1567-1568. That comment should be disregarded as a false positive.
Draft state is noted: Live interoperability with a BIP434-capable Core node and validation on the declared Rust 1.99 toolchain remain outstanding per the PR description.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e sends pre-charge - fsm already rejects a repeated VERSION as misbehavior; update the discouragement test to assert it (protected leases ignore it and stall to the read deadline) and keep the first version's stored services. - decode_feature: a truncated feature identifier is malformed wire data, so classify it as Misbehavior, not a recoverable Protocol error. - PeerLease::send/send_with: refuse Feature messages before charging the outbound budget so the credit never round-trips for a refused send. - getnetworkinfo: report the negotiated protocol version from wire::PROTOCOL_VERSION (70017) instead of the stale 70016 literal; the pinned-Core corpus declares the divergence as a gap key, and the e2e introspection assertion follows the advertised version.
|
Pushed
This should also fix the |
|
@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. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
# Conflicts: # Cargo.lock # crates/p2p/src/bip324/msg_type.rs # crates/p2p/src/compat.rs # crates/p2p/src/fsm.rs # crates/p2p/src/handshake.rs # crates/p2p/src/wire.rs # crates/p2p/tests/core_compat.rs # crates/rpc/tests/core_compat.rs # crates/rpc/tests/corpus/core-31.1/20_batch_two_requests.json # crates/rpc/tests/handler_smoke.rs # docs/contracts/p2p-wire.md # docs/policies/p2p-compatibility.md # e2e/tests/p2p_sync.rs # fuzz/Cargo.lock
Adapt the PR's remaining send-window gating and tests to the
crate::feature::{FeatureAnnouncement, FeatureId} API that main landed in
#1694 (Message::Feature tuple variant, FEATURE_VERSION in crate::feature),
and keep the PR's duplicate-VERSION rejection ahead of the service gate:
main's receive_version overwrites remote_version mid-handshake, which
routes a repeated VERSION into 'outbound peer lacks desirable services'
instead of the duplicate-version misbehavior this PR asserts.
|
Merge-conflict sweep: I resolved all 14 conflicted files against current Resolution notes:
Remaining red is inherited from |
|
Closing as superseded. The base BIP434 implementation is already on main through #1697; the remaining duplicate-VERSION and BIP434 send-window/queue protections were adapted to current main in #1714. #1714 preserves current BIP332/BIP331 negotiation and includes scoped unit, loopback v1/v2, integration and synchronization evidence (149 tests passed). It does not claim new live BIP434 Core interoperability or close the outstanding reference-evidence requirement in #1634. The source branch is preserved; this PR is closed without merging. |
…1714) BIP434 feature advertisements could pass through `Peer::send` without a negotiated remote version or after our VERACK, and the post-handshake writer queue could accept them. A second VERSION before readiness could also replace the accepted version and replay negotiation responses. Extract the remaining boundary fixes from #1674 onto current main: - Keep the local VERACK write fact in `Peer`. Permit `feature` only with a known remote version >=70017 and before a successful local VERACK write. The remote VERACK closes only the receive window. - Reject `feature` at both PeerLease queue entrypoints, before ordinary budget charging and with reserved credits released. This remains a local caller error even under aggregate pressure. - Reject duplicate VERSION before mutating peer state, preserve the original receive timestamp, and keep protected peers under the existing ignore/deadline rules. Update the existing address-book regression to retain the first accepted service flags. - Update the owning handshake policy/evidence references. Existing BIP332/BIP331 offers, message codecs, startup preference ordering and protocol version remain unchanged. [BIP434](https://github.com/bitcoin/bips/blob/master/bip-0434.md#specification) requires the remote version threshold and prohibits sending feature after the sender's VERACK. The send fact is independent of receiving the other side's VERACK. Validation on Windows, Rust 1.99: - Before the fix, the new direct-send test accepted feature with no remote version; the queue test classified feature as Shared backpressure instead of refusing it. - **113 selected library tests passed**: FSM, handshake/BIP324, peer sends, connection budgets, duplicate VERSION dispatch/metadata, real v1/v2 listener/fallback/deadline tests, and both synchronization proofs below. - **36 integration tests passed**: `cargo test -p bitcoin-rs-p2p --test handshake_roundtrip --test wire_codec --test core_compat`. - The two direct-send tests passed again after the final assertion-only lint edits. - `cargo clippy -p bitcoin-rs-p2p --lib --tests --no-deps -- -D warnings -A dead_code`, workspace formatting and diff checks passed. The command without `-A dead_code` remains blocked by existing Windows `sam::tests::FakeSam` fields `strangers`/`accepted`; the existing storage checkpoint dead-code warning is also unchanged. - New loopback evidence enables BIP332 and BIP331 together, checks exact handshake command order for 70016/70017 over v1/v2, sends features after receiving the server VERACK but before sending the client VERACK, refuses local post-VERACK feature writes, then verifies ping/pong. Synchronization impact: this touches peer handshake/readiness and outbound queue admission, not authoritative header admission, chain validation, chain selection or durable commit. Relevant clauses are SYNC-02 (connection ownership/frontier recovery), SYNC-04 (typed protocol failure), and SYNC-06 (eligible peer/request drivers). The smallest existing evidence-map proofs `tick_fetches_new_tip_headers_from_at_tip_peers` and `cold_start_stall_hedges_front_without_reassigning_owner` both passed. Existing connection reservation/release and socket fallback/deadline tests cover the changed local boundary. No locks, persisted representations, schemas or generation counters are added. Limits: loopback peers use this implementation. No new live BIP434-capable Core interoperability campaign was run; this does not close the outstanding reference-evidence requirement in #1634.
Implements the BIP434 protocol layer from #1634 so peers can advertise independent extensions before verack. Previously outbound startup sent preference messages before learning the remote version and repeated them on VERSION reception. Startup now sends VERSION only; the existing VERSION dispatch generates negotiation responses once. Duplicate VERSION messages are rejected rather than replaying negotiation.
Implemented behavior
featurecodec with 4–80-byte IDs, at most 512 configuration bytes, minimal CompactSize, exact payload consumption, and bounded allocation. Non-UTF8 IDs remain ignorable, as printable ASCII is a recommendation.8598ec220450b05eff03fc1d1c81f51269cca247.No BIP434 extension is enabled yet, so production generates no
featureadvertisements. In particular, this does not advertise or implement BIP332 (#1637), retain unknown feature registries, or introduce another handshake/state owner. Recognized extension configuration belongs to its eventual implementation in the existing connection state.Validation
Executed under WSL with Rust 1.95.0 using
--ignore-rust-version; the repository declares Rust 1.99.0, which is not installed. No toolchain, manifest, or lockfile requirements were changed.cargo fmt --all -- --checkandgit diff --check: pass.cargo test --ignore-rust-version -p bitcoin-rs-p2p --test wire_codec --test handshake_roundtrip --test core_compat: 36 tests pass, including independent codec bytes, malformed/nonminimal/trailing payloads and existing TCP/serving compatibility regressions.bip434,fsm::tests,handshake::tests,bip324::msg_type::tests, andlistener::v2_transport_tests: pass. The new real-socket test exercises 70015/70016/70017 clients over v1/v2, duplicate/non-UTF8 unknown features before the remote verack, exact server handshake order, and subsequent ping responses. Existing outbound v2, legacy fallback, and deadline tests also pass.tick_fetches_new_tip_headers_from_at_tip_peersandcold_start_stall_hedges_front_without_reassigning_owner: pass.cargo clippy --ignore-rust-version --locked -p bitcoin-rs-p2p --all-targets --no-deps -- -D warnings -A clippy::duration_suboptimal_units: pass. The unmodified strict command fails on existingduration_suboptimal_unitswarnings in UTXO;--no-depsalone also encounters existing P2P service/tx-policy instances. No unrelated lint cleanup is included.Synchronization impact
This changes the peer readiness/handshake path and the new message's outbound queue classification. It does not change authoritative header admission, validation, chain selection, or durable commit. Relevant clauses are
SYNC-02(connection ownership/frontier recovery),SYNC-04(protocol failure classification), andSYNC-06(eligible peers reach the request driver). The two existing sync proofs above are the smallest relevant driver/headroom evidence from the synchronization contract's map; the real-socket v2/fallback tests cover the affected connection boundary. No new locks, persistence schema, chain frontier, or retained operator-data representation are introduced.Remaining evidence / why draft
Live interoperability with a BIP434-capable Core and validation on the declared Rust 1.99 toolchain remain outstanding. The socket tests use this implementation's listener, not a Core daemon; existing Core 31.1 evidence is not new BIP434 evidence. Deep Core interoperability stays in its existing evidence lane. Unknown-feature duplicates are covered; supported-feature configuration/conflict semantics will be defined when an actual extension is enabled. This PR deliberately leaves #1634 open pending its live-reference acceptance evidence.
No code defect found in this re-review blocks merging.Summary
Adds BIP434 feature negotiation while keeping the Core 31.1 baseline explicit.
Live Core interoperability and verification on Rust 1.99 remain explicitly outstanding; this review did not run tests.
Reviews (3) · Last reviewed commit: "fix(p2p): reject duplicate VERSION befor..." · Reviewed by Greptile