fix: only revalidated rejected transactions on startup#7209
Conversation
WalkthroughThis change introduces a new capability to revalidate only rejected transactions in the wallet's transaction service, rather than all transactions. It adds new API methods, request variants, and database logic to support marking rejected transactions as unvalidated and triggering their revalidation, updating both the service and storage layers accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ConsoleWallet
participant TransactionServiceHandle
participant TransactionService
participant TransactionDatabase
User->>ConsoleWallet: Start Wallet
ConsoleWallet->>TransactionServiceHandle: revalidate_rejected_transactions()
TransactionServiceHandle->>TransactionService: ReValidateRejectedTransactions request
TransactionService->>TransactionDatabase: mark_all_rejected_transactions_as_unvalidated()
TransactionDatabase-->>TransactionService: Result
TransactionService->>TransactionService: start_transaction_validation_protocol()
TransactionService-->>TransactionServiceHandle: ValidationStarted
TransactionServiceHandle-->>ConsoleWallet: Ok / Error
Suggested reviewers
Poem
✨ 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 (4)
applications/minotari_console_wallet/src/init/mod.rs (1)
618-620: Update log message to reflect new behaviourLogging still says "revalidating all transactions", but the code now revalidates only rejected transactions.
Keeping the message in sync avoids misleading operators during troubleshooting.- debug!("revalidating all transactions"); + debug!("revalidating rejected transactions");base_layer/wallet/src/transaction_service/handle.rs (2)
399-400: Minor: keepDisplayarms alphabetically groupedFor long
matchchains the project typically keeps the arms alphabetically (or thematically) grouped.
Consider movingReValidateRejectedTransactionsnext toReValidateTransactionsfor quicker scanning.
1163-1172: Factor out duplicated validation helper
revalidate_rejected_transactionsrepeats the same three-line pattern thatrevalidate_all_transactionsalready implements.
A tiny helper would avoid the duplication:+async fn send_validation_request( + handle: &mut Self, + req: TransactionServiceRequest, +) -> Result<(), TransactionServiceError> { + match handle.handle.call(req).await?? { + TransactionServiceResponse::ValidationStarted(_) => Ok(()), + _ => Err(TransactionServiceError::UnexpectedApiResponse), + } +} pub async fn revalidate_all_transactions(&mut self) -> Result<(), TransactionServiceError> { - match self - .handle - .call(TransactionServiceRequest::ReValidateTransactions) - .await?? - { - TransactionServiceResponse::ValidationStarted(_) => Ok(()), - _ => Err(TransactionServiceError::UnexpectedApiResponse), - } + Self::send_validation_request(self, TransactionServiceRequest::ReValidateTransactions).await } pub async fn revalidate_rejected_transactions(&mut self) -> Result<(), TransactionServiceError> { - match self - .handle - .call(TransactionServiceRequest::ReValidateRejectedTransactions) - .await?? - { - TransactionServiceResponse::ValidationStarted(_) => Ok(()), - _ => Err(TransactionServiceError::UnexpectedApiResponse), - } + Self::send_validation_request(self, TransactionServiceRequest::ReValidateRejectedTransactions).await }Not critical, but trims repetition and eases future maintenance.
base_layer/wallet/src/transaction_service/service.rs (1)
1015-1018: New request variant wired in correctly – minor naming nitThe new
ReValidateRejectedTransactionsbranch cleanly mirrors the existingReValidateTransactionscase and re-uses the same response type, so behaviour looks sound.Small nit: consider pluralising the helper method (
start_rejected_transactions_revalidation) to align with the (plural) enum variant and to match the existingstart_transaction_revalidationnaming scheme for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
applications/minotari_console_wallet/src/init/mod.rs(1 hunks)base_layer/wallet/src/transaction_service/handle.rs(3 hunks)base_layer/wallet/src/transaction_service/service.rs(2 hunks)base_layer/wallet/src/transaction_service/storage/database.rs(2 hunks)base_layer/wallet/src/transaction_service/storage/sqlite_db.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/wallet/src/transaction_service/storage/database.rs (1)
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)
mark_all_rejected_transactions_as_unvalidated(976-999)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: ci
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: cargo check with stable
🔇 Additional comments (4)
base_layer/wallet/src/transaction_service/storage/database.rs (2)
144-145: Trait extension may break other back-ends – verify all implementations
mark_all_rejected_transactions_as_unvalidatedwas added toTransactionBackend.
Every struct implementing this trait must now implement the new method, otherwise the crate will fail to compile.
sqlite_db.rswas updated, butmemory_db.rs/ any other back-end were not shown in this diff.Please confirm all concrete back-ends compile with the new signature or add a default
unimplemented!()stub behind a feature-gate.
792-794: Thin wrapper LGTMThe database façade simply delegates to the backend; no additional logic introduced.
base_layer/wallet/src/transaction_service/storage/sqlite_db.rs (1)
973-999: Reuse existing helper, fix log label, preserve cancellation reasonThe added method works but:
- Code duplicated from
mark_all_non_coinbases_transactions_as_unvalidated. Consider a private helper that takes the status filter as an argument to avoid maintenance drift.- Trace label still says
set_transactions_to_be_revalidated. This now fires in two places and is misleading.- Clearing the
cancelledcolumn discards the original rejection reason. If that information is still valuable (e.g. telemetry or manual troubleshooting) keep it and rely solely on thestatuscolumn for the protocol to decide what to re-validate.Minimal fix for the confusing log text:
- target: LOG_TARGET, - "sqlite profile - set_transactions_to_be_revalidated: lock {} + db_op {} = {} ms", + target: LOG_TARGET, + "sqlite profile - revalidate_rejected_tx: lock {} + db_op {} = {} ms",A small helper would further DRY the two nearly-identical update blocks:
fn mark_transactions_as_unvalidated<F>( &self, filter: F, ) -> Result<(), TransactionStorageError> where F: FnOnce(completed_transactions::BoxedQuery<'_, _>) -> completed_transactions::BoxedQuery<'_, _>, { // shared body... }[ suggest_optional_refactor ]
base_layer/wallet/src/transaction_service/handle.rs (1)
207-208: Validate that every layer recognises the newReValidateRejectedTransactionsrequestAdding a new enum variant is straightforward here, but please double-check that:
- All switch / match statements in the service layer (
service.rs) and storage back-end are updated.- FFI, JSON-RPC, GRPC, or IPC endpoints that serialise
TransactionServiceRequestare regenerated if required.Failing to propagate the variant will surface as a runtime
UnexpectedApiResponseor silent no-op.
Test Results (CI) 3 files 126 suites 54m 34s ⏱️ For more details on these failures, see this check. Results for commit 710245a. |
Test Results (Integration tests) 2 files 1 errors 1 suites 24m 22s ⏱️ For more details on these parsing errors and failures, see this check. Results for commit 710245a. |
* development: (607 commits) Wallet GRPC port comment fix from 18142 to 18143 (tari-project#7221) feat: integrated address support for Ledger (tari-project#7198) chore: new release v4.1.1-pre.0 (tari-project#7211) fix: migration can now correctly resume after stopping (tari-project#7210) fix: only revalidated rejected transactions on startup (tari-project#7209) fix: add filtering flag back (tari-project#7208) feat: improve wallet balance checks from external clients (tari-project#7207) feat!: update grpc supply query (tari-project#7137) docs: Updated API GRPC and Exchange Guide (tari-project#7205) chore: new release v4.4.0-pre.0 (tari-project#7202) feat: update base node proto to search bytes (tari-project#7201) feat: full PayRef implementation (tari-project#7154) test: add ffi cucumber wallet balance test (tari-project#7189) chore: fix tests (tari-project#7196) fix(network-discovery): add back idle event handling (tari-project#7194) Update SECURITY.md (tari-project#7193) fix: transaction manager service unmined lookup (tari-project#7192) fix: wallet ffi database name mismatch for mobile wallet (tari-project#7191) fix: payment_id deserialize (tari-project#7187) fix: remove code for deleting stale peers (tari-project#7184) ...
1 participant
Description
We should only revalidate rejected transactions on startup, and not all transactions
Summary by CodeRabbit