[kwvg]

Motivation

See base-sdk#89.

Breaking Changes

• Newtypes generated by make_num! no longer expose their inner integer. Construct them with new() or From, and read them with the new get() or Numeric::to_base().

Affecting dash-p2p-core:

Added

• CompressionState gains a public prev_block_hash() getter, as the hash is cached lazily and skipped by serde.

Changed

• GovSync.hash is now object_hash to avoid shadowing Hashable trait
• Inventory.hash is now inv_hash, pairing with inv_type to avoid shadowing Hashable trait
• DeletedQuorum.hash is now quorum_hash to avoid shadowing Hashable trait
• UserAgentTooLong.len has been renamed to size to avoid confusion with UserAgent::len().
• Affected types by change in make_num! behaviour are ProtocolVersion, ServiceFlags and ShortId
• Deserializing a UserAgent (and by extension, Version) now rejects values longer than 256 bytes due to stricter validation.

How Has This Been Tested?

just comb

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

Code generated with Claude Code using Opus 5.5, medium effort. Reviewed with Claude Code using Opus 5.5, medium effort.

[coderabbitai]

Caution

Review failed

The pull request is closed.

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: dashpay/base-sdk
• Branch: develop
• File: .coderabbit.yml
• Commit: 70606e8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: c121f549-f5a9-4b03-9249-83b2620bf9ea

Commits

Reviewing files that changed from the base of the PR and between 70606e8 and 8ce6ae9.

Files ignored due to path filters: 2

• Cargo.lock is excluded by !**/*.lock, !**/*.lock
• MODULE.bazel.lock is excluded by !**/*.lock, !**/*.lock

Files selected for processing: 21

• maint/codeql/rust/policy.model.yml
• pkgs/p2p_core/Cargo.toml
• pkgs/p2p_core/src/bip324.rs
• pkgs/p2p_core/src/codec.rs
• pkgs/p2p_core/src/macros.rs
• pkgs/p2p_core/src/msg/addr.rs
• pkgs/p2p_core/src/msg/blocks.rs
• pkgs/p2p_core/src/msg/coinjoin.rs
• pkgs/p2p_core/src/msg/gov.rs
• pkgs/p2p_core/src/msg/headers.rs
• pkgs/p2p_core/src/msg/headers2.rs
• pkgs/p2p_core/src/msg/inv.rs
• pkgs/p2p_core/src/msg/mn_list.rs
• pkgs/p2p_core/src/msg/ping.rs
• pkgs/p2p_core/src/msg/quorum_sigs.rs
• pkgs/p2p_core/src/msg/tx_reconcile.rs
• pkgs/p2p_core/src/msg/version.rs
• pkgs/p2p_core/src/short_id.rs
• pkgs/p2p_core/tests/addrv2.rs
• pkgs/types/CHANGELOG.md
• pkgs/types/src/numeric.rs

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Summary

Summary by CodeRabbit

• API Changes
  • P2P message fields and numeric identifiers are now accessed through getters and constructors rather than direct field access.
  • Oversized user-agent values are rejected during deserialization, with a 256-byte limit.
  • P2P message data continues to use the existing wire formats while providing updated access patterns.
• Documentation
  • Updated the types changelog with guidance for constructing and reading numeric values.

P2P numeric wrappers and message fields now use private storage with public accessors and constructors. The P2P codec macro accepts struct declarations and generates constructors and codec implementations. User-agent deserialization enforces the 256-byte limit. Header compression state provides a previous-block-hash accessor.

Suggested reviewers: thepastaclaw, pastapastapasta

Priority: Not assessed

Merge Risk: Low · up to 970f7

Repeated access can waste CPU for consumers using deserialized compression state; the published finding should still be addressed.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly summarizes the primary changes: restricting direct access to encoded values, applying the getter policy, and updating make_num! newtypes.
Description check		The description directly explains the breaking API changes, affected types, validation behavior, testing, and documentation updates in the changeset.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

• 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thepastaclaw]

🕓 Review not started yet because the new head is waiting for the 30-minute push debounce.

• Request normal review — click when the PR is ready for review.
• Request priority review — click to move this review to the front of the queue.

Commit 970f773. Normal review starts when eligible; priority review starts as soon as a slot is available.

[github-actions]

Warning

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

Potential conflicts

• p2p_core%fix!: align handshake and header sync messages with the reference implementation #86

[thepastaclaw]

Final validation — Phase 1 + Phase 2

The refactor preserves the P2P wire field order, encoding widths, payload bounds, and serde names, so I found no blocking correctness or security issues. Several public API and test-quality concerns remain: ShortId now exposes consensus codec and hashing traits despite being a BIP324 framing-only type, vector-bearing messages no longer provide ownership-consuming accessors, and the new UserAgent serde limit lacks a negative test. The required Breaking Changes section is also still a placeholder; relevant CI checks are queued or in progress with no reported code failure.

🟡 4 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Document the breaking API changes before merging
PR description:9-11

This PR is marked breaking and changes public construction and access patterns throughout p2p_core: struct literals now require generated constructors, direct field reads require getters, tuple newtypes require new()/get(), and several Rust field names changed while retaining their serde names. The Breaking Changes section still contains only _Work in progress._, so downstream users have no migration guidance. List the affected p2p_core types and the corresponding constructor/getter replacements before merging, including the renamed hash fields and UserAgentTooLong.size.

_{source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general)}

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: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 4: muse-spark-1.3-contributor (agent: phase1-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) — This is a large cross-cutting change to peer-facing P2P message encoding and deserialization across pkgs/p2p_core/src/codec.rs and numerous pkgs/p2p_core/src/msg/*.rs files.
• 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 `PR description`:
- [SUGGESTION] PR description:9-11: Document the breaking API changes before merging
  This PR is marked breaking and changes public construction and access patterns throughout p2p_core: struct literals now require generated constructors, direct field reads require getters, tuple newtypes require `new()`/`get()`, and several Rust field names changed while retaining their serde names. The Breaking Changes section still contains only `_Work in progress._`, so downstream users have no migration guidance. List the affected p2p_core types and the corresponding constructor/getter replacements before merging, including the renamed hash fields and `UserAgentTooLong.size`.

In `pkgs/p2p_core/src/short_id.rs`:
- [SUGGESTION] pkgs/p2p_core/src/short_id.rs:34-39: Keep ShortId unencodable instead of using make_num!
  ShortId was previously a private-field-free BIP324 framing hint explicitly marked `Unencodable`. Replacing it with `make_num!` generates `BaseCodec`, `Encode`, `Decode`, `TypeId`, and serde implementations, and `hash_impl!(ShortId)` adds a consensus-style SHA256d hash operation. The BIP324 call sites only need `new()`, `get()`, and the existing table lookup; this change unnecessarily presents a transport-only identifier as a consensus-encodable and hashable type. Keep a private `u8` field with manual `new()`/`get()` and the `Numeric` conversions needed by the framing code, while retaining `Unencodable` and removing the hash implementation.

In `pkgs/p2p_core/src/msg/headers.rs`:
- [SUGGESTION] pkgs/p2p_core/src/msg/headers.rs:55-60: Add consuming accessors for vector-bearing messages
  Privatizing the message fields removes the ability to move owned vectors directly out of decoded messages. `Headers`, `Headers2`, `GetData`, `Inv`, `NotFound`, `AddrV2Msg`, and the vector-bearing fields of `MnListDiffPayload` now expose only borrowed getters, so callers that need ownership must clone potentially large allocations even though they own the message. Add `into_headers`, `into_inventory`, and equivalent consuming accessors alongside the borrowed getters to preserve an allocation-free ownership path.

In `pkgs/p2p_core/src/msg/version.rs`:
- [SUGGESTION] pkgs/p2p_core/src/msg/version.rs:158-163: Test rejection of overlong UserAgent values through serde
  The custom `Deserialize` implementation now enforces the 256-byte limit, but the tests only round-trip valid values and construct them through the private tuple field, bypassing `UserAgent::new`. Add a negative test that verifies both `UserAgent::new(vec![...; MAX_USER_AGENT + 1])` and JSON deserialization reject an overlong value. This directly covers the new validation branch and avoids teaching callers to bypass the constructor.

[thepastaclaw]

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

The refactor preserves the wire formats, validation bounds, and documented JSON compatibility, and the supplied build, test, and lint checks pass for the exact head. One performance regression remains: CompressionState::prev_block_hash computes the hash on a cold cache without storing it, so repeated calls after serde deserialization recompute the block-header hash.

🟡 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); 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) — This is a large cross-cutting change that directly modifies peer-facing network deserialization and validation in files such as pkgs/p2p_core/src/codec.rs and pkgs/p2p_core/src/msg/version.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 each finding against the current code and only fix it if needed.

In `pkgs/p2p_core/src/msg/headers2.rs`:
- [SUGGESTION] pkgs/p2p_core/src/msg/headers2.rs:64-67: Preserve lazy caching in CompressionState::prev_block_hash
  The new immutable getter returns a computed hash when `prev_block_hash` is absent but never stores that result. Serde intentionally skips `prev_block_hash` while restoring `prev_header`, so repeated calls on a deserialized `CompressionState` recompute the full block-header hash instead of warming the cache as the previous private `prev_hash(&mut self)` helper did. Make the accessor mutable and restore the lazy assignment, or provide an equivalent caching path.

[thepastaclaw]

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

The refactor preserves wire field order, serialization names, decoding bounds, and BIP324 framing behavior. One performance regression remains: the new immutable CompressionState getter recomputes a skipped serde cache value without storing it, so repeated cold-cache lookups redo the header hash. CI checks were still queued or in progress with no reported code-check failures.

🟡 1 suggestion(s)

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

Review provenance

Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); 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) — This is a large cross-cutting change to peer-facing network serialization and deserialization, including pkgs/p2p_core/src/msg/version.rs and codec paths that alter validation and accepted wire values.
• 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 each finding against the current code and only fix it if needed.

In `pkgs/p2p_core/src/msg/headers2.rs`:
- [SUGGESTION] pkgs/p2p_core/src/msg/headers2.rs:64-67: Preserve lazy caching in CompressionState::prev_block_hash
  (existing thread: https://github.com/dashpay/base-sdk/pull/92#discussion_r4402099059)
  The new public getter takes `&self` and computes `prev_header.hash()` when `prev_block_hash` is empty, but it cannot store the result. Because `prev_block_hash` is skipped by serde, a deserialized state can retain `prev_header` with an empty cache; repeated calls then recompute the header hash instead of preserving the previous lazy-cache behavior. Restore a mutating cache path, either by making this accessor mutable or by retaining a private `&mut self` helper for the existing `decode_header` and `encode_header` call sites.