sdk%feat: introduce trait BaseCodec and {impl,codec}_* helper macros to reduce boilerplate, enforce create::prelude use, drop premature limits#4
Draft
kwvg wants to merge 29 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (91)
💤 Files with no reviewable changes (11)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (50)
📝 WalkthroughWalkthroughThis PR migrates numeric, byte, and message codecs onto ChangesCodec migration and wiring
CodeQL declaration checks
|
|
Note This pull request has no conflicts! 🎊 🎉 🎊 |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkgs/primitives/src/payload/cbtx.rs (1)
34-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not model version-required fields as silently skippable on encode.
For
version >= 2andversion >= 3, the wire format requires the quorum and ChainLock fields, butencode()only writes them when theOptions areSome. That makes states likeversion: 3with missing v3 fields serializable into truncated payloads, andvalidate()does not currently reject those combinations.As per coding guidelines, make invalid states unrepresentable.
Also applies to: 43-94, 120-141
🤖 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/primitives/src/payload/cbtx.rs` around lines 34 - 40, The struct currently models version-required fields (e.g., merkle_root_quorums, best_cl_height_diff, best_cl_signature, credit_pool_balance) as Option but encode() only writes them when Some, allowing invalid versioned payloads; change encode() to enforce presence for the required versions (return an encoding error if a required Option is None) and update validate() to reject missing required fields for version >=2/>=3; apply the same checks for the other version-gated Option fields referenced in the diff ranges (lines 43-94 and 120-141) so that encode() and validate() make those states unrepresentable rather than silently skipping them.pkgs/primitives/src/payload/mod.rs (1)
95-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire exact consumption for recognized payloads.
Unknownexplicitly drainsdata, but the recognized branches now succeed even when trailing bytes remain. That makesSpecialPayload::decodeaccept non-canonicalextra_payloadvalues unless every caller remembers to checkdata.is_empty()afterward. Please make the recognized paths fail on leftover bytes inside this method as well.🤖 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/primitives/src/payload/mod.rs` around lines 95 - 127, The decode function (pub fn decode) currently returns success for recognized TxType branches even if trailing bytes remain; change each recognized-branch path (e.g., ProviderRegister -> ProRegTx::decode, ProviderUpdateService -> ProUpServTx::decode, CoinbaseCommitment -> cbtx::CoinbaseCommitment::decode, QuorumCommitment -> FinalCommitment::decode, MnhfSignal -> MnHardFork::decode, AssetLock/AssetUnlock, etc.) to verify that all input bytes were consumed before returning Ok: after the inner decode succeeds and you map to the enum variant, check that *data is empty (or compute and clear remaining bytes) and if not return a PayloadError (use the existing err closure or construct a PayloadError with tx_type and a message about trailing bytes); keep the Unknown branch behavior of draining data. Ensure you use the same PayloadError type and tx_type for the failure path so callers don't need to re-check data.is_empty().pkgs/primitives/src/support.rs (1)
204-208:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
DynBitset::encodecan serialize states that its own decoder rejects.Because
num_bitsanddataare both public, callers can construct mismatched values. Lines 315-316 then writeself.dataverbatim, so a too-short buffer yields truncated payloads, a too-long buffer bleeds bytes into the next field, and non-zero padding bits survive on the wire even thoughdecoderejects them. Please canonicalize the emitted byte slice fromnum_bitshere, or make the fields private and enforce the invariant at construction time. As per coding guidelines, "Use newtypes over primitives, enums over booleans, make invalid states unrepresentable."Also applies to: 290-317
🤖 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/primitives/src/support.rs` around lines 204 - 208, The DynBitset currently exposes pub num_bits and pub data so encode can emit mismatched bytes that decode will reject; fix by canonicalizing the serialized payload in DynBitset::encode (or make fields private and validate at construction) — specifically, compute required_bytes = (num_bits + 7) / 8, then when serializing take exactly that many bytes (pad with 0x00 if data is shorter, truncate if longer) and clear any padding bits in the final byte beyond num_bits before writing; alternatively, change num_bits and data to private, add a new(fn new / from_parts) that enforces data length and zeroes padding bits so encode/decode invariants hold (referencing DynBitset, encode, decode, num_bits, data).
🟠 Major comments (20)
pkgs/num/src/prelude.rs-9-9 (1)
9-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix clippy failure from unused prelude import.
Line 9 currently triggers
unused importunder-D warnings, which blocks CI. Gate this re-export to the same feature path that needs it.Proposed fix
- pub(crate) use alloc::string::String; + #[cfg(feature = "serde")] + pub(crate) use alloc::string::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/num/src/prelude.rs` at line 9, The prelude re-export pub(crate) use alloc::string::String; is causing a Clippy unused-import failure; gate this re-export with the same cfg/feature used by the code that needs alloc (e.g. add the appropriate #[cfg(...)] or #[cfg(feature = "...")] attribute matching the crate's alloc-enabled feature) or remove it if nothing depends on it, so the import is only compiled when that feature/path is active.pkgs/p2p_core/src/msg/addr.rs-22-31 (1)
22-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the 1,000-entry cap for
addrandaddrv2.Both decoders now accept any vector length that fits under the generic payload budget. Dash's
addrmessage caps the list at 1,000 entries, and BIP155 saysaddrv2messages above 1,000 addresses should be rejected. Without an explicit count check here, oversized address gossip will be treated as valid input. (docs.dash.org)Decode the CompactSize count first, reject
> 1000, then read exactly that many entries.Also applies to: 44-53
🤖 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/addr.rs` around lines 22 - 31, The Addr decoder currently lets Vec::decode read any length; change BaseCodec::decode for Addr to first decode the CompactSize count (the length/count field), check if count > 1000 and return an error if so, then decode exactly that many address entries into addrs (don’t call Vec::decode directly on the remaining buffer); apply the same change to the corresponding impl (addrv2 decoder) so both decode methods validate the CompactSize count and reject >1000 before reading entries.pkgs/p2p_core/src/codec.rs-9-16 (1)
9-16:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe global payload cap is far below Dash's wire limit.
impl_p2p!now rejects every payload above 3 MiB, but Dash's message header still allows payloads up to 32 MiB. With this helper applied crate-wide, valid messages in the 3-32 MiB range will now fail deserialization before any message-specific validation runs. (docs.dash.org)Minimal fix
- pub(crate) const MAX_P2P_PAYLOAD: usize = 3_145_728; + pub(crate) const MAX_P2P_PAYLOAD: usize = 32 * 1024 * 1024;If some message types need tighter bounds, keep those as message-specific checks instead of lowering the global protocol ceiling.
🤖 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/codec.rs` around lines 9 - 16, The global MAX_P2P_PAYLOAD constant and impl_p2p! macro set a 3 MiB ceiling that rejects valid Dash wire messages up to 32 MiB; change the constant MAX_P2P_PAYLOAD from 3_145_728 to the Dash wire maximum 33_554_432 (32 * 1024 * 1024) so dash_types::impl_type! invoked by impl_p2p! uses the protocol ceiling, and keep any tighter, message-specific validation in the individual message implementations rather than lowering the global cap.pkgs/num/src/util.rs-13-26 (1)
13-26:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
#[macro_export]macro pathing fordash_typesinimpl_hash!
impl_hash!is#[macro_export]and emitsdash_types::...paths; those resolve in the caller crate.pkgs/num/src/lib.rsonly showspub mod __private { ... }and there’s nodash_types/__dash_typeshidden re-export in the checkedpkgs/numsources, so downstream may need an unnecessary directdash_typesdependency just to expand the macro.Suggested direction
- impl dash_types::codec::BaseCodec for $name { + impl $crate::__dash_types::codec::BaseCodec for $name { fn decode( data: &mut &[u8], ) -> Result<Self, dash_types::codec::DecodeError> { - dash_types::codec::take::<{ <$base>::LEN }>(data) + $crate::__dash_types::codec::take::<{ <$base>::LEN }>(data) .map(Self::from_bytes) } fn encode(&self, buf: &mut ::alloc::vec::Vec<u8>) { buf.extend_from_slice(self.as_bytes()); } } - dash_types::impl_type!($name); + $crate::__dash_types::impl_type!($name);Add a hidden re-export in
pkgs/num/src/lib.rs, e.g.pub use dash_types as __dash_types;, and route all emitteddash_types::...paths in these exported macros through$crate::__dash_types::....🤖 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/num/src/util.rs` around lines 13 - 26, The exported macro impl_hash! emits absolute dash_types::... paths which resolve in the caller crate; add a hidden re-export in this crate (e.g. pub use dash_types as __dash_types;) in pkgs/num/src/lib.rs and update the macro impl_hash! so every emitted dash_types::... reference is routed through $crate::__dash_types::... (also update any related exported macros like impl_type! / BaseCodec implementations to use $crate::__dash_types::codec::... etc.) so callers don’t need a direct dash_types dependency to expand the macro.pkgs/p2p_core/src/msg/govsync.rs-28-38 (1)
28-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReintroduce a bounded bloom-filter size cap for
GovSync::bloom_filter
GovSyncnow encodes/decodesbloom_filtervia the genericVec<u8>codec (bloom_filter: Vec::decode(data)?/self.bloom_filter.encode(buf)). TheVec<T>codec only limits length bydata.len()and usesMAX_VECTOR_ALLOCATEfor reserve batching, with no bloom-filter-specific maximum. Restore the priorMAX_BLOOM_FILTER-style guard or switch this field to a bounded blob/vector helper.🤖 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/govsync.rs` around lines 28 - 38, GovSync's bloom_filter currently uses the unbounded Vec codec; restore a size cap by validating length during decode and encode. In BaseCodec::decode for GovSync, replace Vec::decode(data)? with manual length-checking: read the vector length (or peek the remaining byte length), ensure it does not exceed MAX_BLOOM_FILTER, return a DecodeError if too large, then read the bytes into bloom_filter; likewise, in BaseCodec::encode for GovSync check self.bloom_filter.len() <= MAX_BLOOM_FILTER before encoding and return/handle an error if exceeded. Use the existing MAX_BLOOM_FILTER constant and reference the GovSync::decode, GovSync::encode, bloom_filter, and DecodeError symbols when making the changes.pkgs/p2p_core/src/msg/headers.rs-30-42 (1)
30-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound
locator_hashessize inGetHeaders
GetHeaderscurrently decodes/encodeslocator_hashesusing the genericVec<T>codec (Vec::decode(data)?), which only limits by remaining buffer length (and an allocation batching cap), not by a protocol/message max count—so oversizedgetheadersmessages are still possible. Replace this with a bounded decode for locator hashes (e.g., enforce aMAX_GETHEADERS_LOCATORS/reuse an existing protocol constant) and ensure encode respects the same bound.🤖 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/headers.rs` around lines 30 - 42, GetHeaders uses an unbounded Vec::decode/encode for locator_hashes; change decoding in BaseCodec::decode for GetHeaders to enforce a MAX_GETHEADERS_LOCATORS protocol constant (or reuse an existing constant) and return a DecodeError if the decoded count exceeds it, and similarly ensure BaseCodec::encode for GetHeaders checks the locator_hashes.len() against the same MAX_GETHEADERS_LOCATORS and fails/returns/error if too large; update the decode call for locator_hashes to a bounded-vec decode helper or manual length-prefixed loop that enforces the limit and reference the GetHeaders type, locator_hashes field, and the MAX_GETHEADERS_LOCATORS constant.pkgs/p2p_core/src/msg/cfheaders.rs-60-74 (1)
60-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd an explicit length bound for
CFHeaders.filter_hashes.
CFHeaderscurrently decodes/encodesfilter_hashesvia the genericBaseCodec for Vec<T>(Vec::decode/Vec::encode), whose count is bounded only bydata.len()(read_compact_size(data, data.len())). SwitchCFHeadersto usedash_types::codec::read_compact_size(data, <cfheaders_limit>)/write_compact_size(and then decode/encode that many hashes) so oversized hash lists are rejected/limited.pkgs/p2p_core/src/msg/cfheaders.rs:60-74; pkgs/types/src/codec.rs: impl
BaseCodec for Vec<T>(usesread_compact_size(data, data.len())).🤖 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/cfheaders.rs` around lines 60 - 74, CFHeaders' BaseCodec implementation decodes filter_hashes with the unbounded Vec::decode, allowing oversized lists; change CFHeaders::decode/encode to first read/write an explicit compact-size count using dash_types::codec::read_compact_size(data, <cfheaders_limit>) and dash_types::codec::write_compact_size(count, buf) (replace <cfheaders_limit> with the desired max), then loop decode that many BlockHash entries into filter_hashes (use BlockHash::decode) and likewise encode by writing the compact size then encoding each hash; update the CFHeaders::encode path to mirror this (write compact size then encode each filter_hash) so oversized lists are rejected.pkgs/p2p_core/src/msg/inv.rs-22-30 (1)
22-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReintroduce a bounded per-message inventory item cap for
Inv,GetData, andNotFoundAll three message codec impls serialize/deserialize
inventoryvia the genericVec<Inventory>codec, which provides no explicit max item count:Vec<T>::decodereads the element count withread_compact_size(data, data.len())?andencodewriteswrite_compact_size(self.len(), buf)(only effectively bounded by remaining bytes / allocation batching). This allows peers to accept and emit oversized inventory lists.Enforce
MAX_INV_ITEMS(or equivalent) at the vector boundary (i.e., a bounded vec helper or an explicit count limit) forInv,GetData, andNotFounddecode and encode.🤖 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/inv.rs` around lines 22 - 30, The Inv, GetData, and NotFound BaseCodec impls currently use Vec::decode/encode for the inventory field with no upper bound; add and enforce a MAX_INV_ITEMS cap when decoding and encoding each inventory vector: in each decode (e.g., Inv::decode, GetData::decode, NotFound::decode) read the compact-size count first, validate count <= MAX_INV_ITEMS before proceeding to decode items (or return a DecodeError), and in each encode (e.g., Inv::encode, GetData::encode, NotFound::encode) check that self.inventory.len() <= MAX_INV_ITEMS (or truncate/return an error) and write the compact-size with that validated count; you can implement a small bounded_vec helper or inline the count check around the existing Vec::decode/encode calls to enforce the limit.pkgs/p2p_core/src/primitives/command.rs-13-14 (1)
13-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
CommandStringASCII + null-padding invariant on decode/serde.
CommandStringderivesDeserialize, anddash_types::impl_bytes!(12, CommandString)generatesBaseCodec::decodethat just takes 12 raw bytes (take::<12>(data).map(Self::from)) with no ASCII/NUL validation. As a result, arbitrary[u8; 12]can deserialize/decode, contradictingas_str()’s “validated on decode” assumption and letting malformed commands persist (they get masked to""viafrom_utf8(...).unwrap_or("")).Add a validating constructor (
TryFrom<[u8;12]>/similar) that enforces allowed ASCII and correct null padding, then use it in bothDeserializeandBaseCodec::decodeforCommandString(manual impls or by extending the macro).🤖 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/primitives/command.rs` around lines 13 - 14, CommandString currently allows arbitrary bytes via the derived Deserialize and the macro-generated BaseCodec::decode (from dash_types::impl_bytes!(12, CommandString)), violating as_str()'s “validated on decode” assumption; add a validating constructor (implement TryFrom<[u8;12]> for CommandString) that enforces ASCII printable-range bytes before the first NUL and requires all trailing bytes after the first NUL to be zero, then replace the auto-derived Deserialize and the macro-generated BaseCodec::decode for CommandString with manual impls that call this TryFrom constructor (or otherwise invoke the validation) so deserialization/decoding fails on invalid bytes and only validated, null-padded commands are constructed.pkgs/p2p_core/src/primitives/mn_list.rs-209-210 (1)
209-210:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
quorum_cl_sigsbecame unconditional even though this field is protocol-gated.The inline comment says these entries exist only for peers at or above protocol
70230, but this codec now always decodes and encodes them. That makes the newimpl_p2p!path incompatible with oldermnlistdiffpayloads unless the protocol gate is restored at the message boundary.Also applies to: 225-226
🤖 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/primitives/mn_list.rs` around lines 209 - 210, The field quorum_cl_sigs was made unconditional but must remain protocol-gated (only present for peers >= 70230); restore that gate by decoding and encoding quorum_cl_sigs conditionally based on the message/protocol version used by the p2p codec path (i.e., where impl_p2p! or the mn_list (de)serialize functions run). Replace the unconditional Vec::decode(data)? and Vec::encode(...) calls for quorum_cl_sigs with an if protocol_version >= 70230 { Vec::decode(data)? / Vec::encode(...) } else { Vec::new() / skip } (or use the existing helper that checks peer protocol), and ensure the same conditional logic is applied in the corresponding other occurrence (the second spot referenced) so encoding/decoding stay symmetric.pkgs/p2p_core/src/primitives/net_addr.rs-126-128 (1)
126-128:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMirror the BIP155 validation on encode.
Decode rejects oversize or wrong-length addresses, but both encode paths serialize
self.addrverbatim. That lets callers build anAddrV2/AddrV2Entrythis crate will emit and then reject on decode, which is an interoperability bug. As per coding guidelines, "Use newtypes over primitives when semantics differ, enums over booleans, make invalid states unrepresentable."Also applies to: 173-178
🤖 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/primitives/net_addr.rs` around lines 126 - 128, The encode methods (the impl of encode that currently calls self.network.to_base().encode(buf); self.addr.encode(buf);) must perform the same BIP155 address validation as decode: check network type (via self.network.to_base()/network discriminator) and enforce correct address length/format (reject oversize or wrong-length addresses) before serializing; update both encode paths (the AddrV2/AddrV2Entry encode implementations) to validate self.addr and return or propagate an error (or otherwise avoid writing invalid bytes) instead of writing self.addr verbatim so malformed AddrV2 values cannot be emitted.pkgs/p2p_core/src/primitives/mn_list.rs-91-95 (1)
91-95:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not silently encode missing Evo platform fields as zero/default.
When
version >= 2 && mn_type == MnType::Evo, these fields are required on the wire.unwrap_or(0)/unwrap_or_default()turnsNoneinto a different payload and breaks round-tripping instead of preserving or rejecting an invalid state. As per coding guidelines, "Use newtypes over primitives when semantics differ, enums over booleans, make invalid states unrepresentable."🤖 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/primitives/mn_list.rs` around lines 91 - 95, When encoding (in the method that checks self.version and calls self.mn_type.to_base().encode(buf)), do not use unwrap_or(0)/unwrap_or_default() for platform_http_port and platform_node_id; those fields are required when self.mn_type == MnType::Evo and must not be silently substituted. Instead validate that self.platform_http_port and self.platform_node_id are Some and propagate a clear error (or panic with a descriptive message) from encode(buf) when they are missing so invalid states are rejected rather than encoded as zeros/defaults.pkgs/p2p_core/src/primitives/user_agent.rs-20-21 (1)
20-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent
UserAgentserde from bypassingMAX_USER_AGENTinvariant
#[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))]onpub struct UserAgent(Vec<u8>);lets serde createUserAgentwith arbitrary byte lengths, bypassingUserAgent::new(which enforcesMAX_USER_AGENT), so invalid values can exist in-memory and later be re-encoded.Suggested patch
#[derive(Debug, Clone, PartialEq, Eq, Hash)] #[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] +#[cfg_attr(feature = "serde", serde(try_from = "Vec<u8>", into = "Vec<u8>"))] pub struct UserAgent(Vec<u8>); + +#[cfg(feature = "serde")] +impl TryFrom<Vec<u8>> for UserAgent { + type Error = UserAgentTooLong; + + fn try_from(bytes: Vec<u8>) -> Result<Self, Self::Error> { + Self::new(bytes) + } +} + +#[cfg(feature = "serde")] +impl From<UserAgent> for Vec<u8> { + fn from(value: UserAgent) -> Self { + value.0 + } +}🤖 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/primitives/user_agent.rs` around lines 20 - 21, The automatic serde derive on UserAgent bypasses the MAX_USER_AGENT invariant—remove or stop deriving Deserialize for UserAgent and add custom serde impls: implement Serialize to serialize the inner Vec<u8> (or bytes) and implement Deserialize to validate input by calling UserAgent::new (or UserAgent::try_from) and return an error if length > MAX_USER_AGENT; reference the UserAgent type, the UserAgent::new constructor (or try_from) and the MAX_USER_AGENT constant when adding the custom Deserialize logic so deserialization cannot create invalid UserAgent values.pkgs/primitives/src/gov.rs-157-180 (1)
157-180:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve unknown
object_typediscriminants across round-trips.Line 165 collapses every unrecognized wire value into
GovObjectType::Unknown, and Line 177 then re-encodes that as0. A decode/encode cycle will therefore mutate any future governance object type instead of preserving it verbatim, which breaks forward compatibility for opaque objects.🤖 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/primitives/src/gov.rs` around lines 157 - 180, The decode/encode cycle currently collapses unknown discriminants by calling GovObjectType::from_i32(...) and later re-encoding via object_type.to_i32(), which forces unknown future values to 0; update GovObject/GovObjectType handling to preserve the raw discriminant: in GovObject::decode, read the raw i32 into a local (e.g. let raw_type = i32::decode(data)?), convert to a GovObjectType for typed handling (e.g. GovObjectType::from_i32(raw_type)), and store either the raw_type alongside object_type on GovObject or extend GovObjectType with an Unknown(i32) variant that keeps the original raw value; then in GovObject::encode, write the preserved raw discriminant (raw_type or Unknown(i32) value) back to the buffer instead of always using to_i32() so unknown discriminants round-trip verbatim.pkgs/primitives/src/payload/proupservtx.rs-66-68 (1)
66-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject trailing bytes inside the extended net-info blob.
This has the same nested-decode gap as
ProRegTx:Vec::decodedelimits the blob, butExtendedNetInfo::decode(&mut &raw[..])does not require full consumption. That lets malformed v3 payloads decode successfully and re-encode to different bytes. Please fail if any bytes remain inrawafter decoding.🤖 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/primitives/src/payload/proupservtx.rs` around lines 66 - 68, The ExtendedNetInfo nested decode currently allows trailing bytes because it calls Vec::decode to get raw and then ExtendedNetInfo::decode(&mut &raw[..]) without ensuring full consumption; update the NetInfo::Extended branch (the code surrounding Vec::decode and ExtendedNetInfo::decode calls) to attempt decoding into ExtendedNetInfo and then check that the cursor has consumed all bytes (i.e., no remaining bytes in raw) and return an error if any trailing bytes remain so malformed v3 payloads fail to decode.pkgs/primitives/src/payload/proupregtx.rs-48-59 (1)
48-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore an explicit size cap for
vch_sig.This switched the signature field from the previous bounded decode path to
Vec::decode(data)?, so a hostile payload can now advertise an arbitrarily large signature length and force an unbounded allocation. Please keep the oldMAX_VCH_SIG_SIZEguard, or reject oversized lengths before materializing the vector.🤖 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/primitives/src/payload/proupregtx.rs` around lines 48 - 59, The ProUpRegTx::decode implementation currently calls Vec::decode(data)? for vch_sig which allows an attacker to declare an arbitrarily large length and force an unbounded allocation; restore the previous guarded decode by checking the decoded length against MAX_VCH_SIG_SIZE (or reject if > MAX_VCH_SIG_SIZE) before allocating or materializing the vector, e.g. decode the length first, validate it, then read/construct vch_sig only if within the cap, keeping the rest of ProUpRegTx fields and error handling intact.pkgs/primitives/src/payload/proregtx.rs-85-88 (1)
85-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject trailing bytes inside the extended net-info blob.
Vec::decodealready gives you the exact length-delimited payload, butExtendedNetInfo::decode(&mut &raw[..])ignores any unread suffix. A malformed v3 payload can therefore decode successfully and round-trip to different bytes instead of failing. Please require the inner slice to be empty after decoding.🤖 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/primitives/src/payload/proregtx.rs` around lines 85 - 88, The ExtendedNetInfo blob is currently decoded with ExtendedNetInfo::decode(&mut &raw[..]) which ignores any trailing bytes; update the v3 branch that constructs NetInfo::Extended (after Vec::decode into raw) to decode into a mutable slice/reader, then require that the reader has no remaining bytes after ExtendedNetInfo::decode returns and return an error if there are leftover bytes so malformed trailing data is rejected (referencing NetInfo::Extended, ExtendedNetInfo::decode, Vec::decode, and the raw variable).pkgs/primitives/src/payload/quorum.rs-82-88 (1)
82-88:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEncode
quorum_indexfromversion, not fromOptionpresence.Right now version 1/3 with
Some(quorum_index)writes an extrai16that decode treats as the start ofsigners, while version 2/4 withNoneomits a required field and shifts the rest of the payload. The wire shape needs to be driven byself.is_indexed(), with impossible states rejected before encoding. As per coding guidelines, "make invalid states unrepresentable."🤖 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/primitives/src/payload/quorum.rs` around lines 82 - 88, The encode method currently writes quorum_index based on Option presence which breaks the wire format; change encode(&self, buf: &mut Vec<u8>) to drive writing of the i16 strictly from self.is_indexed(): if self.is_indexed() then require self.quorum_index.is_some() and encode the inner i16, otherwise require self.quorum_index.is_none() and do not encode it; if the invariants are violated, reject the invalid state (panic or return an error consistent with surrounding API) before writing. Ensure you reference and enforce self.version/self.is_indexed() and self.quorum_index in this check so invalid states are made unrepresentable at encode time.pkgs/primitives/src/transaction.rs-82-110 (1)
82-110:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't silently drop
extra_payloadon non-special transactions.Lines 107-109 omit
extra_payloadwheneverhas_extra_payload()is false, butTransactionstill exposespub extra_payload: Vec<u8>. That means a caller can build aSpendorversion < 3transaction with payload bytes and this codec will serialize a different value than the in-memory struct, whilevalidate()still accepts that state today. Please reject that combination or make it unrepresentable before merging. As per coding guidelines, "Use newtypes over primitives, enums over booleans, make invalid states unrepresentable."🤖 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/primitives/src/transaction.rs` around lines 82 - 110, The codec currently drops extra_payload when has_extra_payload() is false, allowing an in-memory Transaction to contain payload bytes that are lost on encode; fix this by making the invalid state unrepresentable: change Transaction.extra_payload from Vec<u8> to Option<Vec<u8>> (update BaseCodec::decode to set Some(payload) only when version >= 3 && tx_type != TxType::Spend, otherwise None; update BaseCodec::encode to encode extra_payload only when Some) and update any callers/validate() to use Option semantics, or alternatively (if changing the type is too invasive) make encode explicitly reject the invalid combination by checking if extra_payload is non-empty while !has_extra_payload() and returning/propagating an error (or panic) instead of silently dropping bytes; reference Transaction, BaseCodec::decode, BaseCodec::encode, has_extra_payload, extra_payload, and validate when making the change.pkgs/types/src/entity.rs-66-68 (1)
66-68:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReject trailing bytes in
BufferDecoder::end.
BaseCodec::decodeleaves unread bytes in the slice, butend()drops that remainder and still returnsOk. That means any prefix-valid payload can be accepted with trailing garbage, and everyimpl_type!consumer inherits that behavior. This bridge should verify full consumption and return a dedicated decode error when bytes remain.🤖 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/types/src/entity.rs` around lines 66 - 68, BufferDecoder::end currently calls (self.decode_fn)(&mut &self.buf[..]) and ignores any unread trailing bytes; change end to first run the decode via BaseCodec::decode (the existing decode_fn) and then verify the internal slice has no remaining bytes, returning a dedicated decode error (e.g., a new variant like DecodeError::TrailingBytes or reuse an existing decode error) if bytes remain; update the BufferDecoder::end implementation and any error types used by BaseCodec::decode so impl_type! consumers inherit the stricter behavior and full-consumption is enforced.
🟡 Minor comments (1)
pkgs/p2p_core/src/primitives/compressed_header.rs-76-77 (1)
76-77:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
decode_headerfailure modes.The signature now returns
DecodeError, but the rustdoc still omits a# Errorssection. Please document the malformed-input cases this method can return, at least truncated input and invalid MRU version offsets.As per coding guidelines, "Document # Errors section for Result-returning functions."
🤖 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/primitives/compressed_header.rs` around lines 76 - 77, Add a Rustdoc "# Errors" section to the decode_header function documenting the failure modes: list and describe when it returns DecodeError for truncated input (insufficient bytes in the slice), for invalid MRU version offsets (out-of-range or malformed version/offset decoding), and any other DecodeError variants produced by decode_header's internal parsing (e.g., invalid varint/length fields); mention that the slice is advanced only on success and note which inputs cause early return vs. consumed bytes. Update the doc comment above pub(crate) fn decode_header(&mut self, sl: &mut &[u8]) -> Result<BlockHeader, DecodeError> accordingly, referencing the DecodeError enum variants used by the implementation.
🧹 Nitpick comments (1)
pkgs/types/src/codec.rs (1)
174-180: ⚡ Quick winRename
to_basebefore this trait spreads further.
to_base(&self)is a free borrowed conversion, so the new public API is currently out of step with the repository's conversion naming rule. Renaming it to anas_*style accessor now avoids baking the mismatch into the rest of the codec migration.As per coding guidelines, "Use as_ prefix for free conversions (borrow), to_ prefix for conversions that allocate, into_ prefix for conversions that consume."
🤖 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/types/src/codec.rs` around lines 174 - 180, Rename the borrowed conversion method in the NumCodec trait: change fn to_base(&self) -> N to fn as_base(&self) -> N (keep signature as a borrowed, zero-allocation accessor) and update all implementations and call sites to use as_base; also update the doc comment "Returns the base integer." to match and remove or forward any existing to_base usages (either by replacing calls or adding a temporary deprecated to_base delegating to as_base if you need a migration shim). Ensure the trait name NumCodec and the generic N remain unchanged.
🤖 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/primitives/src/support.rs`:
- Around line 462-474: The decoder currently returns NetInfoEntry::Invalid for
unknown entry_type tags (in the match that produces NetInfoEntry), which leaves
the cursor positioned at the start of the unknown entry body and desynchronizes
subsequent parsing; change that arm to return an Err(DecodeError::...) instead
of NetInfoEntry::Invalid so decoding fails immediately. Locate the match on
entry_type that produces NetInfoEntry::Service / NetInfoEntry::Domain /
NetInfoEntry::Invalid and replace the `_ => NetInfoEntry::Invalid` arm with a
call that constructs and returns an appropriate DecodeError (for example
Err(DecodeError::InvalidData or Err(DecodeError::UnexpectedTag) including the
unknown tag byte) so callers stop parsing rather than misreading the remaining
bytes. Ensure you return the error from the enclosing decode function
(propagating with `?` or `return Err(...)`) so the decoder aborts on unknown
NetInfoEntry tags.
---
Outside diff comments:
In `@pkgs/primitives/src/payload/cbtx.rs`:
- Around line 34-40: The struct currently models version-required fields (e.g.,
merkle_root_quorums, best_cl_height_diff, best_cl_signature,
credit_pool_balance) as Option but encode() only writes them when Some, allowing
invalid versioned payloads; change encode() to enforce presence for the required
versions (return an encoding error if a required Option is None) and update
validate() to reject missing required fields for version >=2/>=3; apply the same
checks for the other version-gated Option fields referenced in the diff ranges
(lines 43-94 and 120-141) so that encode() and validate() make those states
unrepresentable rather than silently skipping them.
In `@pkgs/primitives/src/payload/mod.rs`:
- Around line 95-127: The decode function (pub fn decode) currently returns
success for recognized TxType branches even if trailing bytes remain; change
each recognized-branch path (e.g., ProviderRegister -> ProRegTx::decode,
ProviderUpdateService -> ProUpServTx::decode, CoinbaseCommitment ->
cbtx::CoinbaseCommitment::decode, QuorumCommitment -> FinalCommitment::decode,
MnhfSignal -> MnHardFork::decode, AssetLock/AssetUnlock, etc.) to verify that
all input bytes were consumed before returning Ok: after the inner decode
succeeds and you map to the enum variant, check that *data is empty (or compute
and clear remaining bytes) and if not return a PayloadError (use the existing
err closure or construct a PayloadError with tx_type and a message about
trailing bytes); keep the Unknown branch behavior of draining data. Ensure you
use the same PayloadError type and tx_type for the failure path so callers don't
need to re-check data.is_empty().
In `@pkgs/primitives/src/support.rs`:
- Around line 204-208: The DynBitset currently exposes pub num_bits and pub data
so encode can emit mismatched bytes that decode will reject; fix by
canonicalizing the serialized payload in DynBitset::encode (or make fields
private and validate at construction) — specifically, compute required_bytes =
(num_bits + 7) / 8, then when serializing take exactly that many bytes (pad with
0x00 if data is shorter, truncate if longer) and clear any padding bits in the
final byte beyond num_bits before writing; alternatively, change num_bits and
data to private, add a new(fn new / from_parts) that enforces data length and
zeroes padding bits so encode/decode invariants hold (referencing DynBitset,
encode, decode, num_bits, data).
---
Major comments:
In `@pkgs/num/src/prelude.rs`:
- Line 9: The prelude re-export pub(crate) use alloc::string::String; is causing
a Clippy unused-import failure; gate this re-export with the same cfg/feature
used by the code that needs alloc (e.g. add the appropriate #[cfg(...)] or
#[cfg(feature = "...")] attribute matching the crate's alloc-enabled feature) or
remove it if nothing depends on it, so the import is only compiled when that
feature/path is active.
In `@pkgs/num/src/util.rs`:
- Around line 13-26: The exported macro impl_hash! emits absolute
dash_types::... paths which resolve in the caller crate; add a hidden re-export
in this crate (e.g. pub use dash_types as __dash_types;) in pkgs/num/src/lib.rs
and update the macro impl_hash! so every emitted dash_types::... reference is
routed through $crate::__dash_types::... (also update any related exported
macros like impl_type! / BaseCodec implementations to use
$crate::__dash_types::codec::... etc.) so callers don’t need a direct dash_types
dependency to expand the macro.
In `@pkgs/p2p_core/src/codec.rs`:
- Around line 9-16: The global MAX_P2P_PAYLOAD constant and impl_p2p! macro set
a 3 MiB ceiling that rejects valid Dash wire messages up to 32 MiB; change the
constant MAX_P2P_PAYLOAD from 3_145_728 to the Dash wire maximum 33_554_432 (32
* 1024 * 1024) so dash_types::impl_type! invoked by impl_p2p! uses the protocol
ceiling, and keep any tighter, message-specific validation in the individual
message implementations rather than lowering the global cap.
In `@pkgs/p2p_core/src/msg/addr.rs`:
- Around line 22-31: The Addr decoder currently lets Vec::decode read any
length; change BaseCodec::decode for Addr to first decode the CompactSize count
(the length/count field), check if count > 1000 and return an error if so, then
decode exactly that many address entries into addrs (don’t call Vec::decode
directly on the remaining buffer); apply the same change to the corresponding
impl (addrv2 decoder) so both decode methods validate the CompactSize count and
reject >1000 before reading entries.
In `@pkgs/p2p_core/src/msg/cfheaders.rs`:
- Around line 60-74: CFHeaders' BaseCodec implementation decodes filter_hashes
with the unbounded Vec::decode, allowing oversized lists; change
CFHeaders::decode/encode to first read/write an explicit compact-size count
using dash_types::codec::read_compact_size(data, <cfheaders_limit>) and
dash_types::codec::write_compact_size(count, buf) (replace <cfheaders_limit>
with the desired max), then loop decode that many BlockHash entries into
filter_hashes (use BlockHash::decode) and likewise encode by writing the compact
size then encoding each hash; update the CFHeaders::encode path to mirror this
(write compact size then encode each filter_hash) so oversized lists are
rejected.
In `@pkgs/p2p_core/src/msg/govsync.rs`:
- Around line 28-38: GovSync's bloom_filter currently uses the unbounded Vec
codec; restore a size cap by validating length during decode and encode. In
BaseCodec::decode for GovSync, replace Vec::decode(data)? with manual
length-checking: read the vector length (or peek the remaining byte length),
ensure it does not exceed MAX_BLOOM_FILTER, return a DecodeError if too large,
then read the bytes into bloom_filter; likewise, in BaseCodec::encode for
GovSync check self.bloom_filter.len() <= MAX_BLOOM_FILTER before encoding and
return/handle an error if exceeded. Use the existing MAX_BLOOM_FILTER constant
and reference the GovSync::decode, GovSync::encode, bloom_filter, and
DecodeError symbols when making the changes.
In `@pkgs/p2p_core/src/msg/headers.rs`:
- Around line 30-42: GetHeaders uses an unbounded Vec::decode/encode for
locator_hashes; change decoding in BaseCodec::decode for GetHeaders to enforce a
MAX_GETHEADERS_LOCATORS protocol constant (or reuse an existing constant) and
return a DecodeError if the decoded count exceeds it, and similarly ensure
BaseCodec::encode for GetHeaders checks the locator_hashes.len() against the
same MAX_GETHEADERS_LOCATORS and fails/returns/error if too large; update the
decode call for locator_hashes to a bounded-vec decode helper or manual
length-prefixed loop that enforces the limit and reference the GetHeaders type,
locator_hashes field, and the MAX_GETHEADERS_LOCATORS constant.
In `@pkgs/p2p_core/src/msg/inv.rs`:
- Around line 22-30: The Inv, GetData, and NotFound BaseCodec impls currently
use Vec::decode/encode for the inventory field with no upper bound; add and
enforce a MAX_INV_ITEMS cap when decoding and encoding each inventory vector: in
each decode (e.g., Inv::decode, GetData::decode, NotFound::decode) read the
compact-size count first, validate count <= MAX_INV_ITEMS before proceeding to
decode items (or return a DecodeError), and in each encode (e.g., Inv::encode,
GetData::encode, NotFound::encode) check that self.inventory.len() <=
MAX_INV_ITEMS (or truncate/return an error) and write the compact-size with that
validated count; you can implement a small bounded_vec helper or inline the
count check around the existing Vec::decode/encode calls to enforce the limit.
In `@pkgs/p2p_core/src/primitives/command.rs`:
- Around line 13-14: CommandString currently allows arbitrary bytes via the
derived Deserialize and the macro-generated BaseCodec::decode (from
dash_types::impl_bytes!(12, CommandString)), violating as_str()'s “validated on
decode” assumption; add a validating constructor (implement TryFrom<[u8;12]> for
CommandString) that enforces ASCII printable-range bytes before the first NUL
and requires all trailing bytes after the first NUL to be zero, then replace the
auto-derived Deserialize and the macro-generated BaseCodec::decode for
CommandString with manual impls that call this TryFrom constructor (or otherwise
invoke the validation) so deserialization/decoding fails on invalid bytes and
only validated, null-padded commands are constructed.
In `@pkgs/p2p_core/src/primitives/mn_list.rs`:
- Around line 209-210: The field quorum_cl_sigs was made unconditional but must
remain protocol-gated (only present for peers >= 70230); restore that gate by
decoding and encoding quorum_cl_sigs conditionally based on the message/protocol
version used by the p2p codec path (i.e., where impl_p2p! or the mn_list
(de)serialize functions run). Replace the unconditional Vec::decode(data)? and
Vec::encode(...) calls for quorum_cl_sigs with an if protocol_version >= 70230 {
Vec::decode(data)? / Vec::encode(...) } else { Vec::new() / skip } (or use the
existing helper that checks peer protocol), and ensure the same conditional
logic is applied in the corresponding other occurrence (the second spot
referenced) so encoding/decoding stay symmetric.
- Around line 91-95: When encoding (in the method that checks self.version and
calls self.mn_type.to_base().encode(buf)), do not use
unwrap_or(0)/unwrap_or_default() for platform_http_port and platform_node_id;
those fields are required when self.mn_type == MnType::Evo and must not be
silently substituted. Instead validate that self.platform_http_port and
self.platform_node_id are Some and propagate a clear error (or panic with a
descriptive message) from encode(buf) when they are missing so invalid states
are rejected rather than encoded as zeros/defaults.
In `@pkgs/p2p_core/src/primitives/net_addr.rs`:
- Around line 126-128: The encode methods (the impl of encode that currently
calls self.network.to_base().encode(buf); self.addr.encode(buf);) must perform
the same BIP155 address validation as decode: check network type (via
self.network.to_base()/network discriminator) and enforce correct address
length/format (reject oversize or wrong-length addresses) before serializing;
update both encode paths (the AddrV2/AddrV2Entry encode implementations) to
validate self.addr and return or propagate an error (or otherwise avoid writing
invalid bytes) instead of writing self.addr verbatim so malformed AddrV2 values
cannot be emitted.
In `@pkgs/p2p_core/src/primitives/user_agent.rs`:
- Around line 20-21: The automatic serde derive on UserAgent bypasses the
MAX_USER_AGENT invariant—remove or stop deriving Deserialize for UserAgent and
add custom serde impls: implement Serialize to serialize the inner Vec<u8> (or
bytes) and implement Deserialize to validate input by calling UserAgent::new (or
UserAgent::try_from) and return an error if length > MAX_USER_AGENT; reference
the UserAgent type, the UserAgent::new constructor (or try_from) and the
MAX_USER_AGENT constant when adding the custom Deserialize logic so
deserialization cannot create invalid UserAgent values.
In `@pkgs/primitives/src/gov.rs`:
- Around line 157-180: The decode/encode cycle currently collapses unknown
discriminants by calling GovObjectType::from_i32(...) and later re-encoding via
object_type.to_i32(), which forces unknown future values to 0; update
GovObject/GovObjectType handling to preserve the raw discriminant: in
GovObject::decode, read the raw i32 into a local (e.g. let raw_type =
i32::decode(data)?), convert to a GovObjectType for typed handling (e.g.
GovObjectType::from_i32(raw_type)), and store either the raw_type alongside
object_type on GovObject or extend GovObjectType with an Unknown(i32) variant
that keeps the original raw value; then in GovObject::encode, write the
preserved raw discriminant (raw_type or Unknown(i32) value) back to the buffer
instead of always using to_i32() so unknown discriminants round-trip verbatim.
In `@pkgs/primitives/src/payload/proregtx.rs`:
- Around line 85-88: The ExtendedNetInfo blob is currently decoded with
ExtendedNetInfo::decode(&mut &raw[..]) which ignores any trailing bytes; update
the v3 branch that constructs NetInfo::Extended (after Vec::decode into raw) to
decode into a mutable slice/reader, then require that the reader has no
remaining bytes after ExtendedNetInfo::decode returns and return an error if
there are leftover bytes so malformed trailing data is rejected (referencing
NetInfo::Extended, ExtendedNetInfo::decode, Vec::decode, and the raw variable).
In `@pkgs/primitives/src/payload/proupregtx.rs`:
- Around line 48-59: The ProUpRegTx::decode implementation currently calls
Vec::decode(data)? for vch_sig which allows an attacker to declare an
arbitrarily large length and force an unbounded allocation; restore the previous
guarded decode by checking the decoded length against MAX_VCH_SIG_SIZE (or
reject if > MAX_VCH_SIG_SIZE) before allocating or materializing the vector,
e.g. decode the length first, validate it, then read/construct vch_sig only if
within the cap, keeping the rest of ProUpRegTx fields and error handling intact.
In `@pkgs/primitives/src/payload/proupservtx.rs`:
- Around line 66-68: The ExtendedNetInfo nested decode currently allows trailing
bytes because it calls Vec::decode to get raw and then
ExtendedNetInfo::decode(&mut &raw[..]) without ensuring full consumption; update
the NetInfo::Extended branch (the code surrounding Vec::decode and
ExtendedNetInfo::decode calls) to attempt decoding into ExtendedNetInfo and then
check that the cursor has consumed all bytes (i.e., no remaining bytes in raw)
and return an error if any trailing bytes remain so malformed v3 payloads fail
to decode.
In `@pkgs/primitives/src/payload/quorum.rs`:
- Around line 82-88: The encode method currently writes quorum_index based on
Option presence which breaks the wire format; change encode(&self, buf: &mut
Vec<u8>) to drive writing of the i16 strictly from self.is_indexed(): if
self.is_indexed() then require self.quorum_index.is_some() and encode the inner
i16, otherwise require self.quorum_index.is_none() and do not encode it; if the
invariants are violated, reject the invalid state (panic or return an error
consistent with surrounding API) before writing. Ensure you reference and
enforce self.version/self.is_indexed() and self.quorum_index in this check so
invalid states are made unrepresentable at encode time.
In `@pkgs/primitives/src/transaction.rs`:
- Around line 82-110: The codec currently drops extra_payload when
has_extra_payload() is false, allowing an in-memory Transaction to contain
payload bytes that are lost on encode; fix this by making the invalid state
unrepresentable: change Transaction.extra_payload from Vec<u8> to
Option<Vec<u8>> (update BaseCodec::decode to set Some(payload) only when version
>= 3 && tx_type != TxType::Spend, otherwise None; update BaseCodec::encode to
encode extra_payload only when Some) and update any callers/validate() to use
Option semantics, or alternatively (if changing the type is too invasive) make
encode explicitly reject the invalid combination by checking if extra_payload is
non-empty while !has_extra_payload() and returning/propagating an error (or
panic) instead of silently dropping bytes; reference Transaction,
BaseCodec::decode, BaseCodec::encode, has_extra_payload, extra_payload, and
validate when making the change.
In `@pkgs/types/src/entity.rs`:
- Around line 66-68: BufferDecoder::end currently calls (self.decode_fn)(&mut
&self.buf[..]) and ignores any unread trailing bytes; change end to first run
the decode via BaseCodec::decode (the existing decode_fn) and then verify the
internal slice has no remaining bytes, returning a dedicated decode error (e.g.,
a new variant like DecodeError::TrailingBytes or reuse an existing decode error)
if bytes remain; update the BufferDecoder::end implementation and any error
types used by BaseCodec::decode so impl_type! consumers inherit the stricter
behavior and full-consumption is enforced.
---
Minor comments:
In `@pkgs/p2p_core/src/primitives/compressed_header.rs`:
- Around line 76-77: Add a Rustdoc "# Errors" section to the decode_header
function documenting the failure modes: list and describe when it returns
DecodeError for truncated input (insufficient bytes in the slice), for invalid
MRU version offsets (out-of-range or malformed version/offset decoding), and any
other DecodeError variants produced by decode_header's internal parsing (e.g.,
invalid varint/length fields); mention that the slice is advanced only on
success and note which inputs cause early return vs. consumed bytes. Update the
doc comment above pub(crate) fn decode_header(&mut self, sl: &mut &[u8]) ->
Result<BlockHeader, DecodeError> accordingly, referencing the DecodeError enum
variants used by the implementation.
---
Nitpick comments:
In `@pkgs/types/src/codec.rs`:
- Around line 174-180: Rename the borrowed conversion method in the NumCodec
trait: change fn to_base(&self) -> N to fn as_base(&self) -> N (keep signature
as a borrowed, zero-allocation accessor) and update all implementations and call
sites to use as_base; also update the doc comment "Returns the base integer." to
match and remove or forward any existing to_base usages (either by replacing
calls or adding a temporary deprecated to_base delegating to as_base if you need
a migration shim). Ensure the trait name NumCodec and the generic N remain
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+462
to
474
| let entry_type = u8::decode(data)?; | ||
| let entry = match entry_type { | ||
| 0x01 => NetInfoEntry::Service(CService { | ||
| addr: wire::read_array(sl)?, | ||
| port: wire::read_u16_be(sl)?, | ||
| addr: codec::take(data)?, | ||
| port: codec::read_u16_be(data)?, | ||
| }), | ||
| 0x02 => { | ||
| let name_len = wire::read_compact_size(sl, MAX_DOMAIN)?; | ||
| let name = wire::read_bytes(sl, name_len)?.to_vec(); | ||
| let port = wire::read_u16_be(sl)?; | ||
| let name: Vec<u8> = Vec::decode(data)?; | ||
| let port = codec::read_u16_be(data)?; | ||
| NetInfoEntry::Domain { name, port } | ||
| } | ||
| _ => NetInfoEntry::Invalid, | ||
| }; |
There was a problem hiding this comment.
Reject unknown NetInfoEntry tags instead of returning Invalid.
At Line 473, the decoder has only consumed the one-byte tag. Returning NetInfoEntry::Invalid there leaves the cursor at the start of the unknown entry body, so the remaining bytes get misparsed as subsequent fields and the whole ExtendedNetInfo stream becomes desynchronized. This should return a DecodeError immediately.
🤖 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/primitives/src/support.rs` around lines 462 - 474, The decoder currently
returns NetInfoEntry::Invalid for unknown entry_type tags (in the match that
produces NetInfoEntry), which leaves the cursor positioned at the start of the
unknown entry body and desynchronizes subsequent parsing; change that arm to
return an Err(DecodeError::...) instead of NetInfoEntry::Invalid so decoding
fails immediately. Locate the match on entry_type that produces
NetInfoEntry::Service / NetInfoEntry::Domain / NetInfoEntry::Invalid and replace
the `_ => NetInfoEntry::Invalid` arm with a call that constructs and returns an
appropriate DecodeError (for example Err(DecodeError::InvalidData or
Err(DecodeError::UnexpectedTag) including the unknown tag byte) so callers stop
parsing rather than misreading the remaining bytes. Ensure you return the error
from the enclosing decode function (propagating with `?` or `return Err(...)`)
so the decoder aborts on unknown NetInfoEntry tags.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkgs/primitives/src/support.rs (1)
477-480: ⚡ Quick winReuse
CService's codec instead of re-implementing it inline.The
Servicearm hand-decodesaddr/porthere and hand-encodes them at Lines 504-508, duplicatingCService::decode/encode(Lines 369-381). If theCServicewire format changes, this path silently diverges. Delegate to the existing impl.♻️ Delegate to CService codec
Decode (Lines 477-480):
- 0x01 => NetInfoEntry::Service(CService { - addr: AddrV1::decode(data)?, - port: codec::read_u16_be(data)?, - }), + 0x01 => NetInfoEntry::Service(CService::decode(data)?),Encode (Lines 504-508):
NetInfoEntry::Service(svc) => { 0x01u8.encode(buf); - svc.addr.encode(buf); - buf.extend_from_slice(&svc.port.to_be_bytes()); + svc.encode(buf); }🤖 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/primitives/src/support.rs` around lines 477 - 480, The Service arm in NetInfoEntry currently decodes/encodes addr and port manually, which duplicates CService's codec; change the decode site (NetInfoEntry::Service match) to call CService::decode(data)? and wrap the result as NetInfoEntry::Service(CService{...}) as returned by that decode, and change the encode branch to call CService::encode(&service, buf)? (or the existing instance's encode method) instead of writing addr/port manually, ensuring NetInfoEntry::Service uses CService::decode and CService::encode to avoid divergence.
🤖 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/primitives/src/gov.rs`:
- Around line 214-226: The enums VoteOutcome and VoteSignal are missing serde
derives, causing GovVote (which now derives Serialize/Deserialize) to fail when
the serde feature is enabled; add a conditional serde derive attribute to both
enums by attaching #[cfg_attr(feature = "serde", derive(::serde::Serialize,
::serde::Deserialize))] above the VoteOutcome and VoteSignal definitions so they
implement Serialize/Deserialize only when the serde feature is enabled, ensuring
GovVote can derive correctly.
---
Nitpick comments:
In `@pkgs/primitives/src/support.rs`:
- Around line 477-480: The Service arm in NetInfoEntry currently decodes/encodes
addr and port manually, which duplicates CService's codec; change the decode
site (NetInfoEntry::Service match) to call CService::decode(data)? and wrap the
result as NetInfoEntry::Service(CService{...}) as returned by that decode, and
change the encode branch to call CService::encode(&service, buf)? (or the
existing instance's encode method) instead of writing addr/port manually,
ensuring NetInfoEntry::Service uses CService::decode and CService::encode to
avoid divergence.
🪄 Autofix (Beta)
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
Run ID: 970f72fa-d28b-404d-aba1-55bdc5c0d095
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (147)
CLAUDE.mddocs/guide_rust.mdpkgs/num/Cargo.tomlpkgs/num/src/compact.rspkgs/num/src/hash.rspkgs/num/src/hash_encoding.rspkgs/num/src/lib.rspkgs/num/src/prelude.rspkgs/num/src/serialize.rspkgs/num/src/util.rspkgs/num/tests/arith.rspkgs/num/tests/hash.rspkgs/p2p_core/src/codec.rspkgs/p2p_core/src/encode.rspkgs/p2p_core/src/error.rspkgs/p2p_core/src/lib.rspkgs/p2p_core/src/msg/addr.rspkgs/p2p_core/src/msg/cfcheckpt.rspkgs/p2p_core/src/msg/cfheaders.rspkgs/p2p_core/src/msg/cfilter.rspkgs/p2p_core/src/msg/gov.rspkgs/p2p_core/src/msg/govobj.rspkgs/p2p_core/src/msg/govobjvote.rspkgs/p2p_core/src/msg/govsync.rspkgs/p2p_core/src/msg/headers.rspkgs/p2p_core/src/msg/headers2.rspkgs/p2p_core/src/msg/inv.rspkgs/p2p_core/src/msg/mnlistdiff.rspkgs/p2p_core/src/msg/mod.rspkgs/p2p_core/src/msg/ping.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/filter_type.rspkgs/p2p_core/src/primitives/governance.rspkgs/p2p_core/src/primitives/inventory.rspkgs/p2p_core/src/primitives/magic.rspkgs/p2p_core/src/primitives/mn_list.rspkgs/p2p_core/src/primitives/mod.rspkgs/p2p_core/src/primitives/net_addr.rspkgs/p2p_core/src/primitives/protocol_version.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/v2/mod.rspkgs/p2p_core/tests/addrv2.rspkgs/p2p_core/tests/mnlistdiff.rspkgs/params/src/prelude.rspkgs/params/src/types.rspkgs/pkc/src/bls_chia/agg.rspkgs/pkc/src/bls_chia/pk.rspkgs/pkc/src/bls_chia/ser.rspkgs/pkc/src/bls_chia/sig.rspkgs/pkc/src/bls_chia/sk.rspkgs/pkc/src/bls_chia/threshold.rspkgs/pkc/src/bls_ietf/agg.rspkgs/pkc/src/bls_ietf/pk.rspkgs/pkc/src/bls_ietf/sig.rspkgs/pkc/src/bls_ietf/sk.rspkgs/pkc/src/bls_ietf/threshold.rspkgs/pkc/src/common/bls/mod.rspkgs/pkc/src/common/bls/threshold.rspkgs/pkc/src/k256/pk.rspkgs/pkc/src/k256/sig.rspkgs/pkc/src/k256/sk.rspkgs/pkc/src/lib.rspkgs/pkc/src/worker.rspkgs/pkc/tests/bls_chia_llmq.rspkgs/pkc/tests/bls_ietf_llmq.rspkgs/pow/src/cubehash/simd.rspkgs/pow/src/echo/simd.rspkgs/pow/src/groestl/simd.rspkgs/pow/src/jh/simd.rspkgs/pow/src/keccak/mod.rspkgs/pow/src/lib.rspkgs/pow/src/luffa/consts.rspkgs/pow/src/luffa/simd.rspkgs/pow/src/prelude.rspkgs/pow/src/shavite/simd.rspkgs/pow/src/simd_hash/simd.rspkgs/pow/src/worker.rspkgs/pow/tests/blake.rspkgs/pow/tests/bmw.rspkgs/pow/tests/common/mod.rspkgs/pow/tests/cubehash.rspkgs/pow/tests/echo.rspkgs/pow/tests/groestl.rspkgs/pow/tests/jh.rspkgs/pow/tests/keccak.rspkgs/pow/tests/luffa.rspkgs/pow/tests/shavite.rspkgs/pow/tests/simd_hash.rspkgs/pow/tests/skein.rspkgs/primitives/Cargo.tomlpkgs/primitives/src/block.rspkgs/primitives/src/block_header.rspkgs/primitives/src/codec.rspkgs/primitives/src/error.rspkgs/primitives/src/gov.rspkgs/primitives/src/lib.rspkgs/primitives/src/outpoint.rspkgs/primitives/src/payload/assetlock.rspkgs/primitives/src/payload/assetunlock.rspkgs/primitives/src/payload/cbtx.rspkgs/primitives/src/payload/mnhftx.rspkgs/primitives/src/payload/mod.rspkgs/primitives/src/payload/proregtx.rspkgs/primitives/src/payload/proupregtx.rspkgs/primitives/src/payload/prouprevtx.rspkgs/primitives/src/payload/proupservtx.rspkgs/primitives/src/payload/quorum.rspkgs/primitives/src/prelude.rspkgs/primitives/src/script.rspkgs/primitives/src/serialize.rspkgs/primitives/src/support.rspkgs/primitives/src/transaction.rspkgs/primitives/src/tx_in.rspkgs/primitives/src/tx_out.rspkgs/primitives/src/tx_types.rspkgs/primitives/src/validation.rspkgs/primitives/src/wire.rspkgs/primitives/tests/assetlock.rspkgs/primitives/tests/assetunlock.rspkgs/primitives/tests/cbtx.rspkgs/primitives/tests/coinbase.rspkgs/primitives/tests/data.rspkgs/primitives/tests/mnhftx.rspkgs/primitives/tests/proposals.rspkgs/primitives/tests/proregtx.rspkgs/primitives/tests/proupregtx.rspkgs/primitives/tests/prouprevtx.rspkgs/primitives/tests/proupservtx.rspkgs/primitives/tests/qctx.rspkgs/primitives/tests/spend.rspkgs/primitives/tests/triggers.rspkgs/primitives/tests/util/mod.rspkgs/script/src/key_id.rspkgs/script/src/lib.rspkgs/script/src/opcode.rspkgs/script/src/prelude.rspkgs/types/src/codec.rspkgs/types/src/entity.rspkgs/types/src/hex.rspkgs/types/src/lib.rspkgs/types/src/prelude.rspkgs/types/src/uint.rs
💤 Files with no reviewable changes (15)
- pkgs/num/src/serialize.rs
- pkgs/pkc/src/lib.rs
- pkgs/pkc/tests/bls_ietf_llmq.rs
- pkgs/p2p_core/src/primitives/mod.rs
- pkgs/primitives/src/error.rs
- pkgs/p2p_core/src/msg/govobj.rs
- pkgs/pkc/tests/bls_chia_llmq.rs
- pkgs/num/src/hash_encoding.rs
- pkgs/p2p_core/src/encode.rs
- pkgs/primitives/src/wire.rs
- pkgs/script/src/key_id.rs
- pkgs/primitives/src/serialize.rs
- pkgs/p2p_core/src/msg/govsync.rs
- pkgs/p2p_core/src/primitives/governance.rs
- pkgs/p2p_core/src/msg/govobjvote.rs
✅ Files skipped from review due to trivial changes (39)
- pkgs/pkc/src/worker.rs
- pkgs/pow/tests/echo.rs
- CLAUDE.md
- pkgs/primitives/tests/util/mod.rs
- pkgs/pow/tests/bmw.rs
- pkgs/pkc/src/bls_chia/sk.rs
- pkgs/primitives/Cargo.toml
- pkgs/num/src/prelude.rs
- pkgs/pow/tests/skein.rs
- pkgs/pow/src/luffa/simd.rs
- pkgs/pow/src/lib.rs
- pkgs/pow/src/cubehash/simd.rs
- pkgs/pow/src/keccak/mod.rs
- pkgs/p2p_core/tests/addrv2.rs
- pkgs/pkc/src/bls_ietf/threshold.rs
- pkgs/pow/tests/common/mod.rs
- pkgs/num/tests/arith.rs
- pkgs/pow/tests/simd_hash.rs
- pkgs/pow/tests/jh.rs
- pkgs/pow/tests/shavite.rs
- pkgs/pow/src/luffa/consts.rs
- pkgs/num/tests/hash.rs
- pkgs/pow/src/simd_hash/simd.rs
- pkgs/p2p_core/tests/mnlistdiff.rs
- pkgs/pow/tests/luffa.rs
- pkgs/params/src/prelude.rs
- pkgs/pkc/src/common/bls/mod.rs
- pkgs/pow/src/jh/simd.rs
- pkgs/pow/tests/blake.rs
- pkgs/pow/src/echo/simd.rs
- pkgs/pkc/src/bls_chia/ser.rs
- pkgs/pkc/src/common/bls/threshold.rs
- pkgs/pkc/src/bls_chia/threshold.rs
- pkgs/pow/tests/groestl.rs
- pkgs/pow/src/worker.rs
- pkgs/pkc/src/k256/sk.rs
- pkgs/pow/tests/keccak.rs
- pkgs/pow/src/shavite/simd.rs
- pkgs/pow/src/groestl/simd.rs
🚧 Files skipped from review as they are similar to previous changes (58)
- pkgs/p2p_core/src/v2/mod.rs
- pkgs/p2p_core/src/primitives/short_id.rs
- pkgs/pow/tests/cubehash.rs
- pkgs/primitives/tests/qctx.rs
- pkgs/p2p_core/src/primitives/protocol_version.rs
- pkgs/primitives/tests/proregtx.rs
- pkgs/pkc/src/bls_ietf/agg.rs
- pkgs/script/src/opcode.rs
- pkgs/primitives/tests/proupregtx.rs
- pkgs/primitives/src/prelude.rs
- pkgs/primitives/tests/mnhftx.rs
- pkgs/primitives/tests/data.rs
- pkgs/primitives/tests/proposals.rs
- pkgs/primitives/tests/proupservtx.rs
- pkgs/p2p_core/src/prelude.rs
- pkgs/p2p_core/src/primitives/filter_type.rs
- pkgs/p2p_core/src/primitives/command.rs
- pkgs/primitives/tests/assetunlock.rs
- pkgs/primitives/src/payload/mnhftx.rs
- pkgs/pkc/src/bls_ietf/sig.rs
- pkgs/primitives/tests/triggers.rs
- pkgs/pkc/src/bls_chia/sig.rs
- pkgs/pkc/src/bls_chia/agg.rs
- pkgs/primitives/src/script.rs
- pkgs/primitives/tests/assetlock.rs
- pkgs/p2p_core/src/msg/cfcheckpt.rs
- pkgs/p2p_core/src/primitives/compressed_header.rs
- pkgs/num/Cargo.toml
- pkgs/params/src/types.rs
- pkgs/pkc/src/bls_ietf/pk.rs
- docs/guide_rust.md
- pkgs/p2p_core/src/primitives/user_agent.rs
- pkgs/types/src/hex.rs
- pkgs/primitives/src/tx_out.rs
- pkgs/primitives/src/payload/proupregtx.rs
- pkgs/types/src/prelude.rs
- pkgs/p2p_core/src/lib.rs
- pkgs/primitives/tests/coinbase.rs
- pkgs/primitives/tests/spend.rs
- pkgs/pkc/src/bls_chia/pk.rs
- pkgs/num/src/lib.rs
- pkgs/primitives/tests/prouprevtx.rs
- pkgs/p2p_core/src/msg/headers2.rs
- pkgs/p2p_core/src/msg/headers.rs
- pkgs/p2p_core/src/error.rs
- pkgs/primitives/src/tx_types.rs
- pkgs/primitives/src/block.rs
- pkgs/pkc/src/k256/sig.rs
- pkgs/primitives/src/payload/assetlock.rs
- pkgs/primitives/src/payload/proupservtx.rs
- pkgs/primitives/src/payload/quorum.rs
- pkgs/num/src/util.rs
- pkgs/primitives/src/payload/cbtx.rs
- pkgs/p2p_core/src/primitives/inventory.rs
- pkgs/p2p_core/src/msg/cfheaders.rs
- pkgs/p2p_core/src/msg/cfilter.rs
- pkgs/types/src/codec.rs
- pkgs/p2p_core/src/primitives/net_addr.rs
Comment on lines
+214
to
+226
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | ||
| pub enum VoteOutcome { | ||
| /// No vote cast. | ||
| None, | ||
| /// Vote in favour. | ||
| Yes, | ||
| /// Vote against. | ||
| No, | ||
| /// Abstention. | ||
| Abstain, | ||
| /// Unrecognised outcome. | ||
| Unknown(u32), | ||
| } |
There was a problem hiding this comment.
VoteOutcome and VoteSignal must derive serde — GovVote won't compile with the serde feature.
GovVote now derives Serialize/Deserialize (Line 328) with non-skipped fields outcome: VoteOutcome (Line 335) and signal: VoteSignal (Line 337). Both enums must implement those traits; neither does today, so the serde build breaks.
🐛 Add the conditional serde derive to both enums
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))]
pub enum VoteOutcome {Apply the same to VoteSignal (~Line 265):
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
#[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))]
pub enum VoteSignal {📝 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
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | |
| pub enum VoteOutcome { | |
| /// No vote cast. | |
| None, | |
| /// Vote in favour. | |
| Yes, | |
| /// Vote against. | |
| No, | |
| /// Abstention. | |
| Abstain, | |
| /// Unrecognised outcome. | |
| Unknown(u32), | |
| } | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] | |
| #[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] | |
| pub enum VoteOutcome { | |
| /// No vote cast. | |
| None, | |
| /// Vote in favour. | |
| Yes, | |
| /// Vote against. | |
| No, | |
| /// Abstention. | |
| Abstain, | |
| /// Unrecognised outcome. | |
| Unknown(u32), | |
| } |
🤖 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/primitives/src/gov.rs` around lines 214 - 226, The enums VoteOutcome and
VoteSignal are missing serde derives, causing GovVote (which now derives
Serialize/Deserialize) to fail when the serde feature is enabled; add a
conditional serde derive attribute to both enums by attaching #[cfg_attr(feature
= "serde", derive(::serde::Serialize, ::serde::Deserialize))] above the
VoteOutcome and VoteSignal definitions so they implement Serialize/Deserialize
only when the serde feature is enabled, ensuring GovVote can derive correctly.
5 tasks
github-actions
Bot
added
the
needs-rebase
5 tasks
Move the `From<[u8; N]>` impl into `impl_bytes!` so standalone callers no longer need to provide it manually. Remove the now- redundant impl from `make_bytes!`, `CommandString`, and `Magic`.
github-actions
Bot
removed
the
needs-rebase
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkgs/primitives/src/tx_types.rs (1)
96-124:⚠️ Potential issue | 🟠 MajorFix serde feature break:
ProRegTxderives needMnTypeto be serde-serializable
pkgs/primitives/src/payload/proregtx.rsderivesSerialize/Deserializeunder#[cfg_attr(feature = "serde", ...)]and includespub mn_type: MnType, butMnTypeinpkgs/primitives/src/tx_types.rshas no corresponding serde derive/impls—so enablingserdeshould fail to compile.
- Add
#[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))]toMnType(or otherwise gate/removeProRegTx’s serde derives).🤖 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/primitives/src/tx_types.rs` around lines 96 - 124, The ProRegTx serde derives fail because MnType lacks serde impls; update the MnType enum to be serde-serializable by adding #[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] to the MnType declaration so ProRegTx's pub mn_type: MnType compiles when the serde feature is enabled; keep the existing NumCodec/impl_num! logic unchanged and ensure the Unknown(u16) variant is covered by the derived serde impls.pkgs/primitives/src/support.rs (1)
386-418:⚠️ Potential issue | 🟠 MajorAdd
serdederives forNetInfoPurpose.
ExtendedNetInfoderivesSerialize/Deserialize(feature-gated) butentriescontainsNetInfoPurpose;NetInfoPurposecurrently has noserdederive/impl, so enablingfeature = "serde"will fail to compile.🤖 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/primitives/src/support.rs` around lines 386 - 418, NetInfoPurpose is missing serde derives which breaks compilation when ExtendedNetInfo (whose entries use NetInfoPurpose) is built with the "serde" feature; add conditional serde derives to NetInfoPurpose by applying cfg_attr(feature = "serde", derive(Serialize, Deserialize)) to the enum declaration (keep existing derives Clone, Copy, Debug, Eq, Hash, PartialEq) so NetInfoPurpose can be serialized/deserialized when "serde" is enabled; update references remain the same (NetInfoPurpose, ExtendedNetInfo, entries, NumCodec, impl_num!) and re-run cargo check to verify.
🧹 Nitpick comments (5)
pkgs/num/src/lib.rs (1)
23-24: ⚡ Quick winSwitch this shim to
#[expect].
#[allow(unused_imports)]works, but it sidesteps the repo rule and makes the shim silently linger after it stops being necessary.Suggested change
-#[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] +#[expect(unused_imports, reason = "ergonomic shim, exports may be unused")] mod prelude;As per coding guidelines,
Use #[expect] over #[allow].🤖 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/num/src/lib.rs` around lines 23 - 24, The module shim uses #[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] which hides when the shim is no longer needed; change the attribute to #[expect(unused_imports, reason = "ergonomic shim, exports may be unused")] on the mod prelude declaration so the compiler will warn when the shim becomes unused instead of silently permitting it (update the attribute on the mod prelude line).Source: Coding guidelines
pkgs/types/src/lib.rs (1)
17-18: ⚡ Quick winPrefer
#[expect]for the prelude shim.This repo’s Rust guidelines call for
#[expect]instead of#[allow], which is a better fit here because it fails closed once the shim stops triggeringunused_imports.Suggested change
-#[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] +#[expect(unused_imports, reason = "ergonomic shim, exports may be unused")] mod prelude;As per coding guidelines,
Use #[expect] over #[allow].🤖 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/types/src/lib.rs` around lines 17 - 18, Replace the #[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] attribute on the prelude shim with #[expect(unused_imports, reason = "...")] so the shim uses the repository guideline's expect attribute; update the attribute on the mod prelude declaration (the line declaring mod prelude) to use #[expect] with the same reason string.Source: Coding guidelines
pkgs/script/src/lib.rs (1)
15-16: ⚡ Quick winUse
#[expect]here too.This shim has the same policy issue as the other crate roots:
#[allow]suppresses the lint permanently instead of failing once the import stops being intentionally unused.Suggested change
-#[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] +#[expect(unused_imports, reason = "ergonomic shim, exports may be unused")] mod prelude;As per coding guidelines,
Use #[expect] over #[allow].🤖 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/lib.rs` around lines 15 - 16, Replace the unconditional lint suppression on the prelude shim with an expectation so unused imports fail once they stop being intentionally unused: change the attribute on the `mod prelude;` declaration from `#[allow(unused_imports, reason = "...")]` to `#[expect(unused_imports, reason = "...")]` (keeping the same reason text) so the `prelude` module still compiles ergonomically but will trigger a CI-visible expectation if the import becomes unintentionally unused.Source: Coding guidelines
pkgs/p2p_core/src/primitives/filter_type.rs (1)
14-15: ⚡ Quick winDerive
Debuginstead of hand-rolling it.This manual impl prints the tuple newtype exactly the way
#[derive(Debug)]would, so it only adds boilerplate and bypasses the repo rule for eager derives.Proposed fix
-#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct FilterType(pub u8); @@ -impl fmt::Debug for FilterType { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "FilterType({})", self.0) - } -} - impl fmt::Display for FilterType { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Display::fmt(&self.0, f) } }As per coding guidelines, "Derive
Clone,Debug,PartialEq,Eq,Hasheagerly."Also applies to: 62-66
🤖 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/primitives/filter_type.rs` around lines 14 - 15, The manual Debug implementation for the tuple-newtype FilterType should be removed and replaced with an eager derive; update the derive list on pub struct FilterType (and the other similar types mentioned around lines 62-66) to include Debug (i.e., add Debug to #[derive(...)]), and delete the hand-rolled impl Debug blocks so the compiler-generated Debug is used consistently for FilterType and the other matching types.Source: Coding guidelines
pkgs/p2p_core/src/primitives/protocol_version.rs (1)
14-15: ⚡ Quick winDerive
Debughere too.This custom impl is equivalent to the derived output for a tuple newtype, so it is extra surface area without any behavior gain.
Proposed fix
-#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ProtocolVersion(pub u32); @@ -impl fmt::Debug for ProtocolVersion { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "ProtocolVersion({})", self.0) - } -} - impl fmt::Display for ProtocolVersion { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Display::fmt(&self.0, f) } }As per coding guidelines, "Derive
Clone,Debug,PartialEq,Eq,Hasheagerly."Also applies to: 76-80
🤖 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/primitives/protocol_version.rs` around lines 14 - 15, The current tuple-newtype ProtocolVersion (and the similar type around lines 76-80) should derive Debug instead of having a redundant custom impl; update the derive attribute on ProtocolVersion (and the other tuple newtype) to include Debug (e.g., #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]) and remove the manual Debug impl that duplicates the derived behavior so the standard derived Debug is used consistently.Source: Coding guidelines
🤖 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 `@contrib/codeql/lib/traits.qll`:
- Around line 46-56: The impl-to-type join currently uses only fileOf(i) and
implSelfName(i) (in manualImplInfo and therefore hasManualImpl) which misbinds
impls across different lexical modules in the same file; update manualImplInfo
to also capture and compare a stable enclosing-scope identifier for both the
Impl and the TypeItem (e.g. an enclosingModulePath/enclosingContainer predicate
or an AST container node) so the predicate requires that the Impl and the
TypeItem share the same scope identity in addition to fileOf, implSelfName(i)
and implTraitName(i); apply the same scope-equality change to the other
analogous predicates that use the same join logic (the ones around the other
ranges mentioned) so impl-to-type matching is disambiguated by lexical scope
rather than just file and name.
In `@pkgs/p2p_core/src/primitives/command.rs`:
- Line 18: The generic impl_bytes!(12, CommandString) does not validate
ASCII/UTF-8 or null-padding, so replace the macro usage with a bespoke
implementation for CommandString: implement BaseCodec::decode for CommandString
(or a custom FromBytes) that reads exactly 12 bytes, finds the first NUL byte,
verifies all bytes after that NUL are NUL (null-padding), validates the prefix
bytes are valid ASCII/UTF-8 (or ASCII-only if required), and returns an Err on
invalid input; keep CommandString::as_str() relying on that validated decode and
update its comment accordingly. Ensure callers like ShortId::from_command and
Debug/Display still compare raw bytes where intended, but any API returning a
&str uses the validated CommandString::as_str().
In `@pkgs/p2p_core/src/primitives/inventory.rs`:
- Around line 18-19: The InvType enum is missing serde derives required by
Inventory’s serde feature; update the InvType declaration to derive Serialize
and Deserialize when the serde feature is enabled (e.g. add cfg_attr(feature =
"serde", derive(Serialize, Deserialize)) to the enum) so that the Inventory
field inv_type: InvType can be serialized/deserialized; ensure the serde traits
are available under the feature gate in the same module where InvType is
defined.
In `@pkgs/primitives/src/payload/cbtx.rs`:
- Around line 81-89: The encode implementation currently synthesizes defaults
for missing v2/v3 fields; instead, make encode validate presence and fail when
those fields are absent: in the encode method, when self.version >= 2
assert/return an error if merkle_root_quorums.is_none() rather than using
unwrap_or_default(), and when self.version >= 3 assert/return an error if any of
best_cl_height_diff, best_cl_signature, or credit_pool_balance are None rather
than using unwrap_or(0)/unwrap_or_default(); reference the fields
merkle_root_quorums, best_cl_height_diff, best_cl_signature, credit_pool_balance
and the encode method to locate the checks and replace the unwrap_or* calls with
presence checks that return a failure (or panic) to prevent synthesizing
fabricated data.
In `@pkgs/primitives/src/payload/proregtx.rs`:
- Around line 135-156: The encode path must not silently skip or zero required
ProRegTx fields: update the encode implementation to explicitly enforce
version/net_info consistency and presence of Evo platform fields. Specifically,
when self.version >= 3 require that self.net_info is NetInfo::Extended (encode
its blob) and return/raise an error (or panic) if it is NetInfo::Legacy;
likewise when self.version < 3 require NetInfo::Legacy and error on Extended.
For MnType::Evo, do not encode defaults — require that platform_node_id,
platform_p2p_port and platform_http_port are Some and encode their values (or
error if missing) instead of using unwrap_or_default/unwrap_or(0); rely on
validate() or enforce checks inside encode so invalid states are not serialized.
In `@pkgs/primitives/src/support.rs`:
- Around line 277-279: count_ones() currently sums bits across the entire data
vec without respecting DynBitset::num_bits, so trailing bits in the final byte
are incorrectly counted; update count_ones (method on DynBitset) to only count
up to num_bits by either iterating via the existing iter_set_bits() or masking
the final byte before counting, ensuring consistency with get() and
iter_set_bits() behavior.
---
Outside diff comments:
In `@pkgs/primitives/src/support.rs`:
- Around line 386-418: NetInfoPurpose is missing serde derives which breaks
compilation when ExtendedNetInfo (whose entries use NetInfoPurpose) is built
with the "serde" feature; add conditional serde derives to NetInfoPurpose by
applying cfg_attr(feature = "serde", derive(Serialize, Deserialize)) to the enum
declaration (keep existing derives Clone, Copy, Debug, Eq, Hash, PartialEq) so
NetInfoPurpose can be serialized/deserialized when "serde" is enabled; update
references remain the same (NetInfoPurpose, ExtendedNetInfo, entries, NumCodec,
impl_num!) and re-run cargo check to verify.
In `@pkgs/primitives/src/tx_types.rs`:
- Around line 96-124: The ProRegTx serde derives fail because MnType lacks serde
impls; update the MnType enum to be serde-serializable by adding
#[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))]
to the MnType declaration so ProRegTx's pub mn_type: MnType compiles when the
serde feature is enabled; keep the existing NumCodec/impl_num! logic unchanged
and ensure the Unknown(u16) variant is covered by the derived serde impls.
---
Nitpick comments:
In `@pkgs/num/src/lib.rs`:
- Around line 23-24: The module shim uses #[allow(unused_imports, reason =
"ergonomic shim, exports may be unused")] which hides when the shim is no longer
needed; change the attribute to #[expect(unused_imports, reason = "ergonomic
shim, exports may be unused")] on the mod prelude declaration so the compiler
will warn when the shim becomes unused instead of silently permitting it (update
the attribute on the mod prelude line).
In `@pkgs/p2p_core/src/primitives/filter_type.rs`:
- Around line 14-15: The manual Debug implementation for the tuple-newtype
FilterType should be removed and replaced with an eager derive; update the
derive list on pub struct FilterType (and the other similar types mentioned
around lines 62-66) to include Debug (i.e., add Debug to #[derive(...)]), and
delete the hand-rolled impl Debug blocks so the compiler-generated Debug is used
consistently for FilterType and the other matching types.
In `@pkgs/p2p_core/src/primitives/protocol_version.rs`:
- Around line 14-15: The current tuple-newtype ProtocolVersion (and the similar
type around lines 76-80) should derive Debug instead of having a redundant
custom impl; update the derive attribute on ProtocolVersion (and the other tuple
newtype) to include Debug (e.g., #[derive(Clone, Copy, Debug, Default,
PartialEq, Eq, PartialOrd, Ord, Hash)]) and remove the manual Debug impl that
duplicates the derived behavior so the standard derived Debug is used
consistently.
In `@pkgs/script/src/lib.rs`:
- Around line 15-16: Replace the unconditional lint suppression on the prelude
shim with an expectation so unused imports fail once they stop being
intentionally unused: change the attribute on the `mod prelude;` declaration
from `#[allow(unused_imports, reason = "...")]` to `#[expect(unused_imports,
reason = "...")]` (keeping the same reason text) so the `prelude` module still
compiles ergonomically but will trigger a CI-visible expectation if the import
becomes unintentionally unused.
In `@pkgs/types/src/lib.rs`:
- Around line 17-18: Replace the #[allow(unused_imports, reason = "ergonomic
shim, exports may be unused")] attribute on the prelude shim with
#[expect(unused_imports, reason = "...")] so the shim uses the repository
guideline's expect attribute; update the attribute on the mod prelude
declaration (the line declaring mod prelude) to use #[expect] with the same
reason string.
🪄 Autofix (Beta)
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
Run ID: 9906f935-c89d-4cd5-9fa2-ff9dac57a2f3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (91)
contrib/codeql/decl.qlcontrib/codeql/lib/filters.qllcontrib/codeql/lib/fmt.qllcontrib/codeql/lib/policy.qllcontrib/codeql/lib/traits.qllpkgs/num/Cargo.tomlpkgs/num/src/compact.rspkgs/num/src/hash.rspkgs/num/src/hash_encoding.rspkgs/num/src/lib.rspkgs/num/src/prelude.rspkgs/num/src/serialize.rspkgs/num/src/util.rspkgs/p2p_core/src/codec.rspkgs/p2p_core/src/encode.rspkgs/p2p_core/src/error.rspkgs/p2p_core/src/lib.rspkgs/p2p_core/src/msg/addr.rspkgs/p2p_core/src/msg/cfcheckpt.rspkgs/p2p_core/src/msg/cfheaders.rspkgs/p2p_core/src/msg/cfilter.rspkgs/p2p_core/src/msg/gov.rspkgs/p2p_core/src/msg/govobj.rspkgs/p2p_core/src/msg/govobjvote.rspkgs/p2p_core/src/msg/govsync.rspkgs/p2p_core/src/msg/headers.rspkgs/p2p_core/src/msg/headers2.rspkgs/p2p_core/src/msg/inv.rspkgs/p2p_core/src/msg/mnlistdiff.rspkgs/p2p_core/src/msg/mod.rspkgs/p2p_core/src/msg/ping.rspkgs/p2p_core/src/msg/version.rspkgs/p2p_core/src/primitives/command.rspkgs/p2p_core/src/primitives/compressed_header.rspkgs/p2p_core/src/primitives/filter_type.rspkgs/p2p_core/src/primitives/governance.rspkgs/p2p_core/src/primitives/inventory.rspkgs/p2p_core/src/primitives/magic.rspkgs/p2p_core/src/primitives/mn_list.rspkgs/p2p_core/src/primitives/mod.rspkgs/p2p_core/src/primitives/net_addr.rspkgs/p2p_core/src/primitives/protocol_version.rspkgs/p2p_core/src/primitives/service_flags.rspkgs/p2p_core/src/primitives/user_agent.rspkgs/primitives/src/block.rspkgs/primitives/src/block_header.rspkgs/primitives/src/codec.rspkgs/primitives/src/error.rspkgs/primitives/src/gov.rspkgs/primitives/src/lib.rspkgs/primitives/src/outpoint.rspkgs/primitives/src/payload/assetlock.rspkgs/primitives/src/payload/assetunlock.rspkgs/primitives/src/payload/cbtx.rspkgs/primitives/src/payload/mnhftx.rspkgs/primitives/src/payload/mod.rspkgs/primitives/src/payload/proregtx.rspkgs/primitives/src/payload/proupregtx.rspkgs/primitives/src/payload/prouprevtx.rspkgs/primitives/src/payload/proupservtx.rspkgs/primitives/src/payload/quorum.rspkgs/primitives/src/script.rspkgs/primitives/src/serialize.rspkgs/primitives/src/support.rspkgs/primitives/src/transaction.rspkgs/primitives/src/tx_in.rspkgs/primitives/src/tx_out.rspkgs/primitives/src/tx_types.rspkgs/primitives/src/validation.rspkgs/primitives/src/wire.rspkgs/primitives/tests/assetlock.rspkgs/primitives/tests/assetunlock.rspkgs/primitives/tests/cbtx.rspkgs/primitives/tests/coinbase.rspkgs/primitives/tests/data.rspkgs/primitives/tests/mnhftx.rspkgs/primitives/tests/proposals.rspkgs/primitives/tests/proregtx.rspkgs/primitives/tests/proupregtx.rspkgs/primitives/tests/prouprevtx.rspkgs/primitives/tests/proupservtx.rspkgs/primitives/tests/qctx.rspkgs/primitives/tests/spend.rspkgs/primitives/tests/triggers.rspkgs/script/src/key_id.rspkgs/script/src/lib.rspkgs/types/src/codec.rspkgs/types/src/entity.rspkgs/types/src/hex.rspkgs/types/src/lib.rspkgs/types/src/uint.rs
💤 Files with no reviewable changes (11)
- pkgs/num/src/hash_encoding.rs
- pkgs/primitives/src/error.rs
- pkgs/p2p_core/src/encode.rs
- pkgs/script/src/key_id.rs
- pkgs/p2p_core/src/primitives/governance.rs
- pkgs/num/src/serialize.rs
- pkgs/primitives/src/wire.rs
- pkgs/p2p_core/src/msg/govobj.rs
- pkgs/p2p_core/src/msg/govobjvote.rs
- pkgs/p2p_core/src/msg/govsync.rs
- pkgs/primitives/src/serialize.rs
✅ Files skipped from review due to trivial changes (2)
- pkgs/primitives/tests/cbtx.rs
- pkgs/p2p_core/src/primitives/mod.rs
🚧 Files skipped from review as they are similar to previous changes (50)
- pkgs/primitives/tests/assetunlock.rs
- pkgs/primitives/tests/proregtx.rs
- pkgs/primitives/tests/prouprevtx.rs
- pkgs/primitives/tests/spend.rs
- pkgs/p2p_core/src/msg/gov.rs
- pkgs/primitives/tests/data.rs
- pkgs/primitives/tests/triggers.rs
- pkgs/num/Cargo.toml
- pkgs/primitives/tests/mnhftx.rs
- pkgs/primitives/tests/assetlock.rs
- pkgs/primitives/tests/proposals.rs
- pkgs/primitives/src/payload/assetlock.rs
- pkgs/primitives/tests/proupregtx.rs
- pkgs/primitives/tests/coinbase.rs
- pkgs/p2p_core/src/codec.rs
- pkgs/primitives/src/lib.rs
- pkgs/p2p_core/src/msg/ping.rs
- pkgs/primitives/src/payload/assetunlock.rs
- pkgs/p2p_core/src/msg/mnlistdiff.rs
- pkgs/num/src/hash.rs
- pkgs/primitives/tests/qctx.rs
- pkgs/primitives/src/tx_out.rs
- pkgs/types/src/codec.rs
- pkgs/primitives/src/payload/mnhftx.rs
- pkgs/p2p_core/src/msg/headers2.rs
- pkgs/num/src/prelude.rs
- pkgs/primitives/src/block_header.rs
- pkgs/primitives/src/block.rs
- pkgs/p2p_core/src/primitives/user_agent.rs
- pkgs/p2p_core/src/primitives/magic.rs
- pkgs/p2p_core/src/msg/inv.rs
- pkgs/p2p_core/src/primitives/net_addr.rs
- pkgs/primitives/src/script.rs
- pkgs/primitives/src/payload/proupservtx.rs
- pkgs/p2p_core/src/msg/version.rs
- pkgs/p2p_core/src/lib.rs
- pkgs/p2p_core/src/primitives/compressed_header.rs
- pkgs/num/src/util.rs
- pkgs/p2p_core/src/msg/mod.rs
- pkgs/primitives/src/payload/quorum.rs
- pkgs/primitives/src/transaction.rs
- pkgs/primitives/src/gov.rs
- pkgs/p2p_core/src/msg/headers.rs
- pkgs/p2p_core/src/msg/cfilter.rs
- pkgs/p2p_core/src/msg/addr.rs
- pkgs/p2p_core/src/msg/cfcheckpt.rs
- pkgs/types/src/uint.rs
- pkgs/p2p_core/src/primitives/mn_list.rs
- pkgs/types/src/hex.rs
- pkgs/primitives/src/payload/prouprevtx.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkgs/primitives/src/tx_types.rs (1)
96-124:⚠️ Potential issue | 🟠 MajorFix serde feature break:
ProRegTxderives needMnTypeto be serde-serializable
pkgs/primitives/src/payload/proregtx.rsderivesSerialize/Deserializeunder#[cfg_attr(feature = "serde", ...)]and includespub mn_type: MnType, butMnTypeinpkgs/primitives/src/tx_types.rshas no corresponding serde derive/impls—so enablingserdeshould fail to compile.
- Add
#[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))]toMnType(or otherwise gate/removeProRegTx’s serde derives).🤖 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/primitives/src/tx_types.rs` around lines 96 - 124, The ProRegTx serde derives fail because MnType lacks serde impls; update the MnType enum to be serde-serializable by adding #[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] to the MnType declaration so ProRegTx's pub mn_type: MnType compiles when the serde feature is enabled; keep the existing NumCodec/impl_num! logic unchanged and ensure the Unknown(u16) variant is covered by the derived serde impls.pkgs/primitives/src/support.rs (1)
386-418:⚠️ Potential issue | 🟠 MajorAdd
serdederives forNetInfoPurpose.
ExtendedNetInfoderivesSerialize/Deserialize(feature-gated) butentriescontainsNetInfoPurpose;NetInfoPurposecurrently has noserdederive/impl, so enablingfeature = "serde"will fail to compile.🤖 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/primitives/src/support.rs` around lines 386 - 418, NetInfoPurpose is missing serde derives which breaks compilation when ExtendedNetInfo (whose entries use NetInfoPurpose) is built with the "serde" feature; add conditional serde derives to NetInfoPurpose by applying cfg_attr(feature = "serde", derive(Serialize, Deserialize)) to the enum declaration (keep existing derives Clone, Copy, Debug, Eq, Hash, PartialEq) so NetInfoPurpose can be serialized/deserialized when "serde" is enabled; update references remain the same (NetInfoPurpose, ExtendedNetInfo, entries, NumCodec, impl_num!) and re-run cargo check to verify.
🧹 Nitpick comments (5)
pkgs/num/src/lib.rs (1)
23-24: ⚡ Quick winSwitch this shim to
#[expect].
#[allow(unused_imports)]works, but it sidesteps the repo rule and makes the shim silently linger after it stops being necessary.Suggested change
-#[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] +#[expect(unused_imports, reason = "ergonomic shim, exports may be unused")] mod prelude;As per coding guidelines,
Use #[expect] over #[allow].🤖 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/num/src/lib.rs` around lines 23 - 24, The module shim uses #[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] which hides when the shim is no longer needed; change the attribute to #[expect(unused_imports, reason = "ergonomic shim, exports may be unused")] on the mod prelude declaration so the compiler will warn when the shim becomes unused instead of silently permitting it (update the attribute on the mod prelude line).Source: Coding guidelines
pkgs/types/src/lib.rs (1)
17-18: ⚡ Quick winPrefer
#[expect]for the prelude shim.This repo’s Rust guidelines call for
#[expect]instead of#[allow], which is a better fit here because it fails closed once the shim stops triggeringunused_imports.Suggested change
-#[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] +#[expect(unused_imports, reason = "ergonomic shim, exports may be unused")] mod prelude;As per coding guidelines,
Use #[expect] over #[allow].🤖 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/types/src/lib.rs` around lines 17 - 18, Replace the #[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] attribute on the prelude shim with #[expect(unused_imports, reason = "...")] so the shim uses the repository guideline's expect attribute; update the attribute on the mod prelude declaration (the line declaring mod prelude) to use #[expect] with the same reason string.Source: Coding guidelines
pkgs/script/src/lib.rs (1)
15-16: ⚡ Quick winUse
#[expect]here too.This shim has the same policy issue as the other crate roots:
#[allow]suppresses the lint permanently instead of failing once the import stops being intentionally unused.Suggested change
-#[allow(unused_imports, reason = "ergonomic shim, exports may be unused")] +#[expect(unused_imports, reason = "ergonomic shim, exports may be unused")] mod prelude;As per coding guidelines,
Use #[expect] over #[allow].🤖 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/lib.rs` around lines 15 - 16, Replace the unconditional lint suppression on the prelude shim with an expectation so unused imports fail once they stop being intentionally unused: change the attribute on the `mod prelude;` declaration from `#[allow(unused_imports, reason = "...")]` to `#[expect(unused_imports, reason = "...")]` (keeping the same reason text) so the `prelude` module still compiles ergonomically but will trigger a CI-visible expectation if the import becomes unintentionally unused.Source: Coding guidelines
pkgs/p2p_core/src/primitives/filter_type.rs (1)
14-15: ⚡ Quick winDerive
Debuginstead of hand-rolling it.This manual impl prints the tuple newtype exactly the way
#[derive(Debug)]would, so it only adds boilerplate and bypasses the repo rule for eager derives.Proposed fix
-#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct FilterType(pub u8); @@ -impl fmt::Debug for FilterType { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "FilterType({})", self.0) - } -} - impl fmt::Display for FilterType { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Display::fmt(&self.0, f) } }As per coding guidelines, "Derive
Clone,Debug,PartialEq,Eq,Hasheagerly."Also applies to: 62-66
🤖 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/primitives/filter_type.rs` around lines 14 - 15, The manual Debug implementation for the tuple-newtype FilterType should be removed and replaced with an eager derive; update the derive list on pub struct FilterType (and the other similar types mentioned around lines 62-66) to include Debug (i.e., add Debug to #[derive(...)]), and delete the hand-rolled impl Debug blocks so the compiler-generated Debug is used consistently for FilterType and the other matching types.Source: Coding guidelines
pkgs/p2p_core/src/primitives/protocol_version.rs (1)
14-15: ⚡ Quick winDerive
Debughere too.This custom impl is equivalent to the derived output for a tuple newtype, so it is extra surface area without any behavior gain.
Proposed fix
-#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ProtocolVersion(pub u32); @@ -impl fmt::Debug for ProtocolVersion { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "ProtocolVersion({})", self.0) - } -} - impl fmt::Display for ProtocolVersion { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { fmt::Display::fmt(&self.0, f) } }As per coding guidelines, "Derive
Clone,Debug,PartialEq,Eq,Hasheagerly."Also applies to: 76-80
🤖 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/primitives/protocol_version.rs` around lines 14 - 15, The current tuple-newtype ProtocolVersion (and the similar type around lines 76-80) should derive Debug instead of having a redundant custom impl; update the derive attribute on ProtocolVersion (and the other tuple newtype) to include Debug (e.g., #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]) and remove the manual Debug impl that duplicates the derived behavior so the standard derived Debug is used consistently.Source: Coding guidelines
🤖 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 `@contrib/codeql/lib/traits.qll`:
- Around line 46-56: The impl-to-type join currently uses only fileOf(i) and
implSelfName(i) (in manualImplInfo and therefore hasManualImpl) which misbinds
impls across different lexical modules in the same file; update manualImplInfo
to also capture and compare a stable enclosing-scope identifier for both the
Impl and the TypeItem (e.g. an enclosingModulePath/enclosingContainer predicate
or an AST container node) so the predicate requires that the Impl and the
TypeItem share the same scope identity in addition to fileOf, implSelfName(i)
and implTraitName(i); apply the same scope-equality change to the other
analogous predicates that use the same join logic (the ones around the other
ranges mentioned) so impl-to-type matching is disambiguated by lexical scope
rather than just file and name.
In `@pkgs/p2p_core/src/primitives/command.rs`:
- Line 18: The generic impl_bytes!(12, CommandString) does not validate
ASCII/UTF-8 or null-padding, so replace the macro usage with a bespoke
implementation for CommandString: implement BaseCodec::decode for CommandString
(or a custom FromBytes) that reads exactly 12 bytes, finds the first NUL byte,
verifies all bytes after that NUL are NUL (null-padding), validates the prefix
bytes are valid ASCII/UTF-8 (or ASCII-only if required), and returns an Err on
invalid input; keep CommandString::as_str() relying on that validated decode and
update its comment accordingly. Ensure callers like ShortId::from_command and
Debug/Display still compare raw bytes where intended, but any API returning a
&str uses the validated CommandString::as_str().
In `@pkgs/p2p_core/src/primitives/inventory.rs`:
- Around line 18-19: The InvType enum is missing serde derives required by
Inventory’s serde feature; update the InvType declaration to derive Serialize
and Deserialize when the serde feature is enabled (e.g. add cfg_attr(feature =
"serde", derive(Serialize, Deserialize)) to the enum) so that the Inventory
field inv_type: InvType can be serialized/deserialized; ensure the serde traits
are available under the feature gate in the same module where InvType is
defined.
In `@pkgs/primitives/src/payload/cbtx.rs`:
- Around line 81-89: The encode implementation currently synthesizes defaults
for missing v2/v3 fields; instead, make encode validate presence and fail when
those fields are absent: in the encode method, when self.version >= 2
assert/return an error if merkle_root_quorums.is_none() rather than using
unwrap_or_default(), and when self.version >= 3 assert/return an error if any of
best_cl_height_diff, best_cl_signature, or credit_pool_balance are None rather
than using unwrap_or(0)/unwrap_or_default(); reference the fields
merkle_root_quorums, best_cl_height_diff, best_cl_signature, credit_pool_balance
and the encode method to locate the checks and replace the unwrap_or* calls with
presence checks that return a failure (or panic) to prevent synthesizing
fabricated data.
In `@pkgs/primitives/src/payload/proregtx.rs`:
- Around line 135-156: The encode path must not silently skip or zero required
ProRegTx fields: update the encode implementation to explicitly enforce
version/net_info consistency and presence of Evo platform fields. Specifically,
when self.version >= 3 require that self.net_info is NetInfo::Extended (encode
its blob) and return/raise an error (or panic) if it is NetInfo::Legacy;
likewise when self.version < 3 require NetInfo::Legacy and error on Extended.
For MnType::Evo, do not encode defaults — require that platform_node_id,
platform_p2p_port and platform_http_port are Some and encode their values (or
error if missing) instead of using unwrap_or_default/unwrap_or(0); rely on
validate() or enforce checks inside encode so invalid states are not serialized.
In `@pkgs/primitives/src/support.rs`:
- Around line 277-279: count_ones() currently sums bits across the entire data
vec without respecting DynBitset::num_bits, so trailing bits in the final byte
are incorrectly counted; update count_ones (method on DynBitset) to only count
up to num_bits by either iterating via the existing iter_set_bits() or masking
the final byte before counting, ensuring consistency with get() and
iter_set_bits() behavior.
---
Outside diff comments:
In `@pkgs/primitives/src/support.rs`:
- Around line 386-418: NetInfoPurpose is missing serde derives which breaks
compilation when ExtendedNetInfo (whose entries use NetInfoPurpose) is built
with the "serde" feature; add conditional serde derives to NetInfoPurpose by
applying cfg_attr(feature = "serde", derive(Serialize, Deserialize)) to the enum
declaration (keep existing derives Clone, Copy, Debug, Eq, Hash, PartialEq) so
NetInfoPurpose can be serialized/deserialized when "serde" is enabled; update
references remain the same (NetInfoPurpose, ExtendedNetInfo, entries, NumCodec,
impl_num!) and re-run cargo check to verify.
In `@pkgs/primitives/src/tx_types.rs`:
- Around line 96-124: The ProRegTx serde derives fail because MnType lacks serde
impls; update the MnType enum to be serde-serializable by adding
#[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))]
to the MnType declaration so ProRegTx's pub mn_type: MnType compiles when the
serde feature is enabled; keep the existing NumCodec/impl_num! logic unchanged
and ensure the Unknown(u16) variant is covered by the derived serde impls.
---
Nitpick comments:
In `@pkgs/num/src/lib.rs`:
- Around line 23-24: The module shim uses #[allow(unused_imports, reason =
"ergonomic shim, exports may be unused")] which hides when the shim is no longer
needed; change the attribute to #[expect(unused_imports, reason = "ergonomic
shim, exports may be unused")] on the mod prelude declaration so the compiler
will warn when the shim becomes unused instead of silently permitting it (update
the attribute on the mod prelude line).
In `@pkgs/p2p_core/src/primitives/filter_type.rs`:
- Around line 14-15: The manual Debug implementation for the tuple-newtype
FilterType should be removed and replaced with an eager derive; update the
derive list on pub struct FilterType (and the other similar types mentioned
around lines 62-66) to include Debug (i.e., add Debug to #[derive(...)]), and
delete the hand-rolled impl Debug blocks so the compiler-generated Debug is used
consistently for FilterType and the other matching types.
In `@pkgs/p2p_core/src/primitives/protocol_version.rs`:
- Around line 14-15: The current tuple-newtype ProtocolVersion (and the similar
type around lines 76-80) should derive Debug instead of having a redundant
custom impl; update the derive attribute on ProtocolVersion (and the other tuple
newtype) to include Debug (e.g., #[derive(Clone, Copy, Debug, Default,
PartialEq, Eq, PartialOrd, Ord, Hash)]) and remove the manual Debug impl that
duplicates the derived behavior so the standard derived Debug is used
consistently.
In `@pkgs/script/src/lib.rs`:
- Around line 15-16: Replace the unconditional lint suppression on the prelude
shim with an expectation so unused imports fail once they stop being
intentionally unused: change the attribute on the `mod prelude;` declaration
from `#[allow(unused_imports, reason = "...")]` to `#[expect(unused_imports,
reason = "...")]` (keeping the same reason text) so the `prelude` module still
compiles ergonomically but will trigger a CI-visible expectation if the import
becomes unintentionally unused.
In `@pkgs/types/src/lib.rs`:
- Around line 17-18: Replace the #[allow(unused_imports, reason = "ergonomic
shim, exports may be unused")] attribute on the prelude shim with
#[expect(unused_imports, reason = "...")] so the shim uses the repository
guideline's expect attribute; update the attribute on the mod prelude
declaration (the line declaring mod prelude) to use #[expect] with the same
reason string.
🪄 Autofix (Beta)
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
Run ID: 9906f935-c89d-4cd5-9fa2-ff9dac57a2f3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (91)
contrib/codeql/decl.qlcontrib/codeql/lib/filters.qllcontrib/codeql/lib/fmt.qllcontrib/codeql/lib/policy.qllcontrib/codeql/lib/traits.qllpkgs/num/Cargo.tomlpkgs/num/src/compact.rspkgs/num/src/hash.rspkgs/num/src/hash_encoding.rspkgs/num/src/lib.rspkgs/num/src/prelude.rspkgs/num/src/serialize.rspkgs/num/src/util.rspkgs/p2p_core/src/codec.rspkgs/p2p_core/src/encode.rspkgs/p2p_core/src/error.rspkgs/p2p_core/src/lib.rspkgs/p2p_core/src/msg/addr.rspkgs/p2p_core/src/msg/cfcheckpt.rspkgs/p2p_core/src/msg/cfheaders.rspkgs/p2p_core/src/msg/cfilter.rspkgs/p2p_core/src/msg/gov.rspkgs/p2p_core/src/msg/govobj.rspkgs/p2p_core/src/msg/govobjvote.rspkgs/p2p_core/src/msg/govsync.rspkgs/p2p_core/src/msg/headers.rspkgs/p2p_core/src/msg/headers2.rspkgs/p2p_core/src/msg/inv.rspkgs/p2p_core/src/msg/mnlistdiff.rspkgs/p2p_core/src/msg/mod.rspkgs/p2p_core/src/msg/ping.rspkgs/p2p_core/src/msg/version.rspkgs/p2p_core/src/primitives/command.rspkgs/p2p_core/src/primitives/compressed_header.rspkgs/p2p_core/src/primitives/filter_type.rspkgs/p2p_core/src/primitives/governance.rspkgs/p2p_core/src/primitives/inventory.rspkgs/p2p_core/src/primitives/magic.rspkgs/p2p_core/src/primitives/mn_list.rspkgs/p2p_core/src/primitives/mod.rspkgs/p2p_core/src/primitives/net_addr.rspkgs/p2p_core/src/primitives/protocol_version.rspkgs/p2p_core/src/primitives/service_flags.rspkgs/p2p_core/src/primitives/user_agent.rspkgs/primitives/src/block.rspkgs/primitives/src/block_header.rspkgs/primitives/src/codec.rspkgs/primitives/src/error.rspkgs/primitives/src/gov.rspkgs/primitives/src/lib.rspkgs/primitives/src/outpoint.rspkgs/primitives/src/payload/assetlock.rspkgs/primitives/src/payload/assetunlock.rspkgs/primitives/src/payload/cbtx.rspkgs/primitives/src/payload/mnhftx.rspkgs/primitives/src/payload/mod.rspkgs/primitives/src/payload/proregtx.rspkgs/primitives/src/payload/proupregtx.rspkgs/primitives/src/payload/prouprevtx.rspkgs/primitives/src/payload/proupservtx.rspkgs/primitives/src/payload/quorum.rspkgs/primitives/src/script.rspkgs/primitives/src/serialize.rspkgs/primitives/src/support.rspkgs/primitives/src/transaction.rspkgs/primitives/src/tx_in.rspkgs/primitives/src/tx_out.rspkgs/primitives/src/tx_types.rspkgs/primitives/src/validation.rspkgs/primitives/src/wire.rspkgs/primitives/tests/assetlock.rspkgs/primitives/tests/assetunlock.rspkgs/primitives/tests/cbtx.rspkgs/primitives/tests/coinbase.rspkgs/primitives/tests/data.rspkgs/primitives/tests/mnhftx.rspkgs/primitives/tests/proposals.rspkgs/primitives/tests/proregtx.rspkgs/primitives/tests/proupregtx.rspkgs/primitives/tests/prouprevtx.rspkgs/primitives/tests/proupservtx.rspkgs/primitives/tests/qctx.rspkgs/primitives/tests/spend.rspkgs/primitives/tests/triggers.rspkgs/script/src/key_id.rspkgs/script/src/lib.rspkgs/types/src/codec.rspkgs/types/src/entity.rspkgs/types/src/hex.rspkgs/types/src/lib.rspkgs/types/src/uint.rs
💤 Files with no reviewable changes (11)
- pkgs/num/src/hash_encoding.rs
- pkgs/primitives/src/error.rs
- pkgs/p2p_core/src/encode.rs
- pkgs/script/src/key_id.rs
- pkgs/p2p_core/src/primitives/governance.rs
- pkgs/num/src/serialize.rs
- pkgs/primitives/src/wire.rs
- pkgs/p2p_core/src/msg/govobj.rs
- pkgs/p2p_core/src/msg/govobjvote.rs
- pkgs/p2p_core/src/msg/govsync.rs
- pkgs/primitives/src/serialize.rs
✅ Files skipped from review due to trivial changes (2)
- pkgs/primitives/tests/cbtx.rs
- pkgs/p2p_core/src/primitives/mod.rs
🚧 Files skipped from review as they are similar to previous changes (50)
- pkgs/primitives/tests/assetunlock.rs
- pkgs/primitives/tests/proregtx.rs
- pkgs/primitives/tests/prouprevtx.rs
- pkgs/primitives/tests/spend.rs
- pkgs/p2p_core/src/msg/gov.rs
- pkgs/primitives/tests/data.rs
- pkgs/primitives/tests/triggers.rs
- pkgs/num/Cargo.toml
- pkgs/primitives/tests/mnhftx.rs
- pkgs/primitives/tests/assetlock.rs
- pkgs/primitives/tests/proposals.rs
- pkgs/primitives/src/payload/assetlock.rs
- pkgs/primitives/tests/proupregtx.rs
- pkgs/primitives/tests/coinbase.rs
- pkgs/p2p_core/src/codec.rs
- pkgs/primitives/src/lib.rs
- pkgs/p2p_core/src/msg/ping.rs
- pkgs/primitives/src/payload/assetunlock.rs
- pkgs/p2p_core/src/msg/mnlistdiff.rs
- pkgs/num/src/hash.rs
- pkgs/primitives/tests/qctx.rs
- pkgs/primitives/src/tx_out.rs
- pkgs/types/src/codec.rs
- pkgs/primitives/src/payload/mnhftx.rs
- pkgs/p2p_core/src/msg/headers2.rs
- pkgs/num/src/prelude.rs
- pkgs/primitives/src/block_header.rs
- pkgs/primitives/src/block.rs
- pkgs/p2p_core/src/primitives/user_agent.rs
- pkgs/p2p_core/src/primitives/magic.rs
- pkgs/p2p_core/src/msg/inv.rs
- pkgs/p2p_core/src/primitives/net_addr.rs
- pkgs/primitives/src/script.rs
- pkgs/primitives/src/payload/proupservtx.rs
- pkgs/p2p_core/src/msg/version.rs
- pkgs/p2p_core/src/lib.rs
- pkgs/p2p_core/src/primitives/compressed_header.rs
- pkgs/num/src/util.rs
- pkgs/p2p_core/src/msg/mod.rs
- pkgs/primitives/src/payload/quorum.rs
- pkgs/primitives/src/transaction.rs
- pkgs/primitives/src/gov.rs
- pkgs/p2p_core/src/msg/headers.rs
- pkgs/p2p_core/src/msg/cfilter.rs
- pkgs/p2p_core/src/msg/addr.rs
- pkgs/p2p_core/src/msg/cfcheckpt.rs
- pkgs/types/src/uint.rs
- pkgs/p2p_core/src/primitives/mn_list.rs
- pkgs/types/src/hex.rs
- pkgs/primitives/src/payload/prouprevtx.rs
🛑 Comments failed to post (7)
contrib/codeql/lib/traits.qll (1)
46-56:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDisambiguate impl-to-type matching by lexical scope, not just file and name
These predicates associate
Implblocks toTypeItemusing(fileOf, implSelfName)only. If two modules in the same file define the same type name, impls can be attached to the wrong type, which then misclassifies slots in declaration-ordering output. Add a scope discriminator (for example enclosing module path or another stable container identity) to these joins.Also applies to: 60-73, 98-105, 137-161
🤖 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 `@contrib/codeql/lib/traits.qll` around lines 46 - 56, The impl-to-type join currently uses only fileOf(i) and implSelfName(i) (in manualImplInfo and therefore hasManualImpl) which misbinds impls across different lexical modules in the same file; update manualImplInfo to also capture and compare a stable enclosing-scope identifier for both the Impl and the TypeItem (e.g. an enclosingModulePath/enclosingContainer predicate or an AST container node) so the predicate requires that the Impl and the TypeItem share the same scope identity in addition to fileOf, implSelfName(i) and implTraitName(i); apply the same scope-equality change to the other analogous predicates that use the same join logic (the ones around the other ranges mentioned) so impl-to-type matching is disambiguated by lexical scope rather than just file and name.pkgs/p2p_core/src/primitives/command.rs (1)
18-18:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the impl_bytes! macro and nearby CommandString code. rg -n -C5 'macro_rules!\s*impl_bytes|impl_bytes!\(' pkgs/types pkgs/p2p_core rg -n -C5 'from_bytes|from_static|as_str|CommandString' pkgs/p2p_core/src/primitives/command.rsRepository: dashpay/base-sdk
Length of output: 11234
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the macro-generated decode/encode logic. rg -n -C2 "macro_rules! impl_bytes" pkgs/types/src/hex.rs sed -n '1,120p' pkgs/types/src/hex.rs sed -n '120,240p' pkgs/types/src/hex.rs # Find how BaseCodec decode errors are handled for CommandString usage. rg -n "CommandString|impl_bytes!\(12,\s*CommandString\)" pkgs/p2p_core/src/primitives/command.rs rg -n "decode\([^)]*CommandString|BaseCodec.*CommandString|from_bytes.*CommandString" pkgs/p2p_core/src -SRepository: dashpay/base-sdk
Length of output: 4654
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the take::<N> helper used by impl_bytes! decode. rg -n "fn take::<|take::<\w+>|fn take\(" pkgs/types/src -S rg -n "take\s*::<\s*\$n|take::<" pkgs/types/src/codec -S || true fd -a "codec" pkgs/types/src | head -n 50 || true # Inspect BaseCodec / Decodable wiring and any decode-time validation. rg -n "trait BaseCodec|trait Decodable|trait Encodable|DecodeError" pkgs/types/src -S rg -n "impl_type!\(" pkgs/types/src -S rg -n -C2 "impl_type!" pkgs/types/src/hex.rs # Inspect CommandString docs for the "validated on decode" claim. sed -n '35,75p' pkgs/p2p_core/src/primitives/command.rsRepository: dashpay/base-sdk
Length of output: 6034
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect codec take + DecodeError. sed -n '1,220p' pkgs/types/src/codec.rs # Find and inspect impl_type! (likely wires BaseCodec into Decodable/Encodable). rg -n "macro_rules!\s*impl_type" pkgs/types/src -S rg -n "impl_type!\(" pkgs/types/src/hex.rs pkgs/types/src -S # Inspect how Decodable/Encodable are implemented for BaseCodec types. rg -n "trait\s+Decodable|trait\s+Encodable" pkgs/types/src -S rg -n "Decodable\s+for|Encodable\s+for" pkgs/types/src -S # Confirm whether as_str() is used for parsing anywhere. rg -n "as_str\(\)" pkgs/p2p_core/src -SRepository: dashpay/base-sdk
Length of output: 6324
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect types for Decodable/Encodable and impl_type macro. sed -n '1,220p' pkgs/types/src/entity.rs sed -n '220,420p' pkgs/types/src/entity.rs # Inspect impl_type! usage in fixed-size byte newtypes (hex.rs/uint.rs call sites) sed -n '1,120p' pkgs/types/src/hex.rs sed -n '1,120p' pkgs/types/src/uint.rs # Inspect CommandString decode usage in p2p core, if any specific decode path exists. rg -n "CommandString" pkgs/p2p_core/src/primitives/command.rs pkgs/p2p_core/src -SRepository: dashpay/base-sdk
Length of output: 13144
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect how CommandString is used when decoding messages. sed -n '1,220p' pkgs/p2p_core/src/msg/mod.rs sed -n '220,520p' pkgs/p2p_core/src/msg/mod.rs # See if decoding performs any additional validation using as_str() / byte checks. rg -n "as_str\(\)|from_utf8|position\(\|&b\|\s*b\s*==\s*0|CommandString::from_bytes|CommandString\(" pkgs/p2p_core/src/msg -S rg -n "decode_msg|CommandString.*decode|CommandString.*cmd" pkgs/p2p_core/src/msg -SRepository: dashpay/base-sdk
Length of output: 10241
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "CommandString::as_str|\.as_str\(\)|as_str\(\)" pkgs/p2p_core/src -S sed -n '1,260p' pkgs/p2p_core/src/primitives/short_id.rs rg -n "from_command\(" pkgs/p2p_core/src/primitives/short_id.rsRepository: dashpay/base-sdk
Length of output: 5433
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "ShortId::from_command|from_command\(&\s*.*CommandString" pkgs/p2p_core/src -S rg -n "short_id\(\)" pkgs/p2p_core/src -SRepository: dashpay/base-sdk
Length of output: 295
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,220p' pkgs/p2p_core/src/v2/mod.rs rg -n "UnknownCommand|cmd\.as_str\(|as_str\(\)|from_utf8" pkgs/p2p_core/src -SRepository: dashpay/base-sdk
Length of output: 2974
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,140p' pkgs/p2p_core/src/error.rs sed -n '1,120p' pkgs/p2p_core/src/primitives/command.rsRepository: dashpay/base-sdk
Length of output: 6681
impl_bytes!(12, CommandString)doesn’t enforceCommandString’s ASCII/null-padding invariants
- In
pkgs/types/src/hex.rs,impl_bytes!’sBaseCodec::decodeis justtake::<12>(data)(copies 12 bytes; no ASCII / UTF-8 / null-padding validation).- Therefore
CommandString::as_str()’s comment that the bytes are “validated on decode” is wrong: it trims at the first NUL, then returns""if the prefix is not valid UTF-8.- Dispatch to known message variants still relies on exact byte equality, but any code using
as_str()(e.g.,ShortId::from_command, Debug/Display) will see malformed commands as"".🤖 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/primitives/command.rs` at line 18, The generic impl_bytes!(12, CommandString) does not validate ASCII/UTF-8 or null-padding, so replace the macro usage with a bespoke implementation for CommandString: implement BaseCodec::decode for CommandString (or a custom FromBytes) that reads exactly 12 bytes, finds the first NUL byte, verifies all bytes after that NUL are NUL (null-padding), validates the prefix bytes are valid ASCII/UTF-8 (or ASCII-only if required), and returns an Err on invalid input; keep CommandString::as_str() relying on that validated decode and update its comment accordingly. Ensure callers like ShortId::from_command and Debug/Display still compare raw bytes where intended, but any API returning a &str uses the validated CommandString::as_str().pkgs/p2p_core/src/primitives/inventory.rs (1)
18-19:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="pkgs/p2p_core/src/primitives/inventory.rs" echo "== $FILE ==" nl -ba "$FILE" | sed -n '1,140p'Repository: dashpay/base-sdk
Length of output: 169
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="pkgs/p2p_core/src/primitives/inventory.rs" echo "== $FILE ==" cat -n "$FILE" | sed -n '1,160p'Repository: dashpay/base-sdk
Length of output: 3287
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "InvType" pkgs/p2p_core/src/primitives/inventory.rs rg -n "InvType.*serde|serde.*InvType|impl\s+.*Serialize.*InvType|impl\s+.*Deserialize.*InvType|derive\(.*Serialize.*\).*InvType|derive\(.*Deserialize.*\).*InvType" pkgs/p2p_core/src -S || true rg -n "cfg_attr\(feature\s*=\s*\"serde\".*InvType|feature\s*=\s*\"serde\".*InvType" pkgs -S || trueRepository: dashpay/base-sdk
Length of output: 209
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n "impl\s+(?:<[^>]*>\s*)?(?:serde::)?Serialize\s+for\s+InvType|impl\s+(?:<[^>]*>\s*)?(?:serde::)?Deserialize\s+for\s+InvType" . -S || true rg -n "derive\([^)]*(?:Serialize|Deserialize)[^)]*\)\s*pub\s+enum\s+InvType|serde.*InvType|cfg_attr\(feature\s*=\s*\"serde\".*InvType" . -S || trueRepository: dashpay/base-sdk
Length of output: 42
Add
serdederives toInvTypeto satisfyInventory’s serde feature
InventoryderivesSerialize/Deserializeunder theserdefeature, but its fieldinv_type: InvTypeneedsInvTypeto implement those traits;InvTypecurrently has no serde derives, which breaks serde-enabled builds.Suggested change
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +#[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] pub enum InvType {📝 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.#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] #[cfg_attr(feature = "serde", derive(::serde::Serialize, ::serde::Deserialize))] pub enum InvType {🤖 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/primitives/inventory.rs` around lines 18 - 19, The InvType enum is missing serde derives required by Inventory’s serde feature; update the InvType declaration to derive Serialize and Deserialize when the serde feature is enabled (e.g. add cfg_attr(feature = "serde", derive(Serialize, Deserialize)) to the enum) so that the Inventory field inv_type: InvType can be serialized/deserialized; ensure the serde traits are available under the feature gate in the same module where InvType is defined.pkgs/primitives/src/payload/cbtx.rs (1)
81-89:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDo not synthesize missing v2/v3 fields during encode.
For
version >= 2, decode always materializesmerkle_root_quorums, and forversion >= 3it always materializes the ChainLock fields, but encode fills missing values with defaults instead of rejecting the state. That lets a manually constructed payload round-trip into different bytes and emit fabricated zero/default quorum data on the wire. As per coding guidelines, "make invalid states unrepresentable".🤖 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/primitives/src/payload/cbtx.rs` around lines 81 - 89, The encode implementation currently synthesizes defaults for missing v2/v3 fields; instead, make encode validate presence and fail when those fields are absent: in the encode method, when self.version >= 2 assert/return an error if merkle_root_quorums.is_none() rather than using unwrap_or_default(), and when self.version >= 3 assert/return an error if any of best_cl_height_diff, best_cl_signature, or credit_pool_balance are None rather than using unwrap_or(0)/unwrap_or_default(); reference the fields merkle_root_quorums, best_cl_height_diff, best_cl_signature, credit_pool_balance and the encode method to locate the checks and replace the unwrap_or* calls with presence checks that return a failure (or panic) to prevent synthesizing fabricated data.Source: Coding guidelines
pkgs/primitives/src/payload/proregtx.rs (2)
87-88:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winConsume the whole embedded
ExtendedNetInfoblob.
Vec::decode(data)?gives you a length-delimited buffer, butExtendedNetInfo::decode(&mut &raw[..])?does not verify that it consumed all of it. Any trailing bytes inside that blob are currently accepted and then discarded on re-encode.
135-156:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEncode should not drop or zero required ProRegTx fields.
If
version >= 3butnet_infois stillLegacy, this writes no net-info blob at all and shifts every following field left. Separately, the Evo branch zero-fillsplatform_node_id/ ports when the options are missing, andvalidate()does not reject those missing Evo fields today. This can serialize corrupt payloads from representable invalid states. As per coding guidelines, "make invalid states unrepresentable".🤖 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/primitives/src/payload/proregtx.rs` around lines 135 - 156, The encode path must not silently skip or zero required ProRegTx fields: update the encode implementation to explicitly enforce version/net_info consistency and presence of Evo platform fields. Specifically, when self.version >= 3 require that self.net_info is NetInfo::Extended (encode its blob) and return/raise an error (or panic) if it is NetInfo::Legacy; likewise when self.version < 3 require NetInfo::Legacy and error on Extended. For MnType::Evo, do not encode defaults — require that platform_node_id, platform_p2p_port and platform_http_port are Some and encode their values (or error if missing) instead of using unwrap_or_default/unwrap_or(0); rely on validate() or enforce checks inside encode so invalid states are not serialized.Source: Coding guidelines
pkgs/primitives/src/support.rs (1)
277-279:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
count_ones()ignoresnum_bits.This sums every bit in
self.data, soDynBitset { num_bits: 1, data: vec![0xff] }reports8even thoughget()anditer_set_bits()only treat bit 0 as valid. Mask the final byte or count throughiter_set_bits()instead.🤖 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/primitives/src/support.rs` around lines 277 - 279, count_ones() currently sums bits across the entire data vec without respecting DynBitset::num_bits, so trailing bits in the final byte are incorrectly counted; update count_ones (method on DynBitset) to only count up to num_bits by either iterating via the existing iter_set_bits() or masking the final byte before counting, ensuring consistency with get() and iter_set_bits() behavior.
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%lint: introduce CodeQL and semgrep linters, add shim, derivation, disambiguation rules, resolve violations, add common helper for lint infra #5
Depends on sdk%fix(codeql): deal with inconsistent caches, allocate half of available memory, fix CSV parsing bug, add
--cache/-c#6Breaking Changes
Work in progress.
Checklist