Skip to content

p2p_core%feat!: type the BIP37 and BIP157 messages - #122

Open
PastaPastaPasta wants to merge 2 commits into
developfrom
pasta/p2p-bip37-bip157
Open

PastaPastaPasta wants to merge 2 commits into
developfrom
pasta/p2p-bip37-bip157

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

Motivation

dash-p2p-core recognises the BIP37 and BIP157 messages but leaves their payloads as raw bytes, so a client that loads a bloom filter, reads a filtered block or follows compact block filters has to lay out and parse each message itself. #40 lists the compact filter stubs as a regression from #39. This pull request types the ten messages in both directions and checks them against vectors written from the reference implementation's code.

The types follow bitcoin-p2p-messages (rust-bitcoin#7161, studied at rust-bitcoin 94c6f791): its names, fields, field order and decode semantics, with Dash's block header and hash types where the bytes are Dash's. A TODO(pasta) at the top of each module marks the swap to that crate once it is released. Nothing waits on it, and nothing is deferred.

Additional Information

Description Notes
Type the BIP37 messages FilterLoad, BloomFlags, FilterAdd, PartialMerkleTree, TxMerkleNode and MerkleBlock, in src/msg/bloom.rs. filterclear moves from the stubs to the empty messages.
Type the BIP157 messages GetCFilters, CFilter, GetCFHeaders, CFHeaders, GetCFCheckpt, CFCheckpt, FilterHash and FilterHeader, in src/msg/cfilter.rs. dash-p2p-core now depends on bitcoin-units, for BlockHeight.
Add BIP37 and BIP157 vectors corpus/bloom.json5 and corpus/cfilter.json5, each section with the program that writes it, run in the reference implementation's bloom_tests and blockfilter_tests at v24.0.0-rc.3 (0364ed626e). One section per message holds payloads with their fields. filterload_reads, filteradd_reads and cfilter_reads hold payloads with whether the reference implementation reads them. A comment marks each vector this library reads differently.

Types against bitcoin-p2p-messages

Every type keeps upstream's name, fields and field order. Across all of them, fields are private, with getters and new; decoding goes through BaseCodec and DecodeError rather than a decoder per type; and serde is derived, with renames that keep the vectors' keys.

Type Upstream Fields Differences
FilterLoad message_bloom::FilterLoad filter, hash_funcs, tweak, flags: BloomFlags None.
BloomFlags message_bloom::BloomFlags None = 0, All = 1, PubkeyOnly = 2 Serde writes the number.
FilterAdd message_bloom::FilterAdd data None.
P2pMsg::FilterClear NetworkMessage::FilterClear none None.
MerkleBlock merkle_tree::MerkleBlock header: BlockHeader, txn The header is Dash's, hashed with X11, where upstream holds a Bitcoin header, so this type stays ours after the swap. Serde key tree. Upstream's builders and extract_matches are not mirrored.
PartialMerkleTree merkle_tree::PartialMerkleTree num_transactions, bits: Vec<bool>, hashes: Vec<TxMerkleNode> Serde keys total_transactions and flags, the bits as the hex of their packed bytes. from_txids and extract_matches are not mirrored.
TxMerkleNode primitives::TxMerkleNode 32 bytes A Dash hash type.
GetCFilters, GetCFHeaders message_filter::GetCFilters, GetCFHeaders filter_type, start_height: BlockHeight, stop_hash stop_hash is Dash's BlockHash.
CFilter message_filter::CFilter filter_type, block_hash, filter block_hash is Dash's BlockHash.
CFHeaders message_filter::CFHeaders filter_type, stop_hash, previous_filter_header, filter_hashes Serde key prev_filter_header.
GetCFCheckpt message_filter::GetCFCheckpt filter_type, stop_hash None.
CFCheckpt message_filter::CFCheckpt filter_type, stop_hash, filter_headers None.
FilterHash, FilterHeader message_filter::FilterHash, FilterHeader 32 bytes Dash hash types over the same bytes. FilterHash::filter_header() is not mirrored; decoding doesn't need it.

Limits

Upstream bounds every byte vector and every typed vector at 4,000,000. Each type here that applies the bound names it as an associated const, enforced at decode.

Field Bound Const
FilterLoad::filter 4,000,000 bytes, was 36,000 FilterLoad::MAX_FILTER_SIZE
FilterAdd::data 4,000,000 bytes, was 520 FilterAdd::MAX_DATA_SIZE
FilterLoad::flags 0, 1 or 2, was any byte BloomFlags
PartialMerkleTree hash count and flag byte count 4,000,000 each, was 33,554,432 PartialMerkleTree::MAX_VEC_SIZE
CFilter::filter_type any byte, was 0 only none
CFilter::filter 4,000,000 raw bytes, was 33,554,432 bytes holding exactly their Golomb-Rice codes CFilter::MAX_FILTER_SIZE
CFHeaders::filter_hashes 4,000,000, was 33,554,432 CFHeaders::MAX_COUNT
CFCheckpt::filter_headers 4,000,000, was 33,554,432 CFCheckpt::MAX_COUNT

A payload holds at most 3,145,728 bytes, so where the bound replaces 33,554,432 it changes only which error a bad count gives. The hash function count, the tweak and the filter type of every message stay unchecked.

Decode changes from the reference implementation

  • filterload: a flags byte other than 0, 1 or 2 fails, though the reference reads any byte. A filter of 36,001 bytes or more decodes, up to the payload cap; the reference refuses it.
  • filteradd: an element of 521 bytes or more decodes, up to the payload cap; the reference refuses it.
  • cfilter: any filter type and any filter bytes decode. The reference refuses a type other than 0, and a filter that doesn't hold exactly as many Golomb-Rice codes as its count says.

Every other payload decodes as before. No valid message encodes to different bytes, and every vector that decodes round-trips byte for byte.

BIP152 compact blocks and the block and tx messages stay stubs; the BIP37 and BIP157 types need neither.

Breaking Changes

P2pMsg::FilterLoad, P2pMsg::FilterAdd, P2pMsg::MerkleBlock, P2pMsg::GetCFilters, P2pMsg::CFilter, P2pMsg::GetCFHeaders, P2pMsg::CFHeaders, P2pMsg::GetCFCheckpt and P2pMsg::CFCheckpt hold the typed messages instead of Vec<u8>, so code that builds or matches them must change. P2pMsg::is_stub is false for them and for P2pMsg::FilterClear.

How Has This Been Tested?

  • The vector programs ran in the reference implementation's test_dash at v24.0.0-rc.3. The committed sections are their output; this round only added comments.
  • Each payload section is checked entry by entry. The payload decodes to the listed fields, encodes back to the same bytes, decodes the same through P2pMsg::decode_payload, and round-trips through JSON. In filterload, the one vector with flags 0xff fails at that byte with InvalidValue, and every other vector round-trips.
  • filterload_reads and filteradd_reads (6 and 5 payloads) decode as the reference reads them, apart from the one size each between the old bound and 4,000,000, which this library reads. All 21 cfilter_reads payloads decode and encode back to the same bytes, including the 15 the reference refuses.
  • Unit tests pin:
    • BloomFlags against the vectors' labels (update_all is All, update_p2pubkey_only is PubkeyOnly, empty is None), with bytes 3 and 0xff failing;
    • partial merkle tree bits, least significant first, on three merkleblock vectors and on a hand-built nine-bit tree that packs to 01 01;
    • start_height as little endian, in both requests;
    • a count of 4,000,000 passing and one of 4,000,001 failing with CompactSizeExceedsLimit, for each of the seven bounded vectors;
    • a filterclear with a payload giving PayloadNotEmpty.
  • Each of seven changes to the code makes at least one of these tests fail, with no build error:
    • bits packed most significant first;
    • bits packed and unpacked most significant first;
    • the flag byte count written as the bit count;
    • BloomFlags 1 and 2 swapped;
    • BloomFlags accepting 3;
    • getcfilters without start_height;
    • the cfheaders bound one higher.
  • contrib/git_filter.py --fast-fail origin/develop HEAD -- sh -c '<cmds>' passes on each commit, where <cmds> is cargo clippy -p dash-p2p-core --all-features --all-targets -- -D warnings and cargo nextest run -p dash-p2p-core, with and without --all-features.
  • On the tip:
    • cargo nextest run --workspace --all-features passes, 1502 tests;
    • cargo doc -p dash-p2p-core --all-features --no-deps builds with -D warnings;
    • cargo fmt --check passes.
  • maint/lint/lint_{rust,markdown,python,bazel,nix,just,symlinks,javascript,clang}.py, lint_bazel.py graph, lint_commit.py over the range, semgrep, cargo-deny and gh actions-lock --verify pass on the tip, and CodeQL (rust-security-and-quality plus the repository's queries) reports nothing on it.
  • The branch sits on develop a20120b (nightly-2026-02-24), so every check above ran on that base and toolchain.

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional tests
  • I have made corresponding changes to the documentation
  • I did not use agentic assistance (i.e. LLMs, AI) except as disclosed and can answer questions,
    comments and concerns without said assistance. I assume authorship of the code submitted.
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

Agent disclosure

Claude Code, using Claude Opus 5.5 and running autonomously as an agent on behalf of the author, generated the code, the commit messages and this description. The agent also built the reference implementation's unit tests at v24.0.0-rc.3 and ran the programs that wrote the vectors. A Claude Code agent using Opus 5.5 reviewed the change.

🤖 Generated with Claude Code

@PastaPastaPasta PastaPastaPasta added this to the 0.3 milestone Nov 5, 2026
@github-actions

github-actions Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Warning

This pull request may have conflicts, please coordinate with the authors of these pull requests.

Potential conflicts

@coderabbitai

coderabbitai Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 90952f8 and ad130ec.

Files ignored due to path filters: 4

  • Cargo.lock is excluded by !**/*.lock, !**/*.lock
  • MODULE.bazel.lock is excluded by !**/*.lock, !**/*.lock
  • pkgs/p2p_core/corpus/bloom.json5 is excluded by !**/*.json5
  • pkgs/p2p_core/corpus/cfilter.json5 is excluded by !**/*.json5
Files selected for processing: 4

  • pkgs/p2p_core/Cargo.toml
  • pkgs/p2p_core/src/msg/bloom.rs
  • pkgs/p2p_core/src/msg/cfilter.rs
  • pkgs/p2p_core/src/msg/mod.rs
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@thepastaclaw

thepastaclaw commented Nov 5, 2026 •

Copy link
Copy Markdown

✅ Final review complete — no blockers (commit ad130ec) · triage: critical

@codecov

codecov Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added the status:behind This pull request can be rebased on the base branch. label Nov 5, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Final validation — Phase 1 + Phase 2

Verified both findings against head 972afb8. The new V1 header's generated push decoder does not stop at the 24-byte header boundary; the compact-filter API also exposes distinct hash domains through interchangeable types. Validation was static only: current CI build, test, lint, and cross-platform checks pass, while the corpus build/check job was skipped.

🔴 1 blocking | 🟡 1 suggestion(s)

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The diff adds substantial, intricate peer-facing network deserialization in pkgs/p2p_core/src/msg/bloom.rs and pkgs/p2p_core/src/msg/cfilter.rs, including bounded payload reads and Golomb-Rice validation.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `pkgs/p2p_core/src/v1.rs`:
- [BLOCKING] pkgs/p2p_core/src/v1.rs:22-26: Complete the V1 header's push decoder after 24 bytes
  `codec_p2p!` generates a `VecDecoder` whose buffering limit is `MAX_P2P_PAYLOAD_SIZE` (3,145,728), not `V1Header::SIZE`. That decoder consumes input up to its limit and returns `NeedsMore` until the limit is reached. Consequently, `decode_from_slice_unbounded::<V1Header>` given a 24-byte header followed by an eight-byte ping payload consumes all 32 bytes and fails with `TrailingBytes { remaining: 8 }`; reader-based decoding on an open connection continues waiting after receiving a complete header. This breaks the header-before-payload verification boundary: Dash Core reads only the header before checking magic and size (https://github.com/dashpay/dash/blob/0364ed626e/src/net.cpp#L708-L746). Generate this header's codec with a 24-byte buffering limit in the P2P layer, and add a push-decoder regression asserting `Ready` after 24 bytes with subsequent payload bytes left unconsumed. The existing framing tests call `BaseCodec::decode`, bypassing the affected generated `Decode` implementation.

In `pkgs/p2p_core/src/msg/cfilter.rs`:
- [SUGGESTION] pkgs/p2p_core/src/msg/cfilter.rs:165-168: Distinguish filter hashes from filter headers with newtypes
  `CFHeaders::prev_filter_header`, `CFHeaders::filter_hashes`, and `CFCheckpt::filter_headers` all use `Hash256`, so checkpoint headers can be passed as filter hashes without a compiler error. Collecting `CFilter::hash()` results also satisfies `filter_hashes`, despite that method explicitly hashing the entire payload rather than the encoded filter. These are distinct protocol domains: Core hashes the encoded filter for its filter hash and combines that hash with the previous header for its filter header (https://github.com/dashpay/dash/blob/0364ed626e/src/blockfilter.cpp#L254-L261). Introduce separate `FilterHash` and `FilterHeader` newtypes in the owning module and use them across both responses. This follows the domain-safety guidance in `AGENTS.md` and `docs/dev/guide_rust.md` without changing wire encoding; the sister rust-dashcore implementation already uses these distinct types in `dash/src/network/message_filter.rs`.

Comment thread pkgs/p2p_core/src/v1.rs Outdated
Comment on lines +22 to +26
codec_p2p! {
/// The header in front of every V1 message payload.
#[derive(Clone, Copy, CopyGetters, Debug, Eq, Hash, PartialEq, TypeId)]
#[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))]
pub struct V1Header {

@thepastaclaw thepastaclaw Nov 5, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Resolved at ad130ec0; see the replies below.

🔴 Blocking: Complete the V1 header's push decoder after 24 bytes

codec_p2p! generates a VecDecoder whose buffering limit is MAX_P2P_PAYLOAD_SIZE (3,145,728), not V1Header::SIZE. That decoder consumes input up to its limit and returns NeedsMore until the limit is reached. Consequently, decode_from_slice_unbounded::<V1Header> given a 24-byte header followed by an eight-byte ping payload consumes all 32 bytes and fails with TrailingBytes { remaining: 8 }; reader-based decoding on an open connection continues waiting after receiving a complete header. This breaks the header-before-payload verification boundary: Dash Core reads only the header before checking magic and size (https://github.com/dashpay/dash/blob/0364ed626e/src/net.cpp#L708-L746). Generate this header's codec with a 24-byte buffering limit in the P2P layer, and add a push-decoder regression asserting Ready after 24 bytes with subsequent payload bytes left unconsumed. The existing framing tests call BaseCodec::decode, bypassing the affected generated Decode implementation.

source: gpt-6.1-sol (phase2-reviewer: general)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No longer applies (re-reviewed at 31a4a44d): Your current head contains neither v1.rs nor V1Header and exposes only BIP324 framing, so the affected decoder is no longer present. Both are also absent from this PR's base, making the earlier V1 finding inapplicable to the current review range.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved (re-reviewed at ad130ec0): Your current head removes the V1 module and public exports that existed at the previously reviewed head. The affected header push decoder is no longer present or reachable, so no decoder correction remains necessary.

Comment thread pkgs/p2p_core/src/msg/cfilter.rs Outdated
Comment on lines +165 to +168
prev_filter_header: Hash256,
/// The filter hashes, one per block.
#[getset(get = "pub")]
filter_hashes: Vec<Hash256>,

@thepastaclaw thepastaclaw Nov 5, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Resolved at 31a4a44d; see the replies below.

🟡 Suggestion: Distinguish filter hashes from filter headers with newtypes

CFHeaders::prev_filter_header, CFHeaders::filter_hashes, and CFCheckpt::filter_headers all use Hash256, so checkpoint headers can be passed as filter hashes without a compiler error. Collecting CFilter::hash() results also satisfies filter_hashes, despite that method explicitly hashing the entire payload rather than the encoded filter. These are distinct protocol domains: Core hashes the encoded filter for its filter hash and combines that hash with the previous header for its filter header (https://github.com/dashpay/dash/blob/0364ed626e/src/blockfilter.cpp#L254-L261). Introduce separate FilterHash and FilterHeader newtypes in the owning module and use them across both responses. This follows the domain-safety guidance in AGENTS.md and docs/dev/guide_rust.md without changing wire encoding; the sister rust-dashcore implementation already uses these distinct types in dash/src/network/message_filter.rs.

source: gpt-6.1-sol (phase2-reviewer: rust-quality)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Resolved (re-reviewed at 31a4a44d): You now define and publicly export distinct FilterHash and FilterHeader newtypes and use them consistently in CFHeaders and CFCheckpt. Their generated codecs retain the 32-byte wire representation while preventing accidental interchange of the two domains.

@github-actions github-actions Bot removed the status:behind This pull request can be rebased on the base branch. label Nov 5, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review — Final validation — Phase 1 + Phase 2

Static verification of the complete diff at 31a4a44 found no actionable in-scope defects; the new wire layouts, decoding bounds, and Golomb-Rice validation match the inspected Dash Core reference. The hash-domain finding is fixed, and the earlier V1 decoder finding does not apply to this BIP324-only range. No builds or tests were run locally; current CI reports successful P2P MSRV, Linux, documentation, and Linux cross-build checks, with lint, macOS, Windows, and remaining cross-build checks pending and no failed checks reported.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The change adds substantial peer-facing network deserialization across ten message types, including bounded bloom payload decoding in pkgs/p2p_core/src/msg/bloom.rs and intricate Golomb-Rice validation in check_gcs and CFilter::decode in pkgs/p2p_core/src/msg/cfilter.rs.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

@PastaPastaPasta
PastaPastaPasta marked this pull request as ready for review November 5, 2026 13:43
@PastaPastaPasta
PastaPastaPasta requested a review from kwvg November 5, 2026 13:43
@github-actions github-actions Bot added the status:behind This pull request can be rebased on the base branch. label Nov 5, 2026

@kwvg kwvg left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You may want to look at the old implementation, though it might not be useful now.

What is useful is trying to keep our impl as close to bitcoin-p2p-messages as it appears to be in the pipeline for a release (see rust-bitcoin/rust-bitcoin#7161), decide between deferring this PR for that crate and undo'ing base-sdk#39 or aligning with upstream semantics and TODO'ing a substitution.

@PastaPastaPasta

Copy link
Copy Markdown
Member Author

Thanks, we took the align-and-TODO path. The types now follow bitcoin-p2p-messages as it is on rust-bitcoin master (94c6f791): its names, fields, field order and decode semantics. Each module has a TODO(pasta) for the swap, so the swap is one module per message family, and nothing here waits on an unreleased crate.

The main semantic changes:

  • cfilter: the filter is raw bytes of any filter type. There's no type check and no Golomb-Rice check any more.
  • filterload flags: they're a closed BloomFlags (0, 1, 2), so any other byte fails to decode.
  • Limits: they follow upstream's 4,000,000 bound on every byte and typed vector, each named as an associated const. That replaces 36,000 for the filterload filter and 520 for the filteradd element.
  • Fields: PartialMerkleTree now holds bits: Vec<bool>, MerkleBlock holds txn, the requests hold start_height: BlockHeight, and CFHeaders holds previous_filter_header.

The Dash block header and hash types stay, so MerkleBlock stays ours after the swap. No valid message encodes to different bytes, and the description lists each type against upstream, the limits and the decode changes.


🤖 Posted autonomously by Claude on behalf of pasta.

@PastaPastaPasta
PastaPastaPasta requested a review from kwvg November 5, 2026 18:16
@github-actions github-actions Bot removed the status:behind This pull request can be rebased on the base branch. label Nov 5, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review — Final validation — Phase 1 + Phase 2

Static review of the complete base-to-head diff at 956021b found no actionable in-scope defects: the wire layouts match the inspected reference serializers, and the documented decoding differences match the selected upstream Rust implementation. The filter-hash/header distinction is fixed, and the prior V1 decoder finding does not apply to the current BIP324-only source. Current CI reports successful lint, build/test, stable-platform, and cross-target checks; no builds or tests were run locally.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The substantial new BaseCodec implementations in pkgs/p2p_core/src/msg/bloom.rs and pkgs/p2p_core/src/msg/cfilter.rs change peer-facing network deserialization across ten BIP37 and BIP157 messages.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

@github-actions github-actions Bot added the status:behind This pull request can be rebased on the base branch. label Nov 6, 2026
@github-actions github-actions Bot added status:needs-rebase This pull request conflicts with the base branch and needs to be rebased. and removed status:behind This pull request can be rebased on the base branch. labels Nov 6, 2026
Types follow `bitcoin-p2p-messages`: a closed `BloomFlags`, the merkle
tree held as bits, and 4,000,000 bounds. Vectors come from the reference
implementation.
Types follow `bitcoin-p2p-messages`: a raw `cfilter` of any type,
`BlockHeight` start heights, 4,000,000 bounds and own filter hash and
header types. Vectors come from the reference implementation.
@github-actions github-actions Bot removed the status:needs-rebase This pull request conflicts with the base branch and needs to be rebased. label Nov 6, 2026

@thepastaclaw thepastaclaw left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Re-review — Final validation — Phase 1 + Phase 2

Static verification of the complete diff at ad130ec found no actionable in-scope defects. The typed messages match the inspected reference layouts and documented upstream decoding choices, and both prior findings are addressed. No local builds or tests were run; current CI reports successful build, test, lint, and cross-platform checks, with the PastaClaw review still pending.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 4: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 5: gpt-6-astra (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

  • Triage: critical by gpt-6.1-sol (effort low) — The large change introduces peer-facing network deserialization for ten BIP37 and BIP157 messages in pkgs/p2p_core/src/msg/bloom.rs and pkgs/p2p_core/src/msg/cfilter.rs, clearly meeting both the size and critical-surface criteria.
  • Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer, muse-spark-1.3-contributor — rust-quality (completed, effort xhigh); agent phase1-reviewer
  • Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity quota lookup failed), glm-5.3-flash (not used above high effort; tier asks max)
  • Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
  • Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
  • Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
  • Phase 2 reviewers: gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — general (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6-astra — security-auditor (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — rust-quality (completed, effort xhigh); agent phase2-reviewer, gpt-6.1-sol — security-auditor (completed, effort xhigh); agent phase2-reviewer
  • Model comparison: every Phase-2 reviewer also ran on gpt-6-astra; the verifier weighed both sets without knowing which model wrote which
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants