feat: update gRPC to now use kernel_id as a primary tx id#7140
Closed
fluffypony wants to merge 2 commits intotari-project:developmentfrom
Closed
feat: update gRPC to now use kernel_id as a primary tx id#7140fluffypony wants to merge 2 commits intotari-project:developmentfrom
fluffypony wants to merge 2 commits intotari-project:developmentfrom
Conversation
Contributor
## Walkthrough
The changes standardize transaction identifier fields in wallet gRPC protocol buffers and server code from `transaction_id`/`tx_id` to `internal_dbid`, and introduce a new `kernel_id` field for transaction responses. Backward compatibility is maintained by adding a deprecated service and message types using the original field names.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------|
| applications/minotari_app_grpc/proto/wallet.proto | Renamed all transaction ID fields to `internal_dbid`, added `kernel_id` to transaction messages, adjusted field numbers, introduced `WalletDeprecated` service and deprecated message types for backward compatibility. |
| applications/minotari_app_grpc/src/conversions/transaction.rs | Added `not_found` constructor to `grpc::TransactionInfo`, updated for new identifier fields, annotated deprecated compatibility function. |
| applications/minotari_console_wallet/Cargo.toml | Added `"base_node"` feature to `tari_core` dependency. |
| applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs | Replaced `transaction_id` with `internal_dbid`, added `kernel_id` to transaction-related responses and conversions throughout the gRPC server implementation. |
| applications/minotari_console_wallet/src/ui/components/transactions_tab.rs | Added explicit type annotations for transaction list columns, introduced a new "Kernel ID" column in completed transactions list, updated detailed transaction view label and content to show kernel ID. |
| applications/minotari_console_wallet/src/ui/state/app_state.rs | Added async method to fetch kernel ID by transaction ID, replaced `excess_signature` field with `kernel_id` in `CompletedTransactionInfo`. |
| applications/minotari_console_wallet/src/ui/state/wallet_event_monitor.rs | Replaced raw transaction ID in notifications with kernel ID or fallback to transaction ID via new async helper method. |
| base_layer/core/src/base_node/proto/wallet_rpc.proto | Added `GetBlocksRequest` and `GetBlocksResponse` messages and imported `block.proto`. |
| base_layer/core/src/base_node/rpc/mod.rs | Added new RPC method `get_blocks` to `BaseNodeWalletService` trait. |
| base_layer/core/src/base_node/rpc/service.rs | Implemented `get_blocks` RPC method fetching blocks by heights with validation and error handling. |
| base_layer/wallet/src/transaction_service/service.rs | Added async diagnostic method to log kernel signatures of all completed transactions, invoked at service start. |
| base_layer/wallet/src/transaction_service/tasks/check_faux_transaction_status.rs | Extended transaction status check to update missing kernel signatures by fetching blocks from base node and updating transactions accordingly. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Client
participant WalletGrpcServer
participant WalletDB
Client->>WalletGrpcServer: Send transaction-related request
WalletGrpcServer->>WalletDB: Query or update transaction
WalletDB-->>WalletGrpcServer: Return transaction with internal_dbid, kernel_id
WalletGrpcServer-->>Client: Respond with internal_dbid, kernel_id in response messagePossibly related PRs
Suggested reviewers
Poem
|
Test Results (CI)0 tests 0 ✅ 0s ⏱️ Results for commit 40b3be2. ♻️ This comment has been updated with latest results. |
![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: 3
🧹 Nitpick comments (1)
base_layer/wallet/src/transaction_service/service.rs (1)
3896-3940: Consider performance optimization and logging level adjustment.The diagnostic method is well-implemented but has some areas for improvement:
Performance concern: Retrieving and processing ALL completed transactions could be expensive for wallets with many transactions. Consider adding pagination or a transaction limit for the diagnostic.
Logging verbosity: Using
INFOlevel for potentially hundreds/thousands of transactions might be too verbose for production logs. Consider usingDEBUGlevel or adding a configuration flag to control this diagnostic.Method signature: The method is marked
asyncbut doesn't perform any actual async operations -get_completed_transactions()appears to be synchronous.Consider these improvements:
- async fn log_all_transaction_signatures(&self) { + fn log_all_transaction_signatures(&self) { info!(target: LOG_TARGET, "Starting diagnostic dump of all transaction kernel signatures"); - // Get all completed transactions - let all_transactions = match self.db.get_completed_transactions(None, None, None) { + // Get completed transactions with a reasonable limit for diagnostics + let all_transactions = match self.db.get_completed_transactions(None, None, Some(1000)) { Ok(txs) => txs, Err(e) => { error!(target: LOG_TARGET, "Failed to retrieve completed transactions for diagnostic: {}", e); return; } }; - info!(target: LOG_TARGET, "Diagnostic: Found {} total completed transactions", all_transactions.len()); + debug!(target: LOG_TARGET, "Diagnostic: Found {} completed transactions", all_transactions.len()); for tx in all_transactions { if let Some(kernel_sig) = tx.transaction.first_kernel_excess_sig() { // ... existing logic ... - info!( + debug!( target: LOG_TARGET, "Transaction {}: sig_hex={}, nonce_hex={}, is_empty_sig={}, is_empty_nonce={}", tx.tx_id, sig_hex, nonce_hex, is_empty_sig, is_empty_nonce ); } else { - warn!( + debug!( target: LOG_TARGET, "Transaction {} has no kernel signatures", tx.tx_id ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
applications/minotari_console_wallet/src/ui/components/transactions_tab.rs(7 hunks)applications/minotari_console_wallet/src/ui/state/app_state.rs(4 hunks)applications/minotari_console_wallet/src/ui/state/wallet_event_monitor.rs(6 hunks)base_layer/core/src/base_node/proto/wallet_rpc.proto(2 hunks)base_layer/core/src/base_node/rpc/mod.rs(2 hunks)base_layer/core/src/base_node/rpc/service.rs(2 hunks)base_layer/wallet/src/transaction_service/service.rs(5 hunks)base_layer/wallet/src/transaction_service/tasks/check_faux_transaction_status.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- base_layer/core/src/base_node/proto/wallet_rpc.proto
🧰 Additional context used
🧬 Code Graph Analysis (1)
base_layer/core/src/base_node/rpc/mod.rs (1)
base_layer/core/src/base_node/rpc/service.rs (1)
get_blocks(713-752)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: ci
- GitHub Check: cargo check with stable
🔇 Additional comments (19)
base_layer/core/src/base_node/rpc/mod.rs (2)
32-33: LGTM! Imports are correctly added.The new protobuf imports for
GetBlocksRequestandGetBlocksResponseare properly added and align with the new RPC method being introduced.
137-141: LGTM! RPC method declaration follows existing patterns.The new
get_blocksmethod is correctly declared with:
- Sequential RPC method number (14)
- Proper async signature
- Consistent parameter and return types following the existing pattern
applications/minotari_console_wallet/src/ui/state/wallet_event_monitor.rs (5)
107-110: LGTM! Consistent implementation of kernel ID display.The change to use
get_transaction_identifierfor the "Finalized Transaction Received" notification aligns with the PR objectives to unify transaction identification using kernel IDs.
118-125: LGTM! Proper transaction identifier usage.The transaction mined unconfirmed notification correctly uses the new identifier format, maintaining consistency across all notification types.
133-134: LGTM! Transaction confirmed notification updated correctly.The notification message properly displays the unified transaction identifier format.
144-145: LGTM! All notification messages consistently updated.All transaction-related notifications now use the new
get_transaction_identifiermethod, ensuring consistent display of kernel IDs across the UI.Also applies to: 150-152, 158-161, 180-182
408-415: Well-implemented async helper method.The
get_transaction_identifiermethod correctly:
- Handles async operations with proper await
- Provides fallback to transaction ID when kernel ID is unavailable
- Formats the output consistently with clear labeling
base_layer/core/src/base_node/rpc/service.rs (2)
26-27: LGTM! Imports correctly added.The protobuf imports for the new get_blocks functionality are properly included.
713-752: Well-implemented RPC method with proper safeguards.The
get_blocksimplementation demonstrates good practices:Strengths:
- Proper input validation (empty heights check)
- Rate limiting with reasonable maximum (100 blocks)
- Graceful error handling that logs issues but doesn't fail the entire request
- Efficient pre-allocation with
Vec::with_capacity- Proper async/await usage
Security & Performance:
- The MAX_BLOCKS_PER_REQUEST limit prevents resource exhaustion attacks
- Individual block fetch failures are logged but don't crash the service
- Database operations are properly awaited
The implementation aligns well with the existing codebase patterns and provides the block fetching capability needed for kernel signature updates mentioned in the PR objectives.
applications/minotari_console_wallet/src/ui/components/transactions_tab.rs (5)
104-107: Good code clarity improvement.Adding explicit type annotations (
Vec<ListItem>) to the column vectors improves code readability and makes the intent clearer, especially helpful for maintenance.
208-212: LGTM! Consistent type annotations and new column preparation.The explicit type annotations are consistently applied, and the new
column4_itemsvector is properly declared for the kernel ID column.
307-319: Well-implemented kernel ID display with good UX.The kernel ID column implementation demonstrates good practices:
- Proper handling of empty kernel IDs with "N/A" fallback
- Sensible truncation (12 characters + "...") for table display
- Consistent styling with existing columns
- Clear visual indication of truncated content
326-326: Appropriate column width adjustments.The changes properly accommodate the new kernel ID column:
- Source/Destination Address column reduced from 95 to 75 (reasonable compromise)
- Status column gets fixed width of 15 (good for consistency)
- Kernel ID column uses remaining space (appropriate for variable-length IDs)
Also applies to: 329-330
354-354: Consistent UI label updates for kernel ID.The detailed transaction view correctly transitions from "Excess sig" to "Kernel ID":
- Label consistently updated across the UI
- Full kernel ID displayed in detail view (not truncated)
- Proper fallback to "N/A" with appropriate styling
- Maintains consistent visual hierarchy
Also applies to: 369-369, 401-405, 537-537
base_layer/wallet/src/transaction_service/service.rs (3)
88-88: LGTM: Import addition for hex conversion support.The
Hextrait import is appropriately added to support hexadecimal conversion in the new diagnostic logging method.
400-402: LGTM: Appropriate placement for diagnostic startup logging.The diagnostic function call is well-placed at the start of the main event loop, ensuring kernel signature information is logged once during service initialization.
1126-1126: LGTM: Necessary clone for parameter passing.The
.clone()addition appears to be required for passing the connectivity parameter to thecheck_detected_transactionsfunction.applications/minotari_console_wallet/src/ui/state/app_state.rs (1)
1221-1224: LGTM!The kernel ID computation correctly extracts the hex-encoded signature from the first kernel, with proper fallback to an empty string when no kernels exist.
base_layer/wallet/src/transaction_service/tasks/check_faux_transaction_status.rs (1)
305-320: LGTM!Good error handling for the RPC call with appropriate logging and descriptive error messages.
Comment on lines
+1175
to
+1189
| pub async fn get_transaction_kernel_id(&self, tx_id: TxId) -> Option<String> { | ||
| let mut tx_service = self.wallet.transaction_service.clone(); | ||
| if let Ok(Some(tx)) = tx_service.get_any_transaction(tx_id).await { | ||
| let completed_tx = tx.into(); | ||
| if let Ok(tx_info) = CompletedTransactionInfo::from_completed_transaction( | ||
| completed_tx, | ||
| &self.get_transaction_weight() | ||
| ) { | ||
| if !tx_info.kernel_id.is_empty() { | ||
| return Some(tx_info.kernel_id); | ||
| } | ||
| } | ||
| } | ||
| None | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving error handling and performance
The method silently returns None for all error cases without logging. This makes debugging difficult when kernel IDs are missing.
pub async fn get_transaction_kernel_id(&self, tx_id: TxId) -> Option<String> {
- let mut tx_service = self.wallet.transaction_service.clone();
+ let tx_service = &self.wallet.transaction_service;
if let Ok(Some(tx)) = tx_service.get_any_transaction(tx_id).await {
let completed_tx = tx.into();
if let Ok(tx_info) = CompletedTransactionInfo::from_completed_transaction(
completed_tx,
&self.get_transaction_weight()
) {
if !tx_info.kernel_id.is_empty() {
return Some(tx_info.kernel_id);
}
+ } else {
+ debug!(target: LOG_TARGET, "Failed to create CompletedTransactionInfo for tx_id: {}", tx_id);
}
+ } else {
+ trace!(target: LOG_TARGET, "Transaction not found or error fetching tx_id: {}", tx_id);
}
None
}Also, consider caching kernel IDs to avoid repeated conversions for the same transaction.
📝 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
| pub async fn get_transaction_kernel_id(&self, tx_id: TxId) -> Option<String> { | |
| let mut tx_service = self.wallet.transaction_service.clone(); | |
| if let Ok(Some(tx)) = tx_service.get_any_transaction(tx_id).await { | |
| let completed_tx = tx.into(); | |
| if let Ok(tx_info) = CompletedTransactionInfo::from_completed_transaction( | |
| completed_tx, | |
| &self.get_transaction_weight() | |
| ) { | |
| if !tx_info.kernel_id.is_empty() { | |
| return Some(tx_info.kernel_id); | |
| } | |
| } | |
| } | |
| None | |
| } | |
| pub async fn get_transaction_kernel_id(&self, tx_id: TxId) -> Option<String> { | |
| let tx_service = &self.wallet.transaction_service; | |
| if let Ok(Some(tx)) = tx_service.get_any_transaction(tx_id).await { | |
| let completed_tx = tx.into(); | |
| if let Ok(tx_info) = CompletedTransactionInfo::from_completed_transaction( | |
| completed_tx, | |
| &self.get_transaction_weight() | |
| ) { | |
| if !tx_info.kernel_id.is_empty() { | |
| return Some(tx_info.kernel_id); | |
| } | |
| } else { | |
| debug!( | |
| target: LOG_TARGET, | |
| "Failed to create CompletedTransactionInfo for tx_id: {}", | |
| tx_id | |
| ); | |
| } | |
| } else { | |
| trace!( | |
| target: LOG_TARGET, | |
| "Transaction not found or error fetching tx_id: {}", | |
| tx_id | |
| ); | |
| } | |
| None | |
| } |
🤖 Prompt for AI Agents
In applications/minotari_console_wallet/src/ui/state/app_state.rs around lines
1175 to 1189, the get_transaction_kernel_id method currently suppresses all
errors by returning None without any logging, which hinders debugging. Modify
the method to log errors or unexpected conditions when fetching or processing
the transaction fails. Additionally, implement a caching mechanism to store and
retrieve kernel IDs for transactions to improve performance by avoiding repeated
conversions for the same transaction.
Comment on lines
+265
to
+468
| async fn fetch_and_update_kernel_signatures<TBackend: 'static + TransactionBackend, TWalletConnectivity: WalletConnectivityInterface>( | ||
| mut connectivity: TWalletConnectivity, | ||
| db: TransactionDatabase<TBackend>, | ||
| transactions: Vec<CompletedTransaction>, | ||
| ) -> Result<(), TransactionServiceError> { | ||
| let mut base_node_client = match connectivity.obtain_base_node_wallet_rpc_client().await { | ||
| Some(client) => client, | ||
| None => { | ||
| return Err(TransactionServiceError::ServiceError( | ||
| "Could not connect to base node wallet RPC client".to_string(), | ||
| )); | ||
| }, | ||
| }; | ||
|
|
||
| // Collect unique block heights for transactions that have mined_in_block | ||
| let mut height_to_txs: std::collections::HashMap<u64, Vec<&CompletedTransaction>> = std::collections::HashMap::new(); | ||
|
|
||
| for tx in &transactions { | ||
| if let (Some(_), Some(height)) = (&tx.mined_in_block, tx.mined_height) { | ||
| height_to_txs.entry(height).or_insert_with(Vec::new).push(tx); | ||
| } | ||
| } | ||
|
|
||
| if height_to_txs.is_empty() { | ||
| info!( | ||
| target: LOG_TARGET, | ||
| "No transactions with mined height found for kernel signature migration" | ||
| ); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let heights: Vec<u64> = height_to_txs.keys().cloned().collect(); | ||
| debug!( | ||
| target: LOG_TARGET, | ||
| "Fetching {} blocks for kernel signature migration: {:?}", | ||
| heights.len(), | ||
| heights | ||
| ); | ||
|
|
||
| // Fetch blocks using GetBlocks RPC | ||
| let request = base_node_proto::GetBlocksRequest { heights }; | ||
|
|
||
| let response = match base_node_client.get_blocks(request).await { | ||
| Ok(response) => response, | ||
| Err(e) => { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "Failed to fetch blocks for kernel signature migration: {}", | ||
| e | ||
| ); | ||
| return Err(TransactionServiceError::ServiceError(format!( | ||
| "Failed to fetch blocks: {}", | ||
| e | ||
| ))); | ||
| } | ||
| }; | ||
|
|
||
| let mut updated_count = 0; | ||
|
|
||
| // Process each block in the response | ||
| for historical_block in response.blocks { | ||
| let block_height = historical_block.block.as_ref() | ||
| .and_then(|b| b.header.as_ref()) | ||
| .map(|h| h.height) | ||
| .unwrap_or(0); | ||
|
|
||
| // Get transactions for this block height | ||
| let txs_for_height = match height_to_txs.get(&block_height) { | ||
| Some(txs) => txs, | ||
| None => continue, | ||
| }; | ||
|
|
||
| let empty_kernels = Vec::new(); | ||
| let block_kernels = historical_block.block.as_ref() | ||
| .and_then(|b| b.body.as_ref()) | ||
| .map(|b| &b.kernels) | ||
| .unwrap_or(&empty_kernels); | ||
|
|
||
| debug!( | ||
| target: LOG_TARGET, | ||
| "Processing block at height {} with {} kernels for {} transactions", | ||
| block_height, | ||
| block_kernels.len(), | ||
| txs_for_height.len() | ||
| ); | ||
|
|
||
| // Process each transaction for this block | ||
| for tx in txs_for_height { | ||
| let mut transaction_updated = false; | ||
| let mut updated_kernels = Vec::new(); | ||
|
|
||
| for (kernel_index, wallet_kernel) in tx.transaction.body.kernels().iter().enumerate() { | ||
| let wallet_excess = &wallet_kernel.excess; | ||
|
|
||
| // Find matching kernel in block by excess commitment | ||
| let matching_block_kernel = block_kernels.iter().find(|block_kernel| { | ||
| block_kernel.excess.as_ref().map_or(false, |e| e.data.as_slice() == wallet_excess.as_bytes()) | ||
| }); | ||
|
|
||
| if let Some(block_kernel) = matching_block_kernel { | ||
| // Compare signatures | ||
| let wallet_sig = &wallet_kernel.excess_sig; | ||
| let block_sig = block_kernel.excess_sig.as_ref(); | ||
|
|
||
| if let Some(block_sig) = block_sig { | ||
| // Convert proto signature to internal format | ||
| if let Ok(block_signature) = Signature::try_from((*block_sig).clone()) { | ||
| if wallet_sig != &block_signature { | ||
| info!( | ||
| target: LOG_TARGET, | ||
| "Found matching kernel for transaction {} kernel {}: updating signature", | ||
| tx.tx_id, | ||
| kernel_index | ||
| ); | ||
|
|
||
| // Create updated kernel with correct signature | ||
| let mut updated_kernel = wallet_kernel.clone(); | ||
| updated_kernel.excess_sig = block_signature; | ||
| updated_kernels.push(updated_kernel); | ||
| transaction_updated = true; | ||
| } else { | ||
| // Kernel signature is already correct | ||
| updated_kernels.push(wallet_kernel.clone()); | ||
| } | ||
| } else { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "Failed to convert block kernel signature for transaction {} kernel {}", | ||
| tx.tx_id, | ||
| kernel_index | ||
| ); | ||
| updated_kernels.push(wallet_kernel.clone()); | ||
| } | ||
| } else { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "Block kernel has no signature for transaction {} kernel {}", | ||
| tx.tx_id, | ||
| kernel_index | ||
| ); | ||
| updated_kernels.push(wallet_kernel.clone()); | ||
| } | ||
| } else { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "Could not find matching kernel in block at height {} for transaction {} kernel {}", | ||
| block_height, | ||
| tx.tx_id, | ||
| kernel_index | ||
| ); | ||
| // Keep the original kernel if no match found | ||
| updated_kernels.push(wallet_kernel.clone()); | ||
| } | ||
| } | ||
|
|
||
| // Update the transaction if any kernels were modified | ||
| if transaction_updated { | ||
| // Create a new transaction with updated kernels | ||
| let updated_transaction = Transaction::new( | ||
| tx.transaction.body.inputs().clone(), | ||
| tx.transaction.body.outputs().clone(), | ||
| updated_kernels, | ||
| tx.transaction.offset.clone(), | ||
| tx.transaction.script_offset.clone(), | ||
| ); | ||
|
|
||
| let mut updated_tx = (*tx).clone(); | ||
| updated_tx.transaction = updated_transaction; | ||
|
|
||
| // Save the updated transaction to the database | ||
| if let Err(e) = db.update_completed_transaction(tx.tx_id, updated_tx.clone()) { | ||
| warn!( | ||
| target: LOG_TARGET, | ||
| "Failed to update transaction {} with correct kernel signatures: {}", | ||
| tx.tx_id, | ||
| e | ||
| ); | ||
| } else { | ||
| updated_count += 1; | ||
| info!( | ||
| target: LOG_TARGET, | ||
| "Successfully updated transaction {} with correct kernel signatures", | ||
| tx.tx_id | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if updated_count > 0 { | ||
| info!( | ||
| target: LOG_TARGET, | ||
| "Kernel signature migration completed: {} transactions updated with correct signatures", | ||
| updated_count | ||
| ); | ||
| } else { | ||
| info!( | ||
| target: LOG_TARGET, | ||
| "Kernel signature migration completed: no transactions needed updating" | ||
| ); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor complex function and improve kernel matching logic
This function is doing too much and has potential issues:
- At 200+ lines, it violates single responsibility principle
- The kernel matching assumes order preservation, which may not always be true
- No optimization for transactions from the same block
Consider breaking this down into smaller functions:
+/// Match a wallet kernel with a block kernel by excess commitment
+fn find_matching_block_kernel<'a>(
+ wallet_kernel: &TransactionKernel,
+ block_kernels: &'a [base_node_proto::TransactionKernel],
+) -> Option<&'a base_node_proto::TransactionKernel> {
+ block_kernels.iter().find(|block_kernel| {
+ block_kernel.excess.as_ref()
+ .map_or(false, |e| e.data.as_slice() == wallet_kernel.excess.as_bytes())
+ })
+}
+
+/// Update kernel signatures for a single transaction
+async fn update_transaction_kernel_signatures<TBackend: TransactionBackend>(
+ db: &TransactionDatabase<TBackend>,
+ tx: &CompletedTransaction,
+ block_kernels: &[base_node_proto::TransactionKernel],
+) -> Result<bool, TransactionServiceError> {
+ // Extract kernel update logic here
+ // Return true if transaction was updated
+}
async fn fetch_and_update_kernel_signatures<TBackend: 'static + TransactionBackend, TWalletConnectivity: WalletConnectivityInterface>(
mut connectivity: TWalletConnectivity,
db: TransactionDatabase<TBackend>,
transactions: Vec<CompletedTransaction>,
) -> Result<(), TransactionServiceError> {
// Simplified main function using extracted helpersAlso, add validation to ensure all kernels are matched before updating:
if updated_kernels.len() != tx.transaction.body.kernels().len() {
warn!(
target: LOG_TARGET,
"Kernel count mismatch for transaction {}: expected {}, got {}",
tx.tx_id,
tx.transaction.body.kernels().len(),
updated_kernels.len()
);
continue;
}🤖 Prompt for AI Agents
In
base_layer/wallet/src/transaction_service/tasks/check_faux_transaction_status.rs
between lines 265 and 468, the fetch_and_update_kernel_signatures function is
too large and mixes multiple responsibilities, making it hard to maintain and
test. Refactor by extracting smaller helper functions for tasks like fetching
blocks, grouping transactions by height, matching kernels, and updating
transactions. Improve kernel matching logic to avoid assuming order preservation
by matching kernels based on unique identifiers or commitments. Add validation
before updating a transaction to ensure the number of updated kernels matches
the original kernel count, logging a warning and skipping the update if they
differ, as shown in the suggested code snippet.
Comment on lines
+116
to
+130
| let mut all_completed = match db.get_completed_transactions(None, None, None) { | ||
| Ok(txs) => txs, | ||
| Err(e) => { | ||
| error!( | ||
| target: LOG_TARGET, | ||
| "Problem retrieving completed transactions for kernel signature check: {}", e | ||
| ); | ||
| return; | ||
| }, | ||
| }; | ||
| all_detected_transactions.append(&mut all_completed); | ||
|
|
||
| // Remove duplicates based on tx_id | ||
| all_detected_transactions.sort_by(|a, b| a.tx_id.as_u64().cmp(&b.tx_id.as_u64())); | ||
| all_detected_transactions.dedup_by(|a, b| a.tx_id == b.tx_id); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Performance concern: Fetching all completed transactions on every check
Fetching ALL completed transactions on every call to check for missing kernel signatures is inefficient and will degrade performance as the wallet accumulates more transactions over time.
Consider these alternatives:
- Only check transactions that are known to have missing kernel signatures (maintain a flag in the database)
- Run this migration as a one-time background task rather than on every status check
- Add pagination or date filters to limit the scope
- // Also include ALL completed transactions to check for empty kernel signatures
- let mut all_completed = match db.get_completed_transactions(None, None, None) {
+ // Only fetch confirmed transactions that may have missing kernel signatures
+ // This could be tracked with a database flag or by checking a specific date range
+ let mut transactions_needing_migration = match db.get_transactions_with_missing_kernel_signatures() {
Ok(txs) => txs,
Err(e) => {
error!(
target: LOG_TARGET,
"Problem retrieving completed transactions for kernel signature check: {}", e
);
return;
},
};
- all_detected_transactions.append(&mut all_completed);
+ all_detected_transactions.append(&mut transactions_needing_migration);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
base_layer/wallet/src/transaction_service/tasks/check_faux_transaction_status.rs
around lines 116 to 130, the code fetches all completed transactions every time,
which is inefficient. To fix this, modify the database query to only retrieve
transactions flagged as missing kernel signatures or apply filters such as date
ranges or pagination to limit the number of transactions fetched. Alternatively,
refactor this logic to run as a one-time background migration task instead of
during every status check.
Contributor
Author
|
Deprecated by PayRef |
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.
Description
Implement comprehensive gRPC API unification by transitioning from internal database transaction IDs to human-readable kernel IDs across the entire wallet ecosystem. This PR unifies transaction identification across gRPC API, console wallet UI, and FFI layer, replacing internal database IDs with blockchain-native kernel IDs (hex-encoded excess signatures) while maintaining full backwards compatibility.
Motivation and Context
This PR addresses the fragmented transaction identification system across Tari's wallet ecosystem. Previously, different components used inconsistent transaction identifiers:
transaction_idandtx_idfieldsThe unified approach provides:
How Has This Been Tested?
internal_dbidandkernel_idfieldsWhat process can a PR reviewer use to test or verify this change?
gRPC API Testing:
Console Wallet UI Testing:
cargo run --bin minotari_console_wallet12345678...87654321)FFI Layer Testing:
Integration Testing:
kernel_idfield)Backwards Compatibility:
Breaking Changes
Exchanges and third-party integrations relying on
transaction_idmust now use theWalletDeprecatedservice instead ofWallet.Summary by CodeRabbit
New Features
kernel_id, to transaction-related responses for enhanced transaction identification.Refactor
transaction_id/tx_idtointernal_dbidacross all wallet gRPC responses and requests.Chores