Draft
Conversation
… ManagedWalletInfo
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
Contributor
📝 WalkthroughWalkthroughThis pull request refactors transaction handling across multiple crates by introducing a new high-level transaction module in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WalletManager
participant TransactionBuilder
participant UtxoSelector
participant Signer
Client->>WalletManager: wallet_build_and_sign_transaction(wallet_id, account_index, outputs, fee_rate)
WalletManager->>TransactionBuilder: new(wallet_manager, wallet_id, account_index)
TransactionBuilder->>TransactionBuilder: add_output(address, amount)
TransactionBuilder->>TransactionBuilder: set_fee_rate(fee_rate)
TransactionBuilder->>TransactionBuilder: build()
TransactionBuilder->>UtxoSelector: select(target_amount, utxos, current_height)
UtxoSelector->>UtxoSelector: apply strategy (SmallestFirst/BranchAndBound/etc)
UtxoSelector-->>TransactionBuilder: selected_utxos
TransactionBuilder->>TransactionBuilder: calculate_fee(inputs, outputs)
TransactionBuilder->>TransactionBuilder: apply BIP-69 ordering
TransactionBuilder->>Signer: sign_inputs(private_key, tx)
Signer-->>TransactionBuilder: signed_transaction
TransactionBuilder-->>WalletManager: Transaction
WalletManager-->>Client: signed_tx_bytes, fee
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-wallet-ffi/src/transaction.rs (2)
68-73:⚠️ Potential issue | 🟡 MinorUpdate safety docs to reflect the new ABI.
The safety block still documents a
walletpointer andfee_rate, but this function now takeswallet_idandfee_per_kb. At the FFI boundary, this mismatch is easy to misuse and should be corrected at the Rust source so generated headers/docs stay consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 68 - 73, The safety doc comment in key-wallet-ffi/src/transaction.rs still mentions `wallet` and `fee_rate` but the function ABI now uses `wallet_id` and `fee_per_kb`; update the safety block to list the correct parameters (e.g., `manager` as pointer to FFIWalletManager, `wallet_id` as the wallet identifier (type used in the ABI), `account_index`, `outputs`/`outputs_count`, `fee_per_kb` (instead of `fee_rate`), and `fee_out` as a non-null pointer to u64) so generated headers/docs match the actual signature (locate and edit the doc comment immediately above the FFI function in transaction.rs).
143-153:⚠️ Potential issue | 🔴 CriticalFix unresolved
walletreference in address network validation.At line 143, the code references
wallet.network, but no localwalletvariable exists in this function. After the refactoring to usewallet_id, this is now undefined and causes a compilation error.The simplest fix is to use
manager_ref.network()instead, which is already available in scope:// Verify network matches let addr_network = addr.require_network(manager_ref.network()).map_err(|e| {
manager_refis theFFIWalletManagerreference created at line 107 and is accessible throughout the async block, and it has a publicnetwork()method that returns the network.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-ffi/src/transaction.rs` around lines 143 - 153, The code references a non-existent local `wallet.network` when calling `addr.require_network(...)`; replace that reference with the available `manager_ref.network()` (the `FFIWalletManager` instance in scope) so the call becomes `addr.require_network(manager_ref.network())` and keeps the existing error mapping via `map_err`/`FFIError::set_error`; update any type mismatches if necessary to match `manager_ref.network()`'s return type.
🧹 Nitpick comments (2)
key-wallet-manager/src/wallet_manager/mod.rs (1)
42-53: Consider addingPartialEq, Eqderives toAccountTypePreference.The enum has
Debug, Clone, Copybut lacks equality traits. AddingPartialEq, Eqwould enable comparisons in tests and conditionals without pattern matching.Suggested change
/// Account type preference for transaction building -#[derive(Debug, Clone, Copy)] +#[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum AccountTypePreference {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet-manager/src/wallet_manager/mod.rs` around lines 42 - 53, The enum AccountTypePreference is missing equality derives which prevents simple ==/!= comparisons; update its derive list on the AccountTypePreference declaration (where Debug, Clone, Copy are specified) to also include PartialEq and Eq so callers and tests can compare values directly.key-wallet/src/transaction/mod.rs (1)
1-7: LGTM! Clean module structure with selective re-exports.Consider whether
UtxoSelectorandSelectionErrorfromcoin_selectionshould also be re-exported, as external consumers may need them to useUtxoSelectorStrategyeffectively.Potential additional re-exports
pub use builder::TransactionBuilder; -pub use coin_selection::UtxoSelectorStrategy; +pub use coin_selection::{SelectionError, UtxoSelector, UtxoSelectorStrategy}; pub use fee::FeeRate;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/transaction/mod.rs` around lines 1 - 7, The module exports currently expose TransactionBuilder, UtxoSelectorStrategy, and FeeRate but not the related types UtxoSelector and SelectionError which consumers of UtxoSelectorStrategy may need; update key-wallet/src/transaction/mod.rs to re-export coin_selection::UtxoSelector and coin_selection::SelectionError alongside the existing pub use lines so external code can construct/select UTXO implementations and handle selection errors when using UtxoSelectorStrategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet/Cargo.toml`:
- Line 25: The Cargo.toml entry adding key-wallet-manager as a dependency
creates a circular crate dependency between key-wallet and key-wallet-manager;
remove that direct dependency and break the cycle by extracting shared
types/interfaces into a new crate (e.g., key-wallet-types) that both key-wallet
and key-wallet-manager depend on, or move the implementation that requires
cross-crate access into a single crate (key-wallet or key-wallet-manager) or
convert the shared API into trait-based interfaces to eliminate the mutual
dependency; update Cargo.toml to depend on the new key-wallet-types crate and
adjust imports/usages in functions/modules referencing types from
key-wallet-manager (or move those functions into the owning crate).
In `@key-wallet/src/transaction/builder.rs`:
- Line 43: The error string for the enum variant Self::ZeroValueOutputs is
misspelled; update the write! call that returns "Sero value outputs" to the
correct "Zero value outputs" in the Display/Debug implementation (the line using
write!(f, "...") for Self::ZeroValueOutputs) so diagnostic messages read
correctly.
- Around line 162-166: The code currently sums outputs using unchecked u64
arithmetic and later adds current_fee and DUST without panic-safe checks, which
can overflow; update the logic in the transaction builder where total_output is
computed (the iterator using self.outputs.iter().map(|out| out.value).sum()) to
use checked summation (e.g., fold with checked_add) and validate each addition,
and also replace the unchecked addition of total_output + current_fee + DUST
with sequential checked_add calls, returning an appropriate
TransactionBuildingError (e.g., an Overflow variant or extend
TransactionBuildingError) when any checked_add returns None; also apply the same
checked-add pattern to the other occurrence mentioned (the addition at the other
coin-selection/fee calculation site).
- Around line 301-367: The code computes sighashes before populating tx.input
which yields incorrect hashes; fix by first building and assigning unsigned TxIn
entries (with empty script_sig and proper previous_output/sequence) into
tx.input (the same inputs Vec used now), then create a SighashCache::new(&*tx)
and iterate over utxos with their index to compute legacy_signature_hash(index,
script_pubkey, ...) and sign, and finally replace or set
tx.input[index].script_sig with the generated script_sig (keeping existing use
of root_xpriv/derive_priv, secp, Message/from_digest, signature.serialize_der,
and EcdsaSighashType). Ensure you reference and update tx.input,
SighashCache::legacy_signature_hash, and the TxIn entries rather than signing
before tx.input is assigned.
- Around line 84-106: TransactionBuilder::new currently panics due to todo!()
fields (synced_height, wallet_type, change_address, available_utxos,
managed_account_type, managed_account, account); change its signature to return
Result<Self, TransactionBuilderError> (or an existing error type) and initialize
those fields from the provided WalletManager<&ManagedWalletInfo> and WalletId
(or return Err if required data is missing), remove all todo!() usage, and
ensure callers (including the FFI entry that invokes this constructor) handle
the Result instead of expecting a panic; keep FeeRate, selection_strategy,
version, lock_time, outputs, and special_transaction_payload initialization
as-is when successful.
In `@key-wallet/src/transaction/coin_selection.rs`:
- Around line 242-254: The recursion in find_combination_recursive returns None
as soon as remaining_picks == 0, which skips the exact-match check; change the
logic so you first check if current_total == target (and return Some(current) if
true) before returning None when remaining_picks == 0, or reorder the two checks
(check exact match prior to the remaining_picks guard) so exact-match cases with
remaining_picks == 0 are correctly returned.
- Around line 87-93: UtxoSelectorStrategy::SmallestFirstTill currently truncates
the sorted utxos with utxos.split_at(*v) before attempting selection, which
discards higher-value UTXOs and can cause false InsufficientFunds results;
change the logic to first iterate/select from the smallest utxos up to v
candidates without dropping the remainder (e.g., create an iterator that yields
the smallest v items or use take(v) on the sorted iterator) and pass that
iterator to select_utxo_from_iterator(target) so selection only limits which
candidates are considered, not permanently discarding others if they’re needed.
- Around line 9-10: The module imports rand::seq::SliceRandom and
rand::thread_rng which are only available when the optional std feature is
enabled; wrap the Random strategy (or the entire coin_selection.rs module) with
a feature gate to avoid no_std compilation failures by adding #[cfg(feature =
"std")] to the Random-related code (or top of the module) and, if needed, use
#[cfg_attr(not(feature = "std"), allow(unused_imports))] or provide a no-op
fallback implementation for the coin selection API so compilation succeeds
without the rand types; locate the Random strategy implementation and the
imports (thread_rng, SliceRandom) in coin_selection.rs and apply the cfg(feature
= "std") guard there.
---
Outside diff comments:
In `@key-wallet-ffi/src/transaction.rs`:
- Around line 68-73: The safety doc comment in key-wallet-ffi/src/transaction.rs
still mentions `wallet` and `fee_rate` but the function ABI now uses `wallet_id`
and `fee_per_kb`; update the safety block to list the correct parameters (e.g.,
`manager` as pointer to FFIWalletManager, `wallet_id` as the wallet identifier
(type used in the ABI), `account_index`, `outputs`/`outputs_count`, `fee_per_kb`
(instead of `fee_rate`), and `fee_out` as a non-null pointer to u64) so
generated headers/docs match the actual signature (locate and edit the doc
comment immediately above the FFI function in transaction.rs).
- Around line 143-153: The code references a non-existent local `wallet.network`
when calling `addr.require_network(...)`; replace that reference with the
available `manager_ref.network()` (the `FFIWalletManager` instance in scope) so
the call becomes `addr.require_network(manager_ref.network())` and keeps the
existing error mapping via `map_err`/`FFIError::set_error`; update any type
mismatches if necessary to match `manager_ref.network()`'s return type.
---
Nitpick comments:
In `@key-wallet-manager/src/wallet_manager/mod.rs`:
- Around line 42-53: The enum AccountTypePreference is missing equality derives
which prevents simple ==/!= comparisons; update its derive list on the
AccountTypePreference declaration (where Debug, Clone, Copy are specified) to
also include PartialEq and Eq so callers and tests can compare values directly.
In `@key-wallet/src/transaction/mod.rs`:
- Around line 1-7: The module exports currently expose TransactionBuilder,
UtxoSelectorStrategy, and FeeRate but not the related types UtxoSelector and
SelectionError which consumers of UtxoSelectorStrategy may need; update
key-wallet/src/transaction/mod.rs to re-export coin_selection::UtxoSelector and
coin_selection::SelectionError alongside the existing pub use lines so external
code can construct/select UTXO implementations and handle selection errors when
using UtxoSelectorStrategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30255fe2-48eb-4a2a-8311-8ada1c6724d6
📒 Files selected for processing (19)
key-wallet-ffi/include/key_wallet_ffi.hkey-wallet-ffi/src/transaction.rskey-wallet-manager/examples/wallet_creation.rskey-wallet-manager/src/lib.rskey-wallet-manager/src/wallet_manager/mod.rskey-wallet-manager/src/wallet_manager/transaction_building.rskey-wallet-manager/tests/integration_test.rskey-wallet/Cargo.tomlkey-wallet/src/lib.rskey-wallet/src/transaction/builder.rskey-wallet/src/transaction/coin_selection.rskey-wallet/src/transaction/fee.rskey-wallet/src/transaction/mod.rskey-wallet/src/wallet/managed_wallet_info/coin_selection.rskey-wallet/src/wallet/managed_wallet_info/mod.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_building.rskey-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rskey-wallet/tests/test_optimal_consolidation.rs
💤 Files with no reviewable changes (8)
- key-wallet/src/transaction/fee.rs
- key-wallet-manager/src/wallet_manager/transaction_building.rs
- key-wallet-manager/src/lib.rs
- key-wallet/src/wallet/managed_wallet_info/transaction_building.rs
- key-wallet/src/wallet/managed_wallet_info/mod.rs
- key-wallet/src/wallet/managed_wallet_info/coin_selection.rs
- key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
- key-wallet/tests/test_optimal_consolidation.rs
| internals = { path = "../internals", package = "dashcore-private" } | ||
| dashcore_hashes = { path = "../hashes", default-features = false } | ||
| dashcore = { path="../dash" } | ||
| key-wallet-manager = { path = "../key-wallet-manager", features = ["std"] } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the key-wallet and key-wallet-manager Cargo.toml files
find . -name "Cargo.toml" -path "*/key-wallet*" | head -20Repository: dashpay/rust-dashcore
Length of output: 148
🏁 Script executed:
# Read the key-wallet/Cargo.toml file to see the dependency at line 25
if [ -f "key-wallet/Cargo.toml" ]; then
echo "=== key-wallet/Cargo.toml ==="
cat -n key-wallet/Cargo.toml | head -40
fiRepository: dashpay/rust-dashcore
Length of output: 2548
🏁 Script executed:
# Read the key-wallet-manager/Cargo.toml file to check dependencies
if [ -f "key-wallet-manager/Cargo.toml" ]; then
echo "=== key-wallet-manager/Cargo.toml ==="
cat -n key-wallet-manager/Cargo.toml
fiRepository: dashpay/rust-dashcore
Length of output: 1737
Circular dependency detected between key-wallet and key-wallet-manager.
Adding key-wallet-manager as a dependency here creates a cycle: key-wallet → key-wallet-manager (line 20) → key-wallet. Cargo will fail to resolve this dependency graph and prevent compilation.
Consider restructuring by either:
- Extracting shared types into a separate
key-wallet-typescrate that both depend on - Moving the functionality that requires cross-crate access into one of the crates
- Using traits/interfaces to break the dependency cycle
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/Cargo.toml` at line 25, The Cargo.toml entry adding
key-wallet-manager as a dependency creates a circular crate dependency between
key-wallet and key-wallet-manager; remove that direct dependency and break the
cycle by extracting shared types/interfaces into a new crate (e.g.,
key-wallet-types) that both key-wallet and key-wallet-manager depend on, or move
the implementation that requires cross-crate access into a single crate
(key-wallet or key-wallet-manager) or convert the shared API into trait-based
interfaces to eliminate the mutual dependency; update Cargo.toml to depend on
the new key-wallet-types crate and adjust imports/usages in functions/modules
referencing types from key-wallet-manager (or move those functions into the
owning crate).
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| match self { | ||
| Self::NoOutputs => write!(f, "No outputs provided"), | ||
| Self::ZeroValueOutputs => write!(f, "Sero value outputs"), |
Contributor
There was a problem hiding this comment.
Fix typo in error message string.
"Sero value outputs" should be "Zero value outputs" for clearer diagnostics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/transaction/builder.rs` at line 43, The error string for the
enum variant Self::ZeroValueOutputs is misspelled; update the write! call that
returns "Sero value outputs" to the correct "Zero value outputs" in the
Display/Debug implementation (the line using write!(f, "...") for
Self::ZeroValueOutputs) so diagnostic messages read correctly.
Comment on lines
+84
to
+106
| pub fn new( | ||
| wallet_manager: &'a WalletManager<ManagedWalletInfo>, | ||
| wallet_id: &'a WalletId, | ||
| account_index: u32, | ||
| ) -> Self { | ||
| Self { | ||
| synced_height: todo!(), | ||
| wallet_type: todo!(), | ||
| change_address: todo!(), | ||
| available_utxos: todo!(), | ||
| managed_account_type: todo!(), | ||
| managed_account: todo!(), | ||
| account: todo!(), | ||
|
|
||
| fee_rate: FeeRate::normal(), | ||
| selection_strategy: UtxoSelectorStrategy::OptimalConsolidation, | ||
|
|
||
| version: 2, | ||
| lock_time: 0, | ||
| outputs: Vec::new(), | ||
| special_transaction_payload: None, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the builder file exists and get its structure
find . -name "builder.rs" -path "*/transaction/*" | head -5Repository: dashpay/rust-dashcore
Length of output: 104
🏁 Script executed:
# Check lines 84-106 in the builder.rs file
sed -n '84,106p' key-wallet/src/transaction/builder.rsRepository: dashpay/rust-dashcore
Length of output: 755
🏁 Script executed:
# Search for all todo!() markers in the builder file
rg -n 'todo!\(' key-wallet/src/transaction/builder.rsRepository: dashpay/rust-dashcore
Length of output: 341
🏁 Script executed:
# Check FFI call-sites
rg -n 'TransactionBuilder::new' key-wallet-ffi/src/transaction.rsRepository: dashpay/rust-dashcore
Length of output: 166
🏁 Script executed:
# Check the signature of TransactionBuilder::new to confirm return type
rg -B 2 'fn new' key-wallet/src/transaction/builder.rs | head -20Repository: dashpay/rust-dashcore
Length of output: 155
TransactionBuilder::new is currently a panic path.
Core fields are still todo!(), so every call to new() aborts instead of returning a structured error. This violates the guideline to never panic in library code and return Result<T> for all fallible operations. Since this is called from FFI at line 113 in key-wallet-ffi/src/transaction.rs, a panic would crash the entire process.
💡 Directional fix
-pub fn new(
+pub fn new(
wallet_manager: &'a WalletManager<ManagedWalletInfo>,
wallet_id: &'a WalletId,
account_index: u32,
-) -> Self {
+) -> Result<Self, TransactionBuildingError> {
- Self {
- synced_height: todo!(),
- wallet_type: todo!(),
- change_address: todo!(),
- available_utxos: todo!(),
- managed_account_type: todo!(),
- managed_account: todo!(),
- account: todo!(),
+ // Resolve wallet/info/account and return explicit errors on failure.
+ Ok(Self {
+ synced_height: /* ... */,
+ wallet_type: /* ... */,
+ change_address: /* ... */,
+ available_utxos: /* ... */,
+ managed_account_type: /* ... */,
+ managed_account: /* ... */,
+ account: /* ... */,
@@
- }
+ })
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/transaction/builder.rs` around lines 84 - 106,
TransactionBuilder::new currently panics due to todo!() fields (synced_height,
wallet_type, change_address, available_utxos, managed_account_type,
managed_account, account); change its signature to return Result<Self,
TransactionBuilderError> (or an existing error type) and initialize those fields
from the provided WalletManager<&ManagedWalletInfo> and WalletId (or return Err
if required data is missing), remove all todo!() usage, and ensure callers
(including the FFI entry that invokes this constructor) handle the Result
instead of expecting a panic; keep FeeRate, selection_strategy, version,
lock_time, outputs, and special_transaction_payload initialization as-is when
successful.
Comment on lines
+162
to
+166
| let total_output: u64 = self.outputs.iter().map(|out| out.value).sum(); | ||
|
|
||
| if total_output == 0 && self.special_transaction_payload.is_none() { | ||
| return Err(TransactionBuildingError::ZeroValueOutputs); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard u64 amount math with checked arithmetic.
sum() and total_output + current_fee + DUST are unchecked. Large/untrusted amounts can overflow and produce incorrect coin-selection targets.
🛡️ Proposed fix
- let total_output: u64 = self.outputs.iter().map(|out| out.value).sum();
+ let total_output = self.outputs.iter().try_fold(0u64, |acc, out| {
+ acc.checked_add(out.value).ok_or_else(|| {
+ TransactionBuildingError::InvalidAmount("Output total overflow".to_string())
+ })
+ })?;
@@
- total_output + current_fee + DUST,
+ total_output
+ .checked_add(current_fee)
+ .and_then(|v| v.checked_add(DUST))
+ .ok_or_else(|| {
+ TransactionBuildingError::InvalidAmount(
+ "Selection target overflow".to_string(),
+ )
+ })?,As per coding guidelines Always validate inputs from untrusted sources.
Also applies to: 209-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/transaction/builder.rs` around lines 162 - 166, The code
currently sums outputs using unchecked u64 arithmetic and later adds current_fee
and DUST without panic-safe checks, which can overflow; update the logic in the
transaction builder where total_output is computed (the iterator using
self.outputs.iter().map(|out| out.value).sum()) to use checked summation (e.g.,
fold with checked_add) and validate each addition, and also replace the
unchecked addition of total_output + current_fee + DUST with sequential
checked_add calls, returning an appropriate TransactionBuildingError (e.g., an
Overflow variant or extend TransactionBuildingError) when any checked_add
returns None; also apply the same checked-add pattern to the other occurrence
mentioned (the addition at the other coin-selection/fee calculation site).
Comment on lines
+301
to
+367
| let cache = SighashCache::new(&*tx); | ||
|
|
||
| for (index, &utxo) in utxos.iter().enumerate() { | ||
| let key = { | ||
| // Look up the derivation path for this UTXO's address | ||
| let path = address_to_path.get(&utxo.address).ok_or( | ||
| TransactionBuildingError::SigningFailed(format!( | ||
| "Derivation path not found for address {}", | ||
| utxo.address, | ||
| )), | ||
| )?; | ||
|
|
||
| // Convert root key to ExtendedPrivKey and derive the child key | ||
| let root_ext_priv = root_xpriv.to_extended_priv_key(self.account.network); | ||
| let secp = secp256k1::Secp256k1::new(); | ||
| let derived_xpriv = root_ext_priv | ||
| .derive_priv(&secp, path) | ||
| .map_err(|e| TransactionBuildingError::SigningFailed(e.to_string()))?; | ||
|
|
||
| derived_xpriv.private_key | ||
| }; | ||
|
|
||
| // Get the script pubkey from the UTXO | ||
| let script_pubkey = &utxo.txout.script_pubkey; | ||
|
|
||
| // Create signature hash for P2PKH | ||
| let sighash = cache | ||
| .legacy_signature_hash(index, script_pubkey, EcdsaSighashType::All.to_u32()) | ||
| .map_err(|e| { | ||
| TransactionBuildingError::SigningFailed(format!( | ||
| "Failed to compute sighash: {}", | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| // Sign the hash | ||
| let message = Message::from_digest(*sighash.as_byte_array()); | ||
| let signature = secp.sign_ecdsa(&message, &key); | ||
|
|
||
| // Create script signature (P2PKH) | ||
| let mut sig_bytes = signature.serialize_der().to_vec(); | ||
| sig_bytes.push(EcdsaSighashType::All.to_u32() as u8); | ||
|
|
||
| let pubkey = secp256k1::PublicKey::from_secret_key(&secp, &key); | ||
|
|
||
| let script_sig = Builder::new() | ||
| .push_slice(<&PushBytes>::try_from(sig_bytes.as_slice()).map_err(|_| { | ||
| TransactionBuildingError::SigningFailed("Invalid signature length".into()) | ||
| })?) | ||
| .push_slice(pubkey.serialize()) | ||
| .into_script(); | ||
|
|
||
| // Map the UTXO to TxIn with the new scrip_sig | ||
| // Dash doesn't use RBF, so we use the standard sequence number | ||
| let sequence = 0xffffffff; | ||
|
|
||
| let txin = TxIn { | ||
| previous_output: utxo.outpoint, | ||
| script_sig, | ||
| sequence, | ||
| witness: dashcore::blockdata::witness::Witness::new(), | ||
| }; | ||
|
|
||
| inputs.push(txin); | ||
| } | ||
|
|
||
| tx.input = inputs; |
Contributor
There was a problem hiding this comment.
Sign after materializing unsigned inputs into tx.input.
This routine computes sighashes before the selected inputs are inserted into the transaction. The sighash computation depends on indexed inputs already existing in tx.input, so this ordering can fail immediately or sign against stale structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/transaction/builder.rs` around lines 301 - 367, The code
computes sighashes before populating tx.input which yields incorrect hashes; fix
by first building and assigning unsigned TxIn entries (with empty script_sig and
proper previous_output/sequence) into tx.input (the same inputs Vec used now),
then create a SighashCache::new(&*tx) and iterate over utxos with their index to
compute legacy_signature_hash(index, script_pubkey, ...) and sign, and finally
replace or set tx.input[index].script_sig with the generated script_sig (keeping
existing use of root_xpriv/derive_priv, secp, Message/from_digest,
signature.serialize_der, and EcdsaSighashType). Ensure you reference and update
tx.input, SighashCache::legacy_signature_hash, and the TxIn entries rather than
signing before tx.input is assigned.
Comment on lines
+9
to
+10
| use rand::seq::SliceRandom; | ||
| use rand::thread_rng; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual file and its imports
head -20 key-wallet/src/transaction/coin_selection.rsRepository: dashpay/rust-dashcore
Length of output: 553
🏁 Script executed:
# Check the Cargo.toml to understand the crate setup and dependencies
cat key-wallet/Cargo.tomlRepository: dashpay/rust-dashcore
Length of output: 2821
🏁 Script executed:
# Search for no_std references in the crate
rg "no_std|forbid\|deny" key-wallet/src/ -A 2Repository: dashpay/rust-dashcore
Length of output: 681
🏁 Script executed:
# Check how thread_rng is actually used in the file
rg "thread_rng|Random" key-wallet/src/transaction/coin_selection.rs -B 2 -A 3Repository: dashpay/rust-dashcore
Length of output: 832
🏁 Script executed:
# Check how the transaction module is exposed in lib.rs or transaction/mod.rs
rg "mod transaction|pub mod transaction" key-wallet/src/ -B 2 -A 2Repository: dashpay/rust-dashcore
Length of output: 1164
🏁 Script executed:
# Check if coin_selection is feature-gated
rg "cfg\(feature" key-wallet/src/transaction/ -B 1 -A 1Repository: dashpay/rust-dashcore
Length of output: 635
🏁 Script executed:
# Look at the structure of the transaction module
cat key-wallet/src/transaction/mod.rs | head -50Repository: dashpay/rust-dashcore
Length of output: 212
🏁 Script executed:
# Check if other modules that use std-only deps are feature-gated
rg "use std::|use rand" key-wallet/src/ --type rs | head -20Repository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
# Look at the builder.rs to see if it handles std-only deps properly
cat key-wallet/src/transaction/builder.rs | head -100Repository: dashpay/rust-dashcore
Length of output: 3640
🏁 Script executed:
# Check if coin_selection functions are used anywhere that might require no_std support
rg "UtxoSelectorStrategy\|coin_selection" key-wallet/src/ --type rs | grep -v "^Binary"Repository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
# Check std imports in builder.rs and coin_selection.rs
grep -n "^use std::" key-wallet/src/transaction/builder.rs key-wallet/src/transaction/coin_selection.rsRepository: dashpay/rust-dashcore
Length of output: 136
🏁 Script executed:
# Check if the coin_selection module or UtxoSelectorStrategy has proper feature-gating
grep -n "cfg(feature" key-wallet/src/transaction/coin_selection.rsRepository: dashpay/rust-dashcore
Length of output: 91
🏁 Script executed:
# Examine the full coin_selection.rs file to see if the Random strategy is feature-gated
wc -l key-wallet/src/transaction/coin_selection.rsRepository: dashpay/rust-dashcore
Length of output: 113
🏁 Script executed:
# Get the actual lines where thread_rng is used with context
sed -n '80,105p' key-wallet/src/transaction/coin_selection.rsRepository: dashpay/rust-dashcore
Length of output: 1029
🏁 Script executed:
# Check if transaction module itself needs to be feature-gated or if Random strategy alone needs gating
grep -B5 -A5 "UtxoSelectorStrategy::Random" key-wallet/src/transaction/coin_selection.rsRepository: dashpay/rust-dashcore
Length of output: 539
🏁 Script executed:
# Check if there are other std-only imports in coin_selection.rs
head -40 key-wallet/src/transaction/coin_selection.rs | grep -n "use"Repository: dashpay/rust-dashcore
Length of output: 187
🏁 Script executed:
# Check if transaction module declaration in lib.rs has any feature-gating
grep -B3 -A3 "pub mod transaction" key-wallet/src/lib.rsRepository: dashpay/rust-dashcore
Length of output: 213
🏁 Script executed:
# Verify the actual feature of rand and if it requires getrandom
grep -A2 "^rand = " key-wallet/Cargo.tomlRepository: dashpay/rust-dashcore
Length of output: 230
🏁 Script executed:
# Check if there's a way coin_selection could work in no_std without rand
grep -n "cfg\|feature" key-wallet/src/transaction/coin_selection.rsRepository: dashpay/rust-dashcore
Length of output: 108
Feature-gate coin_selection.rs to prevent compilation errors in no_std builds.
The module unconditionally imports rand::thread_rng and rand::seq::SliceRandom (lines 9–10), but rand is an optional dependency only available with the std feature. This will fail to compile when building without the std feature. Wrap the Random strategy implementation or the entire module with #[cfg(feature = "std")].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/transaction/coin_selection.rs` around lines 9 - 10, The module
imports rand::seq::SliceRandom and rand::thread_rng which are only available
when the optional std feature is enabled; wrap the Random strategy (or the
entire coin_selection.rs module) with a feature gate to avoid no_std compilation
failures by adding #[cfg(feature = "std")] to the Random-related code (or top of
the module) and, if needed, use #[cfg_attr(not(feature = "std"),
allow(unused_imports))] or provide a no-op fallback implementation for the coin
selection API so compilation succeeds without the rand types; locate the Random
strategy implementation and the imports (thread_rng, SliceRandom) in
coin_selection.rs and apply the cfg(feature = "std") guard there.
Comment on lines
+87
to
+93
| UtxoSelectorStrategy::SmallestFirstTill(v) => { | ||
| utxos.sort_by_key(|&u| u.value()); | ||
| let (utxos, _) = utxos.split_at(*v as usize); | ||
| let iter = utxos.to_vec().into_iter(); | ||
|
|
||
| select_utxo_from_iterator(iter, target) | ||
| } |
Contributor
There was a problem hiding this comment.
SmallestFirstTill truncates UTXOs before checking if target can be met.
When v is less than the number of UTXOs needed to meet the target, this will return InsufficientFunds even if remaining UTXOs could satisfy it. The split happens before selection, discarding potentially needed UTXOs.
Suggested fix
UtxoSelectorStrategy::SmallestFirstTill(v) => {
utxos.sort_by_key(|&u| u.value());
- let (utxos, _) = utxos.split_at(*v as usize);
- let iter = utxos.to_vec().into_iter();
-
- select_utxo_from_iterator(iter, target)
+ let limit = (*v as usize).min(utxos.len());
+ let (small_utxos, remaining) = utxos.split_at(limit);
+ // Try with limited set first, fall back to full set if insufficient
+ let iter = small_utxos.iter().chain(remaining.iter()).copied();
+ select_utxo_from_iterator(iter, target)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/transaction/coin_selection.rs` around lines 87 - 93,
UtxoSelectorStrategy::SmallestFirstTill currently truncates the sorted utxos
with utxos.split_at(*v) before attempting selection, which discards higher-value
UTXOs and can cause false InsufficientFunds results; change the logic to first
iterate/select from the smallest utxos up to v candidates without dropping the
remainder (e.g., create an iterator that yields the smallest v items or use
take(v) on the sorted iterator) and pass that iterator to
select_utxo_from_iterator(target) so selection only limits which candidates are
considered, not permanently discarding others if they’re needed.
Comment on lines
+242
to
+254
| if remaining_picks == 0 { | ||
| return None; | ||
| } | ||
|
|
||
| // Check if we've found an exact match | ||
| if current_total == target { | ||
| return Some(current); | ||
| } | ||
|
|
||
| // Prune if we've exceeded the target | ||
| if current_total > target { | ||
| return None; | ||
| } |
Contributor
There was a problem hiding this comment.
find_combination_recursive returns None before checking exact match when remaining_picks == 0.
When remaining_picks reaches 0, the function returns None without checking if current_total == target. This misses valid exact-match cases where the exact number of picks yields the target.
Suggested fix
fn find_combination_recursive<'a>(
utxos: &[&'a Utxo],
target: u64,
remaining_picks: usize,
index: usize,
current: Vec<&'a Utxo>,
current_total: u64,
) -> Option<Vec<&'a Utxo>> {
- if remaining_picks == 0 {
- return None;
- }
-
// Check if we've found an exact match
if current_total == target {
return Some(current);
}
+ if remaining_picks == 0 {
+ return None;
+ }
+
// Prune if we've exceeded the target
if current_total > target {
return None;
}📝 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
| if remaining_picks == 0 { | |
| return None; | |
| } | |
| // Check if we've found an exact match | |
| if current_total == target { | |
| return Some(current); | |
| } | |
| // Prune if we've exceeded the target | |
| if current_total > target { | |
| return None; | |
| } | |
| if current_total == target { | |
| return Some(current); | |
| } | |
| if remaining_picks == 0 { | |
| return None; | |
| } | |
| // Prune if we've exceeded the target | |
| if current_total > target { | |
| return None; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/transaction/coin_selection.rs` around lines 242 - 254, The
recursion in find_combination_recursive returns None as soon as remaining_picks
== 0, which skips the exact-match check; change the logic so you first check if
current_total == target (and return Some(current) if true) before returning None
when remaining_picks == 0, or reorder the two checks (check exact match prior to
the remaining_picks guard) so exact-match cases with remaining_picks == 0 are
correctly returned.
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.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Breaking Changes
create_unsigned_payment_transactionmethod removed from wallet interface.Refactor