fix: remove long timeout in interactive_tx till tx is persisted into db#7252
Conversation
|
""" WalkthroughThe changes update transaction persistence timing and timeout handling in the wallet. Transactions are now stored immediately upon initiation rather than after a broadcast attempt. The timeout for waiting on transaction persistence in the gRPC server is reduced from 30 seconds to 100 milliseconds, and related logic is refactored accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WalletGrpcServer
participant TransactionService
User->>WalletGrpcServer: Initiate transfer
WalletGrpcServer->>TransactionService: Add pending outbound transaction
WalletGrpcServer->>TransactionService: Check transaction size
alt Oversized
TransactionService-->>WalletGrpcServer: Cancel transaction
WalletGrpcServer-->>User: Return error
else Valid size
WalletGrpcServer->>TransactionService: Attempt broadcast
WalletGrpcServer->>TransactionService: Query transaction by ID (loop, up to 100ms)
alt Transaction found
WalletGrpcServer-->>User: Confirm transfer
else Timeout
WalletGrpcServer-->>User: Return timeout error
end
end
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
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
🔭 Outside diff range comments (1)
base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs (1)
341-356: Remove redundant transaction size checkThis size check is unreachable because the transaction size is already validated at lines 335-338. If the transaction is oversized,
cancel_oversized_transaction()is called which returns early.Apply this diff to remove the dead code:
- if let Err(e) = check_transaction_size(&outbound_tx, self.id) { - info!( - target: LOG_TARGET, - "Initial Transaction TxId: {:?} will not be sent due to it being oversize ({:?})", self.id, e - ); - } else { - match self.send_transaction(msg).await { - Ok(val) => initial_send = val, - Err(e) => { - warn!( - target: LOG_TARGET, - "Problem sending Outbound Transaction TxId: {:?}: {:?}", self.id, e - ); - }, - } - }; + match self.send_transaction(msg).await { + Ok(val) => initial_send = val, + Err(e) => { + warn!( + target: LOG_TARGET, + "Problem sending Outbound Transaction TxId: {:?}: {:?}", self.id, e + ); + }, + }
🧹 Nitpick comments (1)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)
27-27: Unnecessary import changeThe change from
tokio::time::Durationtostd::time::Durationis not required for this fix. Thetokio::time::Durationre-exportsstd::time::Duration, so they are functionally equivalent in this context.Consider reverting this import change to minimize the diff:
- time::Duration, + time::Duration,Or keep using tokio's Duration:
-use std::{ +use tokio::time::Duration; +use std::{ convert::{TryFrom, TryInto}, str::FromStr, sync::Arc, - time::Duration, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(3 hunks)base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (1)
base_layer/wallet/src/transaction_service/protocols/transaction_send_protocol.rs (1)
319-339: Good improvement to transaction persistence timing!Moving the transaction persistence earlier in the flow ensures that transactions are saved even if subsequent send operations fail. This addresses the issue where transactions could be lost if the process was interrupted after preparation but before persistence.
Test Results (CI) 3 files 126 suites 42m 48s ⏱️ Results for commit fdf1a21. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests) 2 files 10 suites 1h 2m 44s ⏱️ For more details on these failures, see this check. Results for commit 7a59048. |
There was a problem hiding this comment.
ACK
This provides a better transaction flow.
I also tested this by removing the wait loop in the grpc server method altogether, like it was before the original PR #7234 changed it, but then it still breaks in cucumber test Scenario: Wallet imports pre_mine UTXO although not on the first but at the 2nd interactive transaction not persisted, which means the changed transaction flow improves matters a bit but does not fix it. We still need the wait loop to ensure the transaction is persisted.
hansieodendaal
left a comment
hansieodendaal
left a comment
There was a problem hiding this comment.
ACK
The additional cucumber test Scenario: Wallet sends many interactive transactions to another wallet proves that the grpc timeout is adequate for the wallet to handle many interactive transactions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
integration_tests/tests/steps/wallet_steps.rs (1)
1086-1216: Consider refactoring to reduce code duplication.This function shares significant logic with
send_interactive_amount_from_wallet_to_wallet_at_fee. Consider extracting common functionality into helper functions to improve maintainability.Potential refactoring approach:
- Extract transaction creation logic into a helper function
- Extract broadcast waiting logic into a reusable function
- Create a unified function that handles both single and multiple transaction scenarios
This would reduce the current ~130 lines of duplicated code and make the codebase more maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration_tests/tests/features/WalletTransactions.feature(1 hunks)integration_tests/tests/steps/wallet_steps.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: Cucumber tests / FFI
🔇 Additional comments (2)
integration_tests/tests/features/WalletTransactions.feature (1)
183-195: Well-structured test scenario for validating interactive transaction performance improvements.This new test scenario effectively validates the PR's objective of fixing timeout issues in interactive transactions by testing 40 sequential transactions. The scenario structure follows established patterns, and the balance calculations are appropriate (40M uT + fees vs 10B uT available balance).
Please verify that the new test step
send 40 interactive transactions of 1000000 uT from wallet WALLET_A to wallet WALLET_B at fee 100has been properly implemented in the corresponding step definitions.#!/bin/bash # Description: Verify the implementation of the new test step for sending multiple interactive transactions # Search for the step definition implementation rg -A 10 "send.*interactive transactions.*from wallet.*to wallet.*at fee" --type rustintegration_tests/tests/steps/wallet_steps.rs (1)
1161-1162: Timeout configuration aligns with PR objectives.The reduced polling interval (100ms) and total timeout (30 seconds with 300 retries) effectively addresses the PR objective of removing long timeouts in interactive transactions until they're persisted.
* development: fix: remove long timeout in interactive_tx till tx is persisted into db (tari-project#7252) chore: new release v4.6.1-pre.0 (tari-project#7248) fix: minotari_merge_mining_proxy returns Tari block hash even if submit_to_origin is disabled (tari-project#7242) chore(ci): windows binary audit fix and more code-signing verification (tari-project#7245) fix: database cannot resize on jmt write (tari-project#7244)
2 participants
Description
fixes: #7251
Summary by CodeRabbit