Uh oh!
There was an error while loading. Please reload this page.
sdk%refac: switch from hex to hex-conservative - #2
Merged
Conversation
No actionable comments were generated in the recent review. 🎉 ℹ️ 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 (34)
✅ Files skipped from review due to trivial changes (15)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThis pull request migrates all hex encoding/decoding operations across the codebase from the ChangesHex Crate Migration: hex → hex-conservative
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Warning This pull request may have conflicts, please coordinate with the authors of these pull requests. Potential conflicts |
kwvg
marked this pull request as ready for review
May 5, 2026 10:16
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
pkgs/pkc/tests/bls_chia_llmq.rs (1)
208-213: 💤 Low valueOptional: avoid the intermediate
Vec<u8>allocation when reversingsid_bytes
sid_bytesis[u8; 32]; copying and reversing in-place avoids the heap allocation. The same pattern appears atpkgs/pkc/tests/bls_ietf_llmq.rsLines 255–260.♻️ Proposed refactor
- let sid_display = sid_bytes- .iter()- .copied()- .rev()- .collect::<Vec<u8>>()- .to_lower_hex_string();+ let mut sid_reversed = sid_bytes;+ sid_reversed.reverse();+ let sid_display = sid_reversed.to_lower_hex_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/pkc/tests/bls_chia_llmq.rs` around lines 208 - 213, sid_display is built by reversing sid_bytes into a Vec<u8> which causes an unnecessary heap allocation; replace that allocation by reversing into a fixed-size stack array (e.g., let mut rev = [0u8; 32]; for i in 0..32 { rev[i] = sid_bytes[31 - i]; }) and then call the same hex conversion (to_lower_hex_string or hex::encode_lower) on rev, updating the code that constructs sid_display (referencing sid_bytes and sid_display) to use the stack buffer instead of collecting into Vec<u8>.pkgs/primitives/Cargo.toml (1)
19-20: ⚡ Quick winSame
full = ["std", "serde"]guideline deviation aspkgs/script/Cargo.toml.As per coding guidelines: "Feature layout must follow:
default = [], std = [...], full = ["std"], serde = ["dep:serde"]."🤖 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/Cargo.toml` around lines 19 - 20, The feature entries in Cargo.toml deviate from the required layout: adjust the feature definitions so they follow the scheme default = [], std = [...], full = ["std"], serde = ["dep:serde"]; specifically replace the current serde = ["dep:serde", "dash-num/serde", "dash-script/serde", "dash-types/serde"] with serde = ["dep:serde"], ensure the std feature contains any platform/std subfeatures (keep or add "dash-num/serde", "dash-script/serde", "dash-types/serde" under std if intended), and change full = ["std"] instead of full = ["std", "serde"] so that the features named serde and full match the guideline.pkgs/script/Cargo.toml (1)
15-16: ⚡ Quick win
full = ["std", "serde"]deviates from the feature layout guideline.The workspace guidelines mandate
full = ["std"]. This PR changesfullto["std", "serde"]across multiple crates. If the intent is to makefullthe "kitchen sink" feature, the coding guideline itself should be updated to reflect the new convention.As per coding guidelines: "Feature layout must follow:
default = [], std = [...], full = ["std"], serde = ["dep:serde"]."🤖 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/Cargo.toml` around lines 15 - 16, The Cargo feature layout was changed incorrectly: the feature "full" should not include "serde"; change the "full" feature back to ["std"] (remove "serde" from the "full" array), ensure "serde" remains its own feature (serde = ["dep:serde", "dash-types/serde"] or per crate convention), and keep the overall layout consistent with the guideline (default = [], std = [...], full = ["std"], serde = ["dep:serde"]). Make these edits to the feature definitions (the "full", "std", "serde", and "default" feature entries) so the crate follows the workspace feature convention.pkgs/num/src/serialize.rs (1)
38-44: 💤 Low valueNon-idiomatic
whileloop — preferforrange.♻️ Proposed refactor
- let mut i = 0;- while i < byte_len {+ for i in 0..byte_len { let hi = hex_val(b[i * 2])?; let lo = hex_val(b[i * 2 + 1])?; out[byte_len - 1 - i] = (hi << 4) | lo; - i += 1; }🤖 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/serialize.rs` around lines 38 - 44, Replace the manual while loop with a Rust for-range to be more idiomatic: iterate i over 0..byte_len (or use .enumerate() on an iterator) instead of the while, keep the body logic that calls hex_val(b[i * 2]) and hex_val(b[i * 2 + 1]) and assigns out[byte_len - 1 - i] = (hi << 4) | lo, and remove the explicit i += 1; update any mutable i binding accordingly so behavior remains identical.pkgs/p2p_core/Cargo.toml (1)
17-17: ⚡ Quick win
fullshould not includeserdeper feature-layout guidelines.The stated feature layout mandates
full = ["std"]. Including"serde"bundles an opt-in serialisation dependency intofull, which callers may not want. Opt-in features (serde) should remain separately gated.Note:
pkgs/types/Cargo.tomlfollows the samefull = ["std", "serde"]pattern — if this is intentional it warrants an explicit update to the guidelines; otherwise both crates need correcting.🔧 Suggested fix
-full = ["std", "serde"]+full = ["std"]As per coding guidelines, "
full = ["std"]" is the required feature-layout for allpkgs/*/Cargo.tomlfiles.🤖 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/Cargo.toml` at line 17, The crate's feature group 'full' currently lists ["std", "serde"], but per project feature-layout rules 'full' must only include "std"; remove "serde" from the 'full' array in the Cargo.toml (the 'full' feature declaration) so serde remains an opt-in feature, and apply the same change to the matching 'full' declaration in pkgs/types/Cargo.toml if present.pkgs/types/src/hex.rs (1)
107-156: 💤 Low valueConsider dispatching on
is_human_readable()for future binary format compatibility.The
serde::serialize/deserializefunctions and eachwNsubmodule currently encode as lowercase-hex strings unconditionally. For non-human-readable formats (bincode, postcard, CBOR-binary), this still pays the hex-encoding cost and forces those formats to carry a string instead of raw bytes. Since these adapters use theserde(transparent)+serde(with = "...")pattern already in place, adding this dispatch is straightforward to do now and matches the conventional bitcoin-ecosystem pattern.Sketch for the
Vec<u8>adapter (thewNsubmodules would follow the same shape):♻️ Proposed refactor
/// Serializes bytes as a wire-order hex string. pub fn serialize<S: ::serde::Serializer>(data: &[u8], serializer: S) -> Result<S::Ok, S::Error> { - serializer.serialize_str(&data.to_lower_hex_string())+ if serializer.is_human_readable() {+ serializer.serialize_str(&data.to_lower_hex_string())+ } else {+ serializer.serialize_bytes(data)+ } } /// Deserializes a hex string into bytes. pub fn deserialize<'de, D: ::serde::Deserializer<'de>>(deserializer: D) -> Result<Vec<u8>, D::Error> { - let s = <String as ::serde::Deserialize>::deserialize(deserializer)?;- Vec::<u8>::from_hex(&s).map_err(::serde::de::Error::custom)+ if deserializer.is_human_readable() {+ let s = <String as ::serde::Deserialize>::deserialize(deserializer)?;+ Vec::<u8>::from_hex(&s).map_err(::serde::de::Error::custom)+ } else {+ <Vec<u8> as ::serde::Deserialize>::deserialize(deserializer)+ } }If JSON is the only intended consumer, feel free to defer — this is mainly to make the choice intentional before downstream crates pin the serialization schema.
🤖 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/hex.rs` around lines 107 - 156, The current serde::serialize/deserialize functions and each wN::serialize/wN::deserialize always encode as lowercase hex strings; change them to branch on serializer.is_human_readable() / Deserializer::is_human_readable() so human-readable formats use hex strings (existing to_lower_hex_string()/from_hex paths) while non-human-readable formats serialize/deserialize raw bytes (e.g. serializer.serialize_bytes / deserialize_bytes or direct Vec<u8>/ [u8; N] deserialize paths). Update the top-level functions named serialize and deserialize in the serde module and the identically named functions inside each generated wN module to perform this dispatch so binary formats avoid the hex-string cost.
🤖 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/payload/proupregtx.rs`:
- Around line 9-19: The external import "dash_script::KeyId" is placed before
the internal crate imports; relocate the line so all internal imports (the
existing "use crate::..." lines such as DecodeError, Script, validation items,
wire, InputsHash, TxHash) remain grouped together and then add a blank line
followed by the external import "use dash_script::KeyId;" to match the header
order used in sibling files (e.g., prouprevtx.rs, cbtx.rs).
In `@pkgs/primitives/src/support.rs`:
- Around line 232-250: The Deserialize impl for DynBitset currently only checks
byte count; update it to also reject nonzero padding bits in the final byte
based on raw.num_bits. After computing num_bits (usize) and required bytes, if
num_bits % 8 != 0 compute bits_in_last = num_bits % 8 and mask = (1u8 <<
bits_in_last) - 1, then verify (raw.data[required - 1] & !mask) == 0; if not,
return serde::de::Error::custom describing "padding bits set" (include
raw.num_bits and the offending last byte) so the type cannot be deserialized
into an inconsistent DynBitset (fields: DynBitset::num_bits, DynBitset::data,
types: DynBitsetSerde, and behaviour referenced by count_ones(), get(),
iter_set_bits()).
---
Nitpick comments:
In `@pkgs/num/src/serialize.rs`:
- Around line 38-44: Replace the manual while loop with a Rust for-range to be
more idiomatic: iterate i over 0..byte_len (or use .enumerate() on an iterator)
instead of the while, keep the body logic that calls hex_val(b[i * 2]) and
hex_val(b[i * 2 + 1]) and assigns out[byte_len - 1 - i] = (hi << 4) | lo, and
remove the explicit i += 1; update any mutable i binding accordingly so behavior
remains identical.
In `@pkgs/p2p_core/Cargo.toml`:
- Line 17: The crate's feature group 'full' currently lists ["std", "serde"],
but per project feature-layout rules 'full' must only include "std"; remove
"serde" from the 'full' array in the Cargo.toml (the 'full' feature declaration)
so serde remains an opt-in feature, and apply the same change to the matching
'full' declaration in pkgs/types/Cargo.toml if present.
In `@pkgs/pkc/tests/bls_chia_llmq.rs`:
- Around line 208-213: sid_display is built by reversing sid_bytes into a
Vec<u8> which causes an unnecessary heap allocation; replace that allocation by
reversing into a fixed-size stack array (e.g., let mut rev = [0u8; 32]; for i in
0..32 { rev[i] = sid_bytes[31 - i]; }) and then call the same hex conversion
(to_lower_hex_string or hex::encode_lower) on rev, updating the code that
constructs sid_display (referencing sid_bytes and sid_display) to use the stack
buffer instead of collecting into Vec<u8>.
In `@pkgs/primitives/Cargo.toml`:
- Around line 19-20: The feature entries in Cargo.toml deviate from the required
layout: adjust the feature definitions so they follow the scheme default = [],
std = [...], full = ["std"], serde = ["dep:serde"]; specifically replace the
current serde = ["dep:serde", "dash-num/serde", "dash-script/serde",
"dash-types/serde"] with serde = ["dep:serde"], ensure the std feature contains
any platform/std subfeatures (keep or add "dash-num/serde", "dash-script/serde",
"dash-types/serde" under std if intended), and change full = ["std"] instead of
full = ["std", "serde"] so that the features named serde and full match the
guideline.
In `@pkgs/script/Cargo.toml`:
- Around line 15-16: The Cargo feature layout was changed incorrectly: the
feature "full" should not include "serde"; change the "full" feature back to
["std"] (remove "serde" from the "full" array), ensure "serde" remains its own
feature (serde = ["dep:serde", "dash-types/serde"] or per crate convention), and
keep the overall layout consistent with the guideline (default = [], std =
[...], full = ["std"], serde = ["dep:serde"]). Make these edits to the feature
definitions (the "full", "std", "serde", and "default" feature entries) so the
crate follows the workspace feature convention.
In `@pkgs/types/src/hex.rs`:
- Around line 107-156: The current serde::serialize/deserialize functions and
each wN::serialize/wN::deserialize always encode as lowercase hex strings;
change them to branch on serializer.is_human_readable() /
Deserializer::is_human_readable() so human-readable formats use hex strings
(existing to_lower_hex_string()/from_hex paths) while non-human-readable formats
serialize/deserialize raw bytes (e.g. serializer.serialize_bytes /
deserialize_bytes or direct Vec<u8>/ [u8; N] deserialize paths). Update the
top-level functions named serialize and deserialize in the serde module and the
identically named functions inside each generated wN module to perform this
dispatch so binary formats avoid the hex-string cost.
🪄 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: b82dd8a4-1022-4b01-a937-d1dfeaf6cde3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (85)
.github/workflows/pkg_p2p_core.yml.github/workflows/pkg_pkc.yml.github/workflows/pkg_primitives.yml.github/workflows/pkg_script.yml.github/workflows/pkg_types.ymlCargo.tomlREADME.mdpkgs/num/Cargo.tomlpkgs/num/src/arith256.rspkgs/num/src/compact.rspkgs/num/src/hash.rspkgs/num/src/lib.rspkgs/num/src/serialize.rspkgs/num/src/util.rspkgs/num/tests/serde.rspkgs/p2p_core/Cargo.tomlpkgs/p2p_core/src/primitives/filter_type.rspkgs/p2p_core/src/primitives/governance.rspkgs/p2p_core/src/primitives/mn_list.rspkgs/p2p_core/src/primitives/protocol_version.rspkgs/p2p_core/tests/addrv2.rspkgs/pkc/Cargo.tomlpkgs/pkc/src/bls_chia/pk.rspkgs/pkc/src/bls_chia/sig.rspkgs/pkc/src/bls_ietf/pk.rspkgs/pkc/src/bls_ietf/sig.rspkgs/pkc/src/common/bls/mod.rspkgs/pkc/src/k256/pk.rspkgs/pkc/src/k256/sig.rspkgs/pkc/tests/bls_chia_aggregate.rspkgs/pkc/tests/bls_chia_dh.rspkgs/pkc/tests/bls_chia_keygen.rspkgs/pkc/tests/bls_chia_llmq.rspkgs/pkc/tests/bls_chia_ser.rspkgs/pkc/tests/bls_chia_sign.rspkgs/pkc/tests/bls_ietf_aggregate.rspkgs/pkc/tests/bls_ietf_dh.rspkgs/pkc/tests/bls_ietf_keygen.rspkgs/pkc/tests/bls_ietf_llmq.rspkgs/pkc/tests/bls_ietf_sign.rspkgs/pkc/tests/k256_keygen.rspkgs/pkc/tests/k256_sign.rspkgs/pow/Cargo.tomlpkgs/pow/tests/common/mod.rspkgs/primitives/Cargo.tomlpkgs/primitives/src/gov.rspkgs/primitives/src/lib.rspkgs/primitives/src/outpoint.rspkgs/primitives/src/payload/assetunlock.rspkgs/primitives/src/payload/cbtx.rspkgs/primitives/src/payload/mnhftx.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/types/mod.rspkgs/primitives/src/validation.rspkgs/primitives/tests/assetlock.rspkgs/primitives/tests/assetunlock.rspkgs/primitives/tests/blocks.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/qctx.rspkgs/primitives/tests/spend.rspkgs/primitives/tests/triggers.rspkgs/primitives/tests/util/mod.rspkgs/script/Cargo.tomlpkgs/script/src/key_id.rspkgs/types/Cargo.tomlpkgs/types/src/hex.rspkgs/types/src/lib.rspkgs/types/src/uint.rsunconv.toml
💤 Files with no reviewable changes (3)
- pkgs/primitives/src/types/mod.rs
- pkgs/pkc/src/common/bls/mod.rs
- pkgs/num/src/compact.rs
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Motivation
bitcoin-consensus-encoding(andrust-bitcoinat large) have settled for using thehex-conservativecrate (source). This pull request drops usage of thehexcrate and switches over tohex-conservativeas part of general alignment with upstream dependency choice.Additional Information
Depends on sdk%feat: add
serdeawareness to crates, move newtype definitions to new cratedash-types, dropbincodesupport #1Dependency for sdk%fix: fix incorrect serialisation constants (max script size, P2P msg len), fix
MnListDiffPayloadserialisation routines #3How Has This Been Tested?
cargo test --features full,_internal cargo clippy --features full,_internal --tests cargo fmt --checkChecklist