fix: correctly validate coinbase transactions for recovered wallets#7278
Conversation
WalkthroughThe changes update transaction and UTXO scanning logic in the wallet. The transaction service now also resets coinbase transactions marked as not in the blockchain. UTXO scanning tracks the number of scanned blocks and triggers revalidation if a threshold is reached. A constant's visibility is increased, and a vector access style is adjusted. Changes
Sequence Diagram(s)sequenceDiagram
participant UTXO Scanner Task
participant Peer Node
participant Transaction Service
UTXO Scanner Task->>Peer Node: Begin UTXO scan loop
loop For each block
UTXO Scanner Task->>Peer Node: Scan block
UTXO Scanner Task->>UTXO Scanner Task: Increment scanned_blocks
end
alt scanned_blocks >= SAFETY_HEIGHT_MARGIN
UTXO Scanner Task->>Transaction Service: Revalidate rejected transactions (async)
end
UTXO Scanner Task-->>UTXO Scanner Task: Return results (outputs, height, blocks, amount, time)
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)error: failed to load source for dependency Caused by: Caused by: Caused by: 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
base_layer/wallet/src/transaction_service/tasks/check_faux_transaction_status.rs (1)
23-23: Avoid leaking task-level constant; promote it to a shared config module insteadMaking
SAFETY_HEIGHT_MARGINpubfixes visibility but couples external modules to this task’s implementation details. Consider relocating the constant to a wallet-levelconstants.rs(orTransactionServiceConfig) and re-exporting it there. This keeps module boundaries cleaner and avoids future circular-dependency pain. Also add a brief doc-comment describing why 3 000 blocks is the chosen safety margin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(1 hunks)base_layer/wallet/src/transaction_service/tasks/check_faux_transaction_status.rs(1 hunks)base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs(7 hunks)base_layer/wallet_ffi/src/lib.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
base_layer/wallet_ffi/src/lib.rs (2)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.
</retrieved_learning>
<retrieved_learning>
Learnt from: hansieodendaal
PR: #6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.
</retrieved_learning>
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)
undefined
<retrieved_learning>
Learnt from: hansieodendaal
PR: #7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the hard_delete_all_stale_peers method in comms/core/src/peer_manager/storage/database.rs, the SQL query intentionally uses exact equality (peers.features = ?) rather than bitwise operations (peers.features & ? != 0) when matching COMMUNICATION_NODE features. This is the intended behavior to match only peers with exactly the COMMUNICATION_NODE feature, excluding those with additional feature flags.
</retrieved_learning>
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (2)
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: cargo check with stable
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (7)
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)
989-999: LGTM! Excellent fix for coinbase transaction validation during wallet recovery.This addition correctly addresses the issue described in the PR objectives where coinbase transactions marked as
CoinbaseNotInBlockChainduring recovery need to be reset for proper revalidation. The implementation:
- Appropriately resets mining-related fields to allow fresh validation
- Changes status from
CoinbaseNotInBlockChaintoCoinbaseUnconfirmedfor reprocessing- Maintains consistency with the existing rejected transaction handling pattern
- Includes proper logging for monitoring
This ensures coinbase transactions can be correctly validated after wallet recovery operations.
base_layer/wallet/src/utxo_scanner_service/utxo_scanner_task.rs (6)
63-66: Import addition looks correct.The import of
SAFETY_HEIGHT_MARGINfrom the transaction service module is appropriate and aligns with the PR objectives to use this constant for determining when to trigger revalidation.
112-121: Revalidation logic implementation is sound.The conditional revalidation logic correctly addresses the PR objectives by:
- Checking if scanned blocks exceed the safety margin threshold
- Triggering revalidation of rejected transactions when threshold is reached
- Using fire-and-forget pattern with
let _result = ...which is appropriate for this non-critical operationThis should resolve the coinbase transaction validation issues during wallet recovery.
225-228: Function signature change is consistent.The updated return type correctly includes the
scanned_blocksparameter as the second element in the tuple, maintaining consistency with the calling code.
250-250: Proper initialization of scanned blocks counter.The
scanned_blocksvariable is correctly initialized to 0 at the beginning of the scanning process.
269-269: Return statements updated consistently.Both return statements correctly include the
scanned_blocksparameter in the appropriate position within the tuple, maintaining consistency with the function signature change.Also applies to: 320-320
344-344: Scanned blocks increment is correctly placed.The increment is properly positioned within the scanning loop where blocks are being processed, ensuring accurate tracking of the number of blocks scanned during the recovery process.
Test Results (CI) 3 files 126 suites 50m 31s ⏱️ Results for commit 15360da. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files + 2 1 errors 1 suites +1 15m 29s ⏱️ + 15m 29s For more details on these parsing errors and failures, see this check. Results for commit 15360da. ± Comparison against base commit 4bbc859. ♻️ This comment has been updated with latest results. |
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
In general, not specific to this PR, we may need to add an age limit to rejected/other transactions we re-validate, for example, ignore all transactions older than a week. A user action could then force re-validation of everything.
Some commens below.
3 participants
Description
This ensures that coinbases are correctly validated when doing recovery.
Current wallet, when recovery is done, will very likely mark coinbases are not mined, and never revalidate them.
To fix this we reset those statuses during startup,
and then we also rerun this after a large recovery.
What causes the bug is following events:
1: Wallet detects the coinbases, adds them to the db and marks that as not yet mined.
2: TMS runs validation, sees them as ummined,
3: OMS runs validation and sees them as mined, and updates them correctly
4: TMS runs validation again, but height has now changed beyond the scanned limet as in the name of "efficiency", only checks for a certain height, which the coinbases have long since expired, since the scanning the block height jumps a lot while recovering.
5: The TMS never rechecks them again to ensure its not incorrect.
Summary by CodeRabbit
New Features
Improvements
Other Changes