fix: Properly handle immature transactions #223
Conversation
|
Warning Rate limit exceeded@xdustinface has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughThe changes introduce a chain state synchronization mechanism by adding an Changes
Sequence DiagramsequenceDiagram
actor BP as Block Processor
participant WM as WalletManager
participant WI as WalletInfo
participant Acct as ManagedAccount
BP->>WM: process_block(block_data)
WM->>WI: process_block(...)
WI->>Acct: insert_transaction(tx_record)
activate Acct
Acct-->>WI: ✓ inserted
deactivate Acct
WI->>Acct: add_utxos_from_transaction(tx, addrs, net, height, confirmed)
activate Acct
Acct->>Acct: iterate tx outputs & filter by involved_addresses
Acct->>Acct: create UTXO entries with address, height, coinbase flag
Acct->>Acct: insert into utxos map
Acct-->>WI: ✓ UTXOs added
deactivate Acct
WI-->>WM: ✓ block processed
BP->>WM: update_chain_height(network, height)
WM->>WI: update_chain_height(network, height)
note over WI,Acct: Triggers coinbase maturation<br/>logic for 100 confirmations
WI-->>WM: ✓ height updated
WM-->>BP: ✓ chain state synchronized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
xdustinface
force-pushed
the
fix/immature-processing
branch
from
0ad210d to
aac7324
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
key-wallet/src/managed_account/managed_account_trait.rs (1)
47-54: Add documentation for the new trait method.The method lacks a doc comment explaining its purpose and parameters. Consider adding documentation similar to other trait methods:
+ /// Add UTXOs from a transaction for outputs that pay to the involved addresses. + /// + /// This method iterates through transaction outputs and creates UTXOs for + /// any outputs that pay to addresses in the `involved_addresses` set. fn add_utxos_from_transaction( &mut self, tx: &dashcore::Transaction, involved_addresses: &alloc::collections::BTreeSet<Address>, network: Network, height: u32, is_confirmed: bool, );key-wallet/src/managed_account/mod.rs (1)
851-883: UTXO ingestion logic looks solid; consider guarding against network mismatchesThe output scan and UTXO construction look correct and should fix the missing-UTXO-on-maturation issue. One subtle thing to harden:
- This impl trusts the
networkargument when callingAddress::from_script, but the account already hasself.network. If a caller ever passes a mismatched network,involved_addresses.contains(&addr)will silently fail and UTXOs just won’t be added.To make this more robust, you could add a lightweight guard:
fn add_utxos_from_transaction( &mut self, tx: &dashcore::Transaction, involved_addresses: &alloc::collections::BTreeSet<Address>, network: Network, height: u32, is_confirmed: bool, ) { - let txid = tx.txid(); + let txid = tx.txid(); + debug_assert_eq!( + network, + self.network, + "Network mismatch in add_utxos_from_transaction" + );This keeps the existing API but catches misuses early in debug builds. If you prefer, you could instead just use
self.networkinAddress::from_scriptand treat thenetworkparameter as informational.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
dash-spv/src/client/block_processor.rs(1 hunks)dash-spv/src/client/block_processor_test.rs(2 hunks)dash-spv/src/sync/sequential/message_handlers.rs(1 hunks)key-wallet-manager/src/wallet_interface.rs(1 hunks)key-wallet-manager/src/wallet_manager/process_block.rs(1 hunks)key-wallet/src/managed_account/managed_account_trait.rs(2 hunks)key-wallet/src/managed_account/mod.rs(1 hunks)key-wallet/src/transaction_checking/wallet_checker.rs(2 hunks)key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/src/client/block_processor.rskey-wallet/src/transaction_checking/wallet_checker.rsdash-spv/src/client/block_processor_test.rs
📚 Learning: 2025-08-21T05:01:58.949Z
Learnt from: QuantumExplorer
Repo: dashpay/rust-dashcore PR: 108
File: key-wallet-ffi/src/wallet_manager.rs:270-318
Timestamp: 2025-08-21T05:01:58.949Z
Learning: In the key-wallet-ffi design, wallets retrieved from the wallet manager via lookup functions should return const pointers (*const FFIWallet) to enforce read-only access and prevent unintended modifications. The wallet manager should control wallet lifecycle and mutations through specific APIs rather than allowing external mutation of retrieved wallet references.
Applied to files:
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs
🧬 Code graph analysis (8)
key-wallet-manager/src/wallet_interface.rs (2)
dash-spv/src/client/block_processor_test.rs (2)
update_chain_height(89-89)update_chain_height(305-305)key-wallet-manager/src/wallet_manager/process_block.rs (1)
update_chain_height(217-221)
dash-spv/src/client/block_processor.rs (1)
dash-spv/src/client/core.rs (1)
wallet(161-163)
key-wallet-manager/src/wallet_manager/process_block.rs (2)
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
update_chain_height(115-115)update_chain_height(394-406)key-wallet-manager/src/wallet_interface.rs (1)
update_chain_height(77-77)
key-wallet/src/managed_account/managed_account_trait.rs (5)
key-wallet/src/managed_account/mod.rs (2)
add_utxos_from_transaction(851-883)network(819-821)key-wallet/src/managed_account/address_pool.rs (1)
network(1100-1103)key-wallet/src/account/bls_account.rs (1)
network(201-203)key-wallet/src/account/mod.rs (1)
network(153-155)dash/src/address.rs (1)
network(944-946)
dash-spv/src/sync/sequential/message_handlers.rs (1)
dash-spv/src/client/core.rs (1)
wallet(161-163)
dash-spv/src/client/block_processor_test.rs (3)
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (2)
update_chain_height(115-115)update_chain_height(394-406)key-wallet-manager/src/wallet_interface.rs (1)
update_chain_height(77-77)key-wallet-manager/src/wallet_manager/process_block.rs (1)
update_chain_height(217-221)
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (6)
key-wallet/src/managed_account/mod.rs (1)
network(819-821)key-wallet/src/managed_account/address_pool.rs (1)
network(1100-1103)key-wallet/src/account/bls_account.rs (1)
network(201-203)key-wallet/src/account/eddsa_account.rs (1)
network(202-204)key-wallet/src/account/mod.rs (1)
network(153-155)dash/src/address.rs (1)
network(944-946)
key-wallet/src/managed_account/mod.rs (4)
key-wallet/src/managed_account/managed_account_trait.rs (2)
add_utxos_from_transaction(47-54)network(24-24)key-wallet/src/account/mod.rs (1)
network(153-155)key-wallet/src/account/account_trait.rs (1)
network(21-21)dash/src/address.rs (1)
network(944-946)
🔇 Additional comments (10)
key-wallet-manager/src/wallet_interface.rs (1)
72-77: LGTM!The new
update_chain_heightmethod is well-documented and appropriately placed in the trait. The documentation clearly explains its purpose (processing matured coinbase transactions after 100 confirmations).dash-spv/src/client/block_processor.rs (1)
230-232: LGTM!The
update_chain_heightcall is correctly positioned immediately afterprocess_blockwhile the wallet write lock is still held, ensuring atomic processing of the block and any matured coinbase transactions.dash-spv/src/sync/sequential/message_handlers.rs (1)
767-768: LGTM!Consistent with the
block_processor.rsimplementation. The chain height update is correctly placed within the wallet lock scope, ensuring atomic processing of blocks and matured transactions.key-wallet-manager/src/wallet_manager/process_block.rs (1)
216-221: LGTM!Clean delegation pattern that propagates the chain height update to all managed wallet infos, triggering matured transaction processing for each.
key-wallet/src/wallet/managed_wallet_info/wallet_info_interface.rs (3)
277-287: LGTM - Core fix for the issue.This correctly adds UTXOs when processing matured coinbase transactions. Previously, the transaction record was inserted but UTXOs were not created, leaving the spendable balance at zero even after maturity.
305-315: Consistent UTXO ingestion across account types.The BIP32 and CoinJoin branches correctly mirror the BIP44 implementation, ensuring UTXOs are added for all spendable account types when transactions mature.
Also applies to: 332-342
394-406: LGTM!The
update_chain_heightimplementation correctly delegates toprocess_matured_transactionsand logs the count of processed transactions.dash-spv/src/client/block_processor_test.rs (2)
88-89: LGTM!No-op implementation is appropriate for MockWallet since these tests focus on block processing mechanics, not coinbase maturation.
304-305: LGTM!Consistent no-op implementation for NonMatchingWallet to satisfy the updated WalletInterface trait.
key-wallet/src/transaction_checking/wallet_checker.rs (1)
208-214: LGTM - Clean refactor consolidating UTXO ingestion.Replacing inline UTXO construction with
add_utxos_from_transactionreduces code duplication and ensures consistent behavior between regular transaction processing and matured coinbase processing.
xdustinface
force-pushed
the
fix/immature-processing
branch
from
aac7324 to
2b83560
Compare
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. |
Check the individual commits, basically:
update_chain_heightgets called for the wallet for each block. This is to trigger the immature transaction re-processing which currently isnt triggered.Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.