sdk%feat: switch from {En,De}codable to {En,De}code, replace free-function API with Recipient enum, adopt bitcoin_p2p_messages crate types into p2p_core - #18
Merged
Conversation
📝 WalkthroughWalkthroughThe change centralizes Rust Bitcoin dependencies, updates codec traits from ChangesP2P and codec integration
Sequence Diagram(s)sequenceDiagram
participant BIP324
participant ShortId
participant P2pMsg
participant PayloadCodec
BIP324->>ShortId: Resolve short command ID
ShortId->>P2pMsg: Return command mapping
BIP324->>P2pMsg: Decode payload
P2pMsg->>PayloadCodec: Decode typed or stub payload
PayloadCodec-->>P2pMsg: Return decoded message
P2pMsg-->>BIP324: Return P2pMsg
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
|
Note This pull request has no conflicts! 🎊 🎉 🎊 |
5 tasks
5 tasks
This was referenced Jun 28, 2026
github-actions
Bot
added
the
needs-rebase
github-actions
Bot
removed
the
needs-rebase
github-actions
Bot
added
the
needs-rebase
This was referenced Aug 2, 2026
kwvg
changed the title
{En,De}codable to {En,De}code, house KeyId in dash-script, adopt bitcoin_p2p_messages crate types into p2p_core{En,De}codable to {En,De}code, replace free-function API with Recipient enum, adopt bitcoin_p2p_messages crate types into p2p_core
github-actions
Bot
added
needs-rebase
github-actions
Bot
removed
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkgs/p2p_core/src/bip324.rs (1)
16-28: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReject oversized outbound payloads.
decode_payloadrejects payloads aboveMAX_P2P_PAYLOAD_SIZE.encode_v2emits them without validation. A publicP2pMsgwith an oversized encoded payload produces bytes that this crate rejects during decoding.Validate the payload before appending it. Return an encode error when it exceeds the protocol limit.
🤖 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 `@pkgs/p2p_core/src/bip324.rs` around lines 16 - 28, Update encode_v2 to validate the encoded payload size against MAX_P2P_PAYLOAD_SIZE before appending it, and return the established encode error when the limit is exceeded. Adjust the function’s return type and callers as needed while preserving the existing short/long command encoding for valid payloads.
🧹 Nitpick comments (5)
pkgs/script/src/addrs.rs (2)
95-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the P2PK address mapping.
to_base58cconvertsPubKeyto the pubkey-hash address of the key. The conversion is therefore lossy:from_base58creturnsPubKeyHashfor that address, notPubKey. The current Rustdoc mentions only theUnspendablecase. State the P2PK behavior so callers do not assume a variant-preserving round trip.📝 Proposed doc update
/// Encode as a Base58Check address. /// + /// `PubKey` encodes as the pubkey-hash address of the key, so decoding the + /// result returns `PubKeyHash`. + /// /// Returns `None` for `Unspendable`. pub fn to_base58c(&self, params: &AddrParams) -> Option<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 `@pkgs/script/src/addrs.rs` around lines 95 - 105, Update the Rustdoc for to_base58c in the address type to document that PubKey is encoded as the corresponding pubkey-hash address and that from_base58c reconstructs PubKeyHash rather than PubKey. Keep the existing Unspendable behavior documentation.
275-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend
to_base58c_addresswith P2PKH and P2PK cases.The case list covers P2SH and OP_RETURN only. The P2PKH path and the lossy P2PK path are the two remaining branches of
to_base58c. Add a case forP2PKHand a case for a P2PK script so the pubkey-hash derivation is pinned by a test.🤖 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 `@pkgs/script/src/addrs.rs` around lines 275 - 284, Extend the `to_base58c_address` rstest cases with one valid P2PKH script and one P2PK script, including their expected mainnet Base58Check addresses. Ensure the P2PK case verifies the lossy pubkey-hash derivation path while preserving the existing P2SH and OP_RETURN coverage.pkgs/p2p_core/src/msg/version.rs (1)
107-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider exposing
newasTryFrom<Vec<u8>>.
UserAgent::newis a fallible conversion fromVec<u8>. The coding guidelines ask forFromorTryFromimpls for conversions. Add aTryFrom<Vec<u8>>impl that delegates tonew. Callers then get the standard conversion entry point, andnewcan stay.As per coding guidelines: "Implement
FromorTryFromrather than implementingIntodirectly."♻️ Proposed addition
impl TryFrom<Vec<u8>> for UserAgent { type Error = UserAgentTooLong; fn try_from(bytes: Vec<u8>) -> Result<Self, Self::Error> { Self::new(bytes) } }🤖 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 `@pkgs/p2p_core/src/msg/version.rs` around lines 107 - 118, Implement TryFrom<Vec<u8>> for UserAgent, defining UserAgentTooLong as the associated error and delegating conversion to UserAgent::new. Keep the existing new constructor and its length validation unchanged.Source: Coding guidelines
pkgs/p2p_core/src/msg/headers2.rs (1)
89-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the visibility of
decode_headerandencode_header.
encode_headerispubanddecode_headerispub(crate).CompressionStateis re-exported frompkgs/p2p_core/src/msg/mod.rsat line 32. A downstream user can therefore compress a header stream but cannot decompress one. The Rustdoc on the type describes a symmetric per-message state machine.Choose one visibility for both methods.
Also applies to: 153-153
🤖 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 `@pkgs/p2p_core/src/msg/headers2.rs` at line 89, Align the visibility of CompressionState’s decode_header and encode_header methods so both use the same public API visibility. Update decode_header from pub(crate) to match encode_header’s pub visibility, preserving the symmetric compression/decompression interface exposed through the re-exported CompressionState.pkgs/p2p_core/src/msg/mod.rs (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRe-export the external payload types used by public
P2pMsgvariants.Lines 23-26 import
CFilter,CFHeaders,CFCheckpt,GetCFilters,GetCFHeaders,GetCFCheckpt,FilterAdd,FilterLoad,SendCmpct,GovObject, andGovVote. Thedefine_p2p!table places each one inside a publicP2pMsgvariant. The re-export block at lines 29-36 does not list them.A downstream user can then match on
P2pMsg::CFilter(..)but cannot nameCFilterwithout depending onbitcoin_p2p_messagesdirectly and pinning a matching version. Add these types to the public re-exports.Run the following script to check whether the crate root already re-exports these types:
#!/bin/bash # Check the p2p_core public surface for the external payload types. set -uo pipefail echo "== lib.rs public surface ==" ast-grep outline pkgs/p2p_core/src/lib.rs --items all echo "== pub use lines in p2p_core ==" rg -n --type=rust 'pub use' pkgs/p2p_core/src echo "== references to the external payload types ==" rg -n --type=rust -e 'CFilter' -e 'FilterLoad' -e 'SendCmpct' -e 'GovObject' -e 'GovVote' pkgs/p2p_core/src🤖 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 `@pkgs/p2p_core/src/msg/mod.rs` around lines 22 - 27, Extend the public re-export block in msg/mod.rs to include CFilter, CFHeaders, CFCheckpt, GetCFilters, GetCFHeaders, GetCFCheckpt, FilterAdd, FilterLoad, SendCmpct, GovObject, and GovVote. Re-export each from its existing external crate alongside the corresponding imports so all payload types used by public P2pMsg variants are directly nameable by downstream users.
🤖 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 `@pkgs/p2p_core/src/msg/headers2.rs`:
- Around line 269-280: Prevent silent header loss by making Headers2
construction validate that the input count does not exceed MAX_HEADERS, while
keeping the headers field private. Add a validating constructor modeled on
UserAgent::new, update callers to use it, and document the truncation behavior
in Headers2 Rustdoc only if truncation remains intentional.
- Around line 41-86: Make CompressionState’s derived prev_block_hash invariant
unbreakable by making prev_header private and exposing accessor/setter methods,
with set_prev_header invalidating the cached hash; provide a read-only
version_cache accessor if needed by callers. Update all direct field access,
including encode_header and equality/hash-related usage, to use the accessors,
ensuring states with identical prev_header remain equal regardless of cache
warmth.
- Around line 118-135: Update the compressed timestamp and nBits branches in the
header decoder to return a DecodeError when prev_header is None, instead of
converting the delta or defaulting bits to zero. Preserve the existing
predecessor-based decoding for Some(prev), and use the most appropriate existing
DecodeError variant or add a dedicated missing-predecessor variant if necessary.
In `@pkgs/p2p_core/src/msg/inv.rs`:
- Around line 40-45: Verify the `CompactBlock` discriminant in the inventory
enum against the Dash `GetDataMsg` numbering and update it to the correct Dash
value if the surrounding `GovernanceObject` entries establish that mapping.
Ensure legacy InstantSend txlock and compact-block values are not conflated, and
update any related mappings or fixtures identified by the repository search.
In `@pkgs/p2p_core/src/msg/version.rs`:
- Around line 154-168: Update the UserAgent Serialize and Deserialize
implementations to make arbitrary decoded bytes round-trip without errors:
serialize valid UTF-8 as the existing string form, but encode non-UTF-8 bytes
using a clearly marked fallback representation, and have Deserialize recognize
and decode that marker while preserving ordinary strings. Enforce the existing
256-byte UserAgent bound for both normal and fallback inputs, including
rejecting oversized decoded fallback data.
In `@pkgs/pkc/src/ecdsa/secret_ops.rs`:
- Line 331: Verify the workspace-resolved bitcoin-consensus-encoding version,
then update the decoder flow in consensus_bridge_roundtrip to match
Decoder::push_bytes(...).unwrap() against DecoderStatus::Ready or
DecoderStatus::NeedsMore instead of treating it as a boolean; reject the
NeedsMore/trailing-bytes case before invoking Decoder::end(), while preserving
successful decoding for Ready.
In `@pkgs/types/Cargo.toml`:
- Around line 11-16: Update the bitcoin-p2p-messages feature definition in the
crate’s Cargo.toml to explicitly include this crate’s std feature, while
preserving the no_std feature matrix with default = [] and full = ["std"].
Verify the workspace dependency disables upstream default features and enables
the required alloc configuration so adapters::message_filter resolves only when
both the dependency and std are enabled.
In `@pkgs/types/src/adapters.rs`:
- Around line 42-44: Update the import inside the bitcoin_p2p_messages module to
explicitly qualify the external dependency with a leading ::, changing the path
used by FilterHash and FilterHeader while leaving the module declaration and
feature gate unchanged.
In `@pkgs/types/src/entity.rs`:
- Around line 269-270: Update both macro expansions defining the bitcoin
consensus Encode implementation—at the visible type Encoder<'e> declarations and
the corresponding impl_stype! expansion—to add the required where Self: 'e GAT
lifetime bound, preserving the existing VecEncoder type and all other generated
behavior.
---
Outside diff comments:
In `@pkgs/p2p_core/src/bip324.rs`:
- Around line 16-28: Update encode_v2 to validate the encoded payload size
against MAX_P2P_PAYLOAD_SIZE before appending it, and return the established
encode error when the limit is exceeded. Adjust the function’s return type and
callers as needed while preserving the existing short/long command encoding for
valid payloads.
---
Nitpick comments:
In `@pkgs/p2p_core/src/msg/headers2.rs`:
- Line 89: Align the visibility of CompressionState’s decode_header and
encode_header methods so both use the same public API visibility. Update
decode_header from pub(crate) to match encode_header’s pub visibility,
preserving the symmetric compression/decompression interface exposed through the
re-exported CompressionState.
In `@pkgs/p2p_core/src/msg/mod.rs`:
- Around line 22-27: Extend the public re-export block in msg/mod.rs to include
CFilter, CFHeaders, CFCheckpt, GetCFilters, GetCFHeaders, GetCFCheckpt,
FilterAdd, FilterLoad, SendCmpct, GovObject, and GovVote. Re-export each from
its existing external crate alongside the corresponding imports so all payload
types used by public P2pMsg variants are directly nameable by downstream users.
In `@pkgs/p2p_core/src/msg/version.rs`:
- Around line 107-118: Implement TryFrom<Vec<u8>> for UserAgent, defining
UserAgentTooLong as the associated error and delegating conversion to
UserAgent::new. Keep the existing new constructor and its length validation
unchanged.
In `@pkgs/script/src/addrs.rs`:
- Around line 95-105: Update the Rustdoc for to_base58c in the address type to
document that PubKey is encoded as the corresponding pubkey-hash address and
that from_base58c reconstructs PubKeyHash rather than PubKey. Keep the existing
Unspendable behavior documentation.
- Around line 275-284: Extend the `to_base58c_address` rstest cases with one
valid P2PKH script and one P2PK script, including their expected mainnet
Base58Check addresses. Ensure the P2PK case verifies the lossy pubkey-hash
derivation path while preserving the existing P2SH and OP_RETURN coverage.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b04e85ee-47eb-488d-9613-a3e3a2368bff
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockcontrib/samples/Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (54)
Cargo.tomlcontrib/codeql/lib/policy.qllcontrib/samples/Cargo.tomlcontrib/samples/solver/Cargo.tomlpkgs/dev/Cargo.tomlpkgs/dev/src/lambda.rspkgs/num/Cargo.tomlpkgs/num/src/util.rspkgs/p2p_core/Cargo.tomlpkgs/p2p_core/src/bip324.rspkgs/p2p_core/src/codec.rspkgs/p2p_core/src/command.rspkgs/p2p_core/src/error.rspkgs/p2p_core/src/lib.rspkgs/p2p_core/src/macros.rspkgs/p2p_core/src/magic.rspkgs/p2p_core/src/msg/addr.rspkgs/p2p_core/src/msg/compact_filters.rspkgs/p2p_core/src/msg/headers.rspkgs/p2p_core/src/msg/headers2.rspkgs/p2p_core/src/msg/inv.rspkgs/p2p_core/src/msg/mn_list.rspkgs/p2p_core/src/msg/mod.rspkgs/p2p_core/src/msg/version.rspkgs/p2p_core/src/prelude.rspkgs/p2p_core/src/primitives/command.rspkgs/p2p_core/src/primitives/compressed_header.rspkgs/p2p_core/src/primitives/inventory.rspkgs/p2p_core/src/primitives/mod.rspkgs/p2p_core/src/primitives/service_flags.rspkgs/p2p_core/src/primitives/short_id.rspkgs/p2p_core/src/primitives/user_agent.rspkgs/p2p_core/src/serialize.rspkgs/p2p_core/src/short_id.rspkgs/p2p_core/src/version.rspkgs/params/Cargo.tomlpkgs/pkc/Cargo.tomlpkgs/pkc/src/ecdsa/secret_ops.rspkgs/primitives/Cargo.tomlpkgs/primitives/src/codec.rspkgs/primitives/src/payload/assetlock.rspkgs/primitives/src/payload/proregtx.rspkgs/primitives/src/payload/proupregtx.rspkgs/script/Cargo.tomlpkgs/script/src/addrs.rspkgs/script/src/lib.rspkgs/script/src/prelude.rspkgs/script/src/sigops.rspkgs/types/Cargo.tomlpkgs/types/src/adapters.rspkgs/types/src/entity.rspkgs/types/src/hex.rspkgs/types/src/macros.rspkgs/types/src/uint.rs
💤 Files with no reviewable changes (8)
- pkgs/p2p_core/src/msg/compact_filters.rs
- pkgs/p2p_core/src/primitives/inventory.rs
- pkgs/p2p_core/src/primitives/service_flags.rs
- pkgs/p2p_core/src/primitives/user_agent.rs
- pkgs/p2p_core/src/primitives/mod.rs
- pkgs/p2p_core/src/primitives/command.rs
- pkgs/p2p_core/src/primitives/short_id.rs
- pkgs/p2p_core/src/primitives/compressed_header.rs
Comment on lines
+41
to
+86
| pub struct CompressionState { | ||
| /// MRU version cache (front = most recently used). | ||
| pub version_cache: Vec<i32>, | ||
| /// Previous fully-resolved header. | ||
| pub prev_header: Option<BlockHeader>, | ||
| /// Cached block hash of `prev_header`. | ||
| #[cfg_attr(feature = "serde", serde(skip))] | ||
| prev_block_hash: Option<BlockHash>, | ||
| } | ||
|
|
||
| impl CompressionState { | ||
| /// Creates fresh state with an empty cache and no previous header. | ||
| pub fn new() -> Self { | ||
| Self { | ||
| version_cache: Vec::with_capacity(MAX_VERSION_CACHE), | ||
| prev_header: None, | ||
| prev_block_hash: None, | ||
| } | ||
| } | ||
|
|
||
| /// Moves the version at `position` to the front of the cache. | ||
| fn mark_version_mru(&mut self, position: usize) { | ||
| let v = self.version_cache.remove(position); | ||
| self.version_cache.insert(0, v); | ||
| } | ||
|
|
||
| /// Inserts `version` at the front, evicting the oldest if full. | ||
| fn save_version_mru(&mut self, version: i32) { | ||
| if self.version_cache.len() >= MAX_VERSION_CACHE { | ||
| self.version_cache.pop(); | ||
| } | ||
| self.version_cache.insert(0, version); | ||
| } | ||
|
|
||
| /// Finds the cache position (0-based) for a version, if cached. | ||
| fn find_version(&self, version: i32) -> Option<usize> { | ||
| self.version_cache.iter().position(|&v| v == version) | ||
| } | ||
|
|
||
| /// Returns cached hash, recomputing from `prev_header` if the cache is cold. | ||
| fn prev_hash(&mut self) -> Option<BlockHash> { | ||
| if self.prev_block_hash.is_none() { | ||
| self.prev_block_hash = self.prev_header.as_ref().map(|h| h.hash()); | ||
| } | ||
| self.prev_block_hash | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make the cached hash invariant unbreakable.
version_cache and prev_header are public, but prev_block_hash is private and is a derived cache of prev_header. A caller can assign prev_header directly. prev_hash then returns the stale value, because it only recomputes when prev_block_hash is None. encode_header uses that value to decide FLAG_PREV_HASH, so a stale cache produces a wrong wire image.
The derived PartialEq, Eq, and Hash also include prev_block_hash. Two states with the same prev_header compare unequal when one cache is warm and the other is cold.
Make the fields private and expose accessors, or drop the cache field and compute the hash on demand.
🛠️ Sketch: private fields with a setter that invalidates the cache
pub struct CompressionState {
/// MRU version cache (front = most recently used).
- pub version_cache: Vec<i32>,
+ version_cache: Vec<i32>,
/// Previous fully-resolved header.
- pub prev_header: Option<BlockHeader>,
+ prev_header: Option<BlockHeader>,
/// Cached block hash of `prev_header`.
#[cfg_attr(feature = "serde", serde(skip))]
prev_block_hash: Option<BlockHash>,
}impl CompressionState {
/// Returns the MRU version cache.
pub fn version_cache(&self) -> &[i32] {
&self.version_cache
}
/// Returns the previous header.
pub fn prev_header(&self) -> Option<&BlockHeader> {
self.prev_header.as_ref()
}
/// Sets the previous header and invalidates the cached hash.
pub fn set_prev_header(&mut self, header: Option<BlockHeader>) {
self.prev_header = header;
self.prev_block_hash = None;
}
}🤖 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 `@pkgs/p2p_core/src/msg/headers2.rs` around lines 41 - 86, Make
CompressionState’s derived prev_block_hash invariant unbreakable by making
prev_header private and exposing accessor/setter methods, with set_prev_header
invalidating the cached hash; provide a read-only version_cache accessor if
needed by callers. Update all direct field access, including encode_header and
equality/hash-related usage, to use the accessors, ensuring states with
identical prev_header remain equal regardless of cache warmth.
Comment on lines
+118
to
+135
| let time = if flags & FLAG_TIMESTAMP_FULL != 0 { | ||
| u32::decode(sl)? | ||
| } else { | ||
| let delta = i16::decode(sl)?; | ||
| match &self.prev_header { | ||
| Some(prev) => (prev.time as i64 + delta as i64) as u32, | ||
| None => delta as u32, | ||
| } | ||
| }; | ||
|
|
||
| let bits = if flags & FLAG_NBITS != 0 { | ||
| u32::decode(sl)? | ||
| } else { | ||
| match &self.prev_header { | ||
| Some(prev) => prev.bits, | ||
| None => 0, | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject compressed headers that reference a missing predecessor.
Two decode branches invent values when prev_header is None.
Line 124: delta as u32 sign-extends a negative i16. A delta of -1 produces a time of 4294967295.
Line 133: bits becomes 0, which is not a valid difficulty target.
encode_header always sets FLAG_TIMESTAMP_FULL and FLAG_NBITS for the first header, so a conforming peer never emits these encodings. A malformed or hostile stream does. The decoder currently accepts it and produces a corrupt BlockHeader instead of an error.
Return a decode error in both branches.
🐛 Proposed fix
let time = if flags & FLAG_TIMESTAMP_FULL != 0 {
u32::decode(sl)?
} else {
let delta = i16::decode(sl)?;
- match &self.prev_header {
- Some(prev) => (prev.time as i64 + delta as i64) as u32,
- None => delta as u32,
- }
+ let prev = self.prev_header.as_ref().ok_or(DecodeError::InvalidValue {
+ expected: alloc::vec![1],
+ actual: 0,
+ })?;
+ (prev.time as i64 + delta as i64) as u32
};
let bits = if flags & FLAG_NBITS != 0 {
u32::decode(sl)?
} else {
- match &self.prev_header {
- Some(prev) => prev.bits,
- None => 0,
- }
+ self
+ .prev_header
+ .as_ref()
+ .ok_or(DecodeError::InvalidValue {
+ expected: alloc::vec![1],
+ actual: 0,
+ })?
+ .bits
};Use whichever DecodeError variant best expresses "missing predecessor"; add a dedicated variant if none fits.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let time = if flags & FLAG_TIMESTAMP_FULL != 0 { | |
| u32::decode(sl)? | |
| } else { | |
| let delta = i16::decode(sl)?; | |
| match &self.prev_header { | |
| Some(prev) => (prev.time as i64 + delta as i64) as u32, | |
| None => delta as u32, | |
| } | |
| }; | |
| let bits = if flags & FLAG_NBITS != 0 { | |
| u32::decode(sl)? | |
| } else { | |
| match &self.prev_header { | |
| Some(prev) => prev.bits, | |
| None => 0, | |
| } | |
| }; | |
| let time = if flags & FLAG_TIMESTAMP_FULL != 0 { | |
| u32::decode(sl)? | |
| } else { | |
| let delta = i16::decode(sl)?; | |
| let prev = self.prev_header.as_ref().ok_or(DecodeError::InvalidValue { | |
| expected: alloc::vec![1], | |
| actual: 0, | |
| })?; | |
| (prev.time as i64 + delta as i64) as u32 | |
| }; | |
| let bits = if flags & FLAG_NBITS != 0 { | |
| u32::decode(sl)? | |
| } else { | |
| self | |
| .prev_header | |
| .as_ref() | |
| .ok_or(DecodeError::InvalidValue { | |
| expected: alloc::vec![1], | |
| actual: 0, | |
| })? | |
| .bits | |
| }; |
🤖 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 `@pkgs/p2p_core/src/msg/headers2.rs` around lines 118 - 135, Update the
compressed timestamp and nBits branches in the header decoder to return a
DecodeError when prev_header is None, instead of converting the delta or
defaulting bits to zero. Preserve the existing predecessor-based decoding for
Some(prev), and use the most appropriate existing DecodeError variant or add a
dedicated missing-predecessor variant if necessary.
github-actions
Bot
removed
the
needs-rebase
kwvg
marked this pull request as ready for review
August 8, 2026 21:09
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkgs/p2p_core/src/msg/headers2.rs (1)
266-271: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winLimit the encoded header count to
MAX_HEADERS.
decoderejects counts aboveMAX_HEADERS, butencodewrites the fullself.headerslength. An out-of-contractHeaders2value therefore produces a payload that peers reject.Normalize the encoded slice before writing its
CompactSizecount.Proposed fix
fn encode(&self, buf: &mut impl EncodeBuf) { - CompactSize::from(self.headers.len()).encode(buf); + let headers = &self.headers[..self.headers.len().min(MAX_HEADERS)]; + CompactSize::from(headers.len()).encode(buf); let mut state = CompressionState::new(); - for h in &self.headers { + for h in headers { state.encode_header(h, buf); } }Based on learnings,
BaseCodecencoders must normalize or constrain out-of-contract values so the wire output remains valid.🤖 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 `@pkgs/p2p_core/src/msg/headers2.rs` around lines 266 - 271, Update Headers2::encode to cap the headers slice at MAX_HEADERS before encoding both the CompactSize count and header entries. Ensure the count matches the truncated slice and preserve the existing CompressionState encoding flow for the normalized headers.Source: Learnings
🤖 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 `@pkgs/types/src/serialize.rs`:
- Line 153: Update the deserialization call in the surrounding deserialize
implementation to branch on deserializer.is_human_readable(): use
deserialize_byte_buf(BytesVisitor) for binary formats and retain the existing
deserialize_any(BytesVisitor) path for human-readable formats.
- Line 145: Replace the Vec::with_capacity allocation using
SeqAccess::size_hint() with Vec::new() in the sequence deserialization path, or
otherwise enforce a safe fixed upper bound before reserving. Ensure elements
continue to be read and appended normally without allowing the untrusted hint to
trigger excessive allocation.
---
Outside diff comments:
In `@pkgs/p2p_core/src/msg/headers2.rs`:
- Around line 266-271: Update Headers2::encode to cap the headers slice at
MAX_HEADERS before encoding both the CompactSize count and header entries.
Ensure the count matches the truncated slice and preserve the existing
CompressionState encoding flow for the normalized headers.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23f27e29-59d2-4beb-9f02-44a3cffaaa3e
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lock,!**/*.lockcontrib/samples/Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (16)
pkgs/num/src/util.rspkgs/p2p_core/src/command.rspkgs/p2p_core/src/msg/addr.rspkgs/p2p_core/src/msg/headers.rspkgs/p2p_core/src/msg/headers2.rspkgs/p2p_core/src/msg/inv.rspkgs/p2p_core/src/msg/version.rspkgs/pkc/src/ecdsa/secret_ops.rspkgs/script/src/addrs.rspkgs/types/Cargo.tomlpkgs/types/src/adapters.rspkgs/types/src/entity.rspkgs/types/src/macros.rspkgs/types/src/secret.rspkgs/types/src/serialize.rspkgs/types/src/uint.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- pkgs/types/src/uint.rs
- pkgs/p2p_core/src/msg/headers.rs
- pkgs/types/Cargo.toml
- pkgs/num/src/util.rs
- pkgs/types/src/macros.rs
- pkgs/pkc/src/ecdsa/secret_ops.rs
- pkgs/types/src/adapters.rs
- pkgs/p2p_core/src/msg/version.rs
- pkgs/script/src/addrs.rs
- pkgs/p2p_core/src/command.rs
- pkgs/p2p_core/src/msg/addr.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Additional Information
Depends on sdk%feat(test): introduce
bsdk-utilwithbspcheckverb to validate againstblockchain.datlinearized chains #13Depends on pkc%feat(ecdsa): implement recoverable signatures API,
Ecdsa{Pk,Sk,RecSig}Bytesas canonical types,EcdsaSigBytesas raw type, splitKeyIdinto{PubKey,Script}Hashand usebitcoin_primitives::script#23Within Dash's inventory system, compact blocks are assigned ID
20(source), instead of Bitcoin's ID4(source). This has been corrected inInvType.rust-bitcoinrenamedEncodable/DecodabletoEncode/Decode(see rust-bitcoin#6028), which affects our ability to track and incorporate their latest releases (as we have converged on thebitcoin-crypto-0.2.0tag to start using theirbitcoin_p2p_messagesdefinitions), so we have followed through and propagated the change for ourselves as well. This required updating our CodeQL rules to recognise them correctly.Adoption of the
BaseCodecsystem from base-sdk#4 trimmed down line-count enough to let us reap thedash_p2p_core::primitivesmodule and re-distribute logic to the messages utilising them.The generated
decode_payloadnow validates payload size before attempting a decode:check_payloadrejects anything overMAX_P2P_PAYLOAD_SIZE, andcheck_emptyrejects a non-empty payload if the message is marked as expected empty.The littered implementation of
dash-scriptfree functions were good enough during Hyphen's initial prototyping but as the SDK is expected to present a palatable public API, it has been cleaned up and tucked behindRecipient, which behaves similar toCTxDestinationin Dash Core and was the reason why this pull request depended on base-sdk#23.dash_p2p_core::P2pMsgno longer implementscore::hash::Hash, so it cannot be used as aHashMap/HashSetkey. This was necessary to admit upstream payload types that do not derive it.Breaking Changes
legacy_sigop_countnow terminates the scan on a truncatedPUSHDATAheader instead of skipping it. Previously the truncated operand bytes were rescanned as opcodes and could be counted as sigops:4dacreturned 1 (the trailing0xacwas counted) and now returns 0.Unrecognised scripts return
Nonerather thanScriptKind::Unknown(leading_byte). The leading opcode is discarded.The 11 commands that must carry no payload (
verack,getaddr,sendaddrv2,sendheaders,sendheaders2,filterclear,mempool,getsporks,senddsq,qsendrecsigs,qwatch) now reject trailing bytes withP2pDecodeError::PayloadNotEmpty. Previously the bytes were silently discarded.Stub message payloads are now capped at
MAX_P2P_PAYLOAD_SIZE(3 MiB) and oversizedparsedpayloads now fail early with a structuredPayloadTooLarge { command, size, max }.The
commandfield ofPayloadTooLargeandPayloadNotEmptynow carries the lowercase wire name(
getcfilters) instead of the constant identifier (GETCFILTERS).P2pDecodeErrorgains aPayloadNotEmpty { command, size }variant. Exhaustive matches onP2pDecodeErrorwill need updating.Moved
dash_p2p_core::DashNetworkMessagehas been renamed todash_p2p_core::P2pMsg.{de,en}code_v2()change signatures to match.dash_p2p_core::{CFCheckpt, CFHeaders, CFilter, GetCFCheckpt, GetCFHeaders, GetCFilters}are no longer re-exported; they now come frombitcoin_p2p_messages::message_filter, which consumers must depend on directly.Removed
dash_script::ScriptKind(with variantsP2pkh,P2sh,P2pk,OpReturn,Unknown(u8))dash_script::{classify, is_p2pkh, is_p2sh, is_p2pk, is_op_return}dash_script::{p2pkh_hash160, p2sh_hash160}dash_script::{encode_p2pkh, encode_p2sh, derive_address}dash_script::opcodeis no longer a public module. Only the root re-exportdash_script::Opcoderemains.dash_p2p_core::FilterType(andFilterType::BASIC)dash_p2p_core::ShortId::is_valid_rangeSuperseded
bitcoin_consensus_encoding::{Encodable, Decodable}have been replaced by{Encode, Decode}. Every type built byimpl_type!/impl_stype!implements the new names, so downstream bounds and<T as Decodable>::decoder()calls must be updated.How Has This Been Tested?
Checklist