feat: offline signing#7122
Conversation
WalkthroughThis change implements a full offline signing workflow for one-sided transactions in the Tari wallet. It introduces new CLI commands, gRPC endpoints, protobuf messages, and transaction service logic to support preparing, signing, and broadcasting one-sided transactions offline. Supporting modules for key encryption, offline signing models, and error handling are added throughout the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant WalletGRPC
participant TransactionService
CLI->>WalletGRPC: PrepareOneSidedTransactionForSigning
WalletGRPC->>TransactionService: prepare_one_sided_transaction_for_signing
TransactionService-->>WalletGRPC: PrepareOneSidedTransactionForSigningResult
WalletGRPC-->>CLI: PrepareOneSidedTransactionForSigningResponse (lock details)
CLI->>TransactionService: sign_one_sided_transaction (with lock details)
TransactionService-->>CLI: SignedOneSidedTransactionResult
CLI->>WalletGRPC: BroadcastSignedOneSidedTransaction (with signed tx)
WalletGRPC->>TransactionService: broadcast_signed_one_sided_transaction
TransactionService-->>WalletGRPC: Transaction ID
WalletGRPC-->>CLI: BroadcastSignedOneSidedTransactionResponse
Possibly related issues
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 (
|
martinserts
force-pushed
the
offline_signing
branch
from
767ce90 to
b7aceb1
Compare
martinserts
force-pushed
the
offline_signing
branch
from
090f5a4 to
108649e
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: 8
🔭 Outside diff range comments (1)
base_layer/wallet/src/transaction_service/service.rs (1)
2255-2262: 🛠️ Refactor suggestionConsider using a centralized transaction direction resolver
There's repeated logic for determining transaction direction based on
TxType. Consider extracting this to a single method to ensure consistency and reduce duplication.+fn get_transaction_direction_for_type(tx_type: &TxType) -> TransactionDirection { + match tx_type { + TxType::PaymentToOther | TxType::Burn => TransactionDirection::Outbound, + TxType::PaymentToSelf + | TxType::CoinSplit + | TxType::CoinJoin + | TxType::ValidatorNodeRegistration + | TxType::CodeTemplateRegistration + | TxType::ClaimAtomicSwap + | TxType::HtlcAtomicSwapRefund + | TxType::ImportedUtxoNoneRewindable + | TxType::Coinbase => TransactionDirection::Inbound, + } +}Also applies to: 2755-2763, 2769-2777, 3255-3262
🧹 Nitpick comments (12)
base_layer/wallet/src/transaction_service/service.rs (5)
466-468: Error handling could be more specificThe code converts signatures using
to_capk_signature()?and then performs arithmetic operations. If these conversions fail, the error message might not be very helpful.Consider wrapping these operations with more specific error context:
- .to_capk_signature()? - + &total_meta_data_signature.to_schnorr_signature()?, + .to_capk_signature() + .map_err(|e| TransactionServiceError::ServiceError( + format!("Failed to convert metadata signature for TxId {}: {}", tx_id, e) + ))? + + &total_meta_data_signature.to_schnorr_signature() + .map_err(|e| TransactionServiceError::ServiceError( + format!("Failed to convert total metadata signature for TxId {}: {}", tx_id, e) + ))?,Also applies to: 477-479
495-499: Consider extracting script validation logicThe script validation logic with public key arithmetic is complex and could benefit from being extracted to a separate method for better testability and readability.
+fn validate_script_offset(&self, tx_id: TxId, transaction: &Transaction, input_keys: UncompressedPublicKey, output_keys: UncompressedPublicKey) -> Result<(), TransactionServiceError> { + let lhs = input_keys - output_keys; + if lhs != UncompressedPublicKey::from_secret_key(&transaction.script_offset) { + return Err(TransactionServiceError::ServiceError(format!( + "Invalid script offset (TxId: {})", + tx_id + ))); + } + Ok(()) +}
1823-1825: Consider extracting self-payment check to a methodThe condition checks if the destination equals either the one-sided or interactive Tari address. This logic could be clearer if extracted to a helper method.
+fn is_self_payment(&self, dest_address: &TariAddress) -> bool { + dest_address == &self.resources.one_sided_tari_address || + dest_address == &self.resources.interactive_tari_address +} let payment_id = match payment_id.clone() { PaymentId::Open { .. } | PaymentId::Empty => PaymentId::add_sender_address( payment_id, self.resources.one_sided_tari_address.clone(), true, amount, fee_per_gram, - if dest_address == self.resources.one_sided_tari_address - || dest_address == self.resources.interactive_tari_address + if self.is_self_payment(&dest_address) { Some(TxType::PaymentToSelf) } else { Some(TxType::PaymentToOther) }, ), _ => payment_id, };
2564-2567: Consider consolidating error categorizationThe error categorization assigns reason code 3 to multiple error types that seem to have different root causes. Consider using more specific reason codes for better diagnostics.
let reason = match error { TransactionServiceError::TransactionValidationInProgress => 1, - TransactionServiceError::ProtobufConversionError(_) - | TransactionServiceError::RpcError(_) - | TransactionServiceError::InvalidMessageError(_) - | TransactionServiceError::BaseNodeChanged { .. } => 3, + TransactionServiceError::ProtobufConversionError(_) => 3, + TransactionServiceError::RpcError(_) => 4, + TransactionServiceError::InvalidMessageError(_) => 5, + TransactionServiceError::BaseNodeChanged { .. } => 6, _ => 2, };
2936-2937: Consider making the error message more user-friendlyThe error message for Ledger wallet limitation could be more informative about why interactive transactions aren't supported.
if sending_method.contains(TariAddressFeatures::create_interactive_only()) && matches!(*self.resources.wallet_type, WalletType::Ledger(_)) { return Err(TransactionServiceError::NotSupported( - "Interactive transactions are not supported on Ledger wallets".to_string(), + "Interactive transactions are not supported on Ledger wallets. Please use one-sided transactions instead.".to_string(), )); }applications/minotari_console_wallet/src/automation/commands.rs (2)
2697-2722: Consider security implications of file operations.The implementation correctly prepares unsigned transactions for offline signing. However, consider these security improvements:
- File permissions: Transaction data contains sensitive information. Consider setting restrictive file permissions.
- Path validation: Validate that the output file path is within expected directories to prevent path traversal attacks.
- Temporary file handling: Consider using secure temporary files or warning users about sensitive data storage.
match result { Ok(output) => { - fs::write(&args.output_file, output).map_err(|err| CommandError::FileWriteError { + // Set restrictive permissions (owner read/write only) + let mut file = File::create(&args.output_file).map_err(|err| CommandError::FileWriteError { file_path: args.output_file, err, })?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mut perms = file.metadata()?.permissions(); + perms.set_mode(0o600); // Owner read/write only + file.set_permissions(perms)?; + } + file.write_all(&output).map_err(|err| CommandError::FileWriteError { + file_path: args.output_file, + err, + })?; },
2744-2766: Good workflow integration with input validation consideration.The broadcast implementation correctly completes the offline signing workflow and integrates well with the existing transaction monitoring system. The transaction ID is properly added to the monitoring list.
Consider adding the same input validation (file size limits) as suggested for the signing command for consistency.
base_layer/core/src/transactions/transaction_components/wallet_output.rs (2)
219-253: Good refactoring to support pre-computed signatures.The method correctly checks for a pre-built signature before computing a new one, enabling the offline signing workflow while maintaining backward compatibility.
275-277: Minor: Unnecessary line break.The line break here seems unnecessary and could be combined for better readability.
- let total_ephemeral_public_key = CompressedPublicKey::new_from_pk( - aggregated_script_signature_public_nonces.to_public_key()? - + &ephemeral_public_key_self.pub_key.to_public_key()?, - ); + let total_ephemeral_public_key = CompressedPublicKey::new_from_pk( + aggregated_script_signature_public_nonces.to_public_key()? + &ephemeral_public_key_self.pub_key.to_public_key()?, + );base_layer/core/src/transactions/transaction_key_manager/inner.rs (1)
1724-1758: Secure encryption implementation with proper cryptographic practices.The encryption implementation demonstrates good security practices:
- XChaCha20Poly1305 is an excellent choice for AEAD encryption
- Using the private view key as the encryption key is appropriate for wallet contexts
- Domain separation with
b"key"prevents cross-context attacks- Proper zeroization of decrypted data
Consider using
Hiddenwrapper for theencryptedparameter indecrypt_keyto ensure it's treated as sensitive data throughout its lifetime:-pub async fn decrypt_key(&self, encrypted: Vec<u8>) -> Result<PrivateKey, KeyManagerServiceError> { +pub async fn decrypt_key(&self, encrypted: Vec<u8>) -> Result<PrivateKey, KeyManagerServiceError> {base_layer/wallet/src/transaction_service/offline_signing.rs (2)
56-62: Enhance version error message with compatibility information.Consider providing more helpful error messages that guide users on version compatibility.
if version != &supported_version { return Err(TransactionServiceError::SerializationError(format!( - "Unsupported version. Expected '{}', got '{}'", + "Unsupported transaction format version. Expected '{}', got '{}'. Please ensure both wallets are using compatible versions.", supported_version.to_string(), version.to_string(), ))); }
155-359: Add more debug logging for better troubleshooting.The method performs many critical steps but only has one debug log. Consider adding debug logs at key points for better observability.
Add debug logging at these key points:
// After line 219 +debug!(target: LOG_TARGET, "Transaction {} prepared with {} inputs", tx_id, stp.get_amount_to_self().unwrap_or_default()); // After line 262 +debug!(target: LOG_TARGET, "Commitment mask key imported for transaction {}", tx_id); // After line 271 +debug!(target: LOG_TARGET, "Stealth address script generated for transaction {}", tx_id); // After line 344 +debug!(target: LOG_TARGET, "Transaction {} finalized and ready for signing", tx_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
applications/minotari_app_grpc/proto/wallet.proto(2 hunks)applications/minotari_console_wallet/src/automation/commands.rs(11 hunks)applications/minotari_console_wallet/src/automation/error.rs(3 hunks)applications/minotari_console_wallet/src/cli.rs(2 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(4 hunks)applications/minotari_console_wallet/src/lib.rs(5 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(4 hunks)base_layer/core/src/transactions/transaction_components/wallet_output.rs(8 hunks)base_layer/core/src/transactions/transaction_key_manager/error.rs(1 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(16 hunks)base_layer/core/src/transactions/transaction_key_manager/interface.rs(3 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(5 hunks)base_layer/core/src/transactions/transaction_protocol/sender.rs(24 hunks)base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs(6 hunks)base_layer/wallet/Cargo.toml(1 hunks)base_layer/wallet/src/transaction_service/error.rs(2 hunks)base_layer/wallet/src/transaction_service/handle.rs(6 hunks)base_layer/wallet/src/transaction_service/mod.rs(2 hunks)base_layer/wallet/src/transaction_service/offline_signing.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(19 hunks)integration_tests/src/wallet_process.rs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
base_layer/wallet/src/transaction_service/mod.rs (1)
base_layer/p2p/src/services/utils.rs (1)
map_decode(27-35)
base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (2)
base_layer/core/src/transactions/transaction_key_manager/inner.rs (5)
get_script_private_key(1043-1057)get_script_offset_from_private_key(1059-1079)encrypt_key(1730-1738)get_encrypted_key(1740-1743)decrypt_key(1745-1758)base_layer/core/src/transactions/transaction_key_manager/interface.rs (5)
get_script_private_key(387-387)get_script_offset_from_private_key(389-393)encrypt_key(481-481)get_encrypted_key(483-483)decrypt_key(485-485)
base_layer/core/src/transactions/transaction_key_manager/interface.rs (2)
base_layer/core/src/transactions/transaction_key_manager/inner.rs (5)
get_script_private_key(1043-1057)get_script_offset_from_private_key(1059-1079)encrypt_key(1730-1738)get_encrypted_key(1740-1743)decrypt_key(1745-1758)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (5)
get_script_private_key(416-422)get_script_offset_from_private_key(424-434)encrypt_key(611-613)get_encrypted_key(615-621)decrypt_key(623-629)
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (3)
base_layer/wallet/src/transaction_service/offline_signing.rs (2)
prepare_one_sided_transaction_for_signing(157-359)new(119-129)base_layer/wallet/src/transaction_service/handle.rs (3)
prepare_one_sided_transaction_for_signing(736-760)new(620-628)broadcast_signed_one_sided_transaction(773-785)base_layer/core/src/transactions/transaction_components/encrypted_data.rs (2)
from_bytes(402-539)from_bytes(703-715)
🔇 Additional comments (46)
base_layer/wallet/Cargo.toml (1)
66-66: LGTM: Appropriate dependency addition for offline signing functionality.The semver dependency with serde feature support is correctly added to enable version compatibility checks and serialization in the offline signing module.
base_layer/core/src/transactions/transaction_key_manager/error.rs (1)
89-92: LGTM: Well-structured error variants for cryptographic operations.The new
EncryptionFailedandDecryptionFailederror variants follow the existing enum patterns and provide appropriate error handling for the new encryption/decryption functionality in the key manager.integration_tests/src/wallet_process.rs (2)
189-199: LGTM: Formatting improvement enhances readability.The reformatted wallet insertion improves code readability without affecting functionality.
246-246: LGTM: Appropriate default value for skip_recovery field.The addition of
skip_recovery: falsemaintains backward compatibility while supporting the new CLI option for offline signing workflows.base_layer/wallet/src/transaction_service/error.rs (2)
23-23: LGTM: Necessary imports for file operation error handling.The added imports support the new file-related error variants introduced for offline signing file operations.
205-208: LGTM: Comprehensive error handling for file operations.The new
FileReadErrorandFileWriteErrorvariants provide excellent error context by including both the file path and the underlying IO error. This is particularly valuable for debugging file operation issues in offline signing workflows.applications/minotari_console_wallet/src/automation/error.rs (1)
26-26: LGTM: Well-structured error variants for file operations.The new
FileWriteErrorandFileReadErrorvariants properly include both the file path and underlying I/O error, providing helpful context for debugging file operation failures in the offline signing workflow.Also applies to: 101-104
base_layer/wallet/src/transaction_service/mod.rs (1)
35-35: LGTM: Clean import consolidation and module organization.The import consolidations improve readability by grouping related imports together, and the new
offline_signingmodule is properly positioned with other module declarations to support the offline signing functionality.Also applies to: 40-40, 44-44, 65-65
applications/minotari_console_wallet/src/lib.rs (1)
99-99: LGTM: Clean implementation of skip recovery functionality.The
skip_recoveryflag is properly integrated throughout the wallet initialization flow. The conditional logic correctly gates recovery continuation while maintaining backward compatibility, and the parameter is appropriately passed to the recovery mode function.Also applies to: 215-215, 250-257
base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs (2)
640-640: LGTM: Proper initialization of encrypted script private key field.The addition of
encrypted_script_private_key: Noneto theRawTransactionInfostruct correctly initializes the new field to support offline signing functionality. This aligns with the broader offline signing feature implementation.
287-288:Details
✅ Verification successful
Verify the method signature change is consistent across the codebase.
The method calls have been updated from a different signature to
get_serialized_size(). Ensure that this change is consistent with the API definition and that all callers have been updated accordingly.Also applies to: 343-346
🏁 Script executed:
#!/bin/bash # Description: Verify all usages of get_serialized_size method to ensure consistency # Search for any remaining old method signatures that might need updating rg -A 3 "\.get_serialized_size\(" --type rust # Search for any potential method signature mismatches rg -A 3 "serialized_size|get_serialized|SerializedSize" --type rustLength of output: 36024
All
get_serialized_size()calls are consistent across the codebase
- We scanned all occurrences of
get_serialized_size()and found only calls matching the new signature.- The
SerializedSizetrait inbase_layer/core/src/common/borsh.rsdefinesfn get_serialized_size(&self) -> Result<usize>, and every caller has been updated to use it.- No leftover calls to an old
serialized_sizeAPI or signature mismatches were detected.applications/minotari_console_wallet/src/wallet_modes.rs (3)
418-418: LGTM! Function signature updated to support offline signing.The addition of the
skip_recoveryparameter enables cold wallets to bypass recovery when operating offline, which aligns with the PR objectives for offline signing support.
420-446: Well-implemented conditional recovery logic.The conditional execution of recovery based on the
skip_recoveryflag is correctly implemented. The recovery process, logging, and error handling are properly bypassed whenskip_recoveryis true, while still proceeding to start the appropriate wallet mode.
628-630: Test coverage updated for new CLI commands.The addition of the three new offline signing CLI commands (
PrepareOneSidedTransactionForSigning,SignOneSidedTransaction,BroadcastSignedOneSidedTransaction) to the test parsing logic ensures these commands are recognized during command file parsing.applications/minotari_app_grpc/proto/wallet.proto (2)
227-229: LGTM! gRPC methods added for offline signing workflow.The two new gRPC methods
PrepareOneSidedTransactionForSigningandBroadcastSignedOneSidedTransactioncorrectly implement the hot wallet side of the offline signing workflow as described in the PR objectives.
1091-1111: Well-structured protobuf message definitions.The message definitions for offline signing are appropriately structured:
PrepareOneSidedTransactionForSigningRequestuses the existingPaymentRecipienttype for consistency- Response includes success status and detailed error messaging
BroadcastSignedOneSidedTransactionRequestaccepts a string request (likely serialized transaction data)- All responses follow the established pattern with success flags and failure messages
applications/minotari_console_wallet/src/cli.rs (3)
107-109: LGTM! Skip recovery flag added for offline wallet operation.The
skip_recoveryflag enables cold wallets to bypass the recovery process when operating offline, which is essential for the offline signing workflow described in the PR objectives.
180-182: Complete CLI command set for offline signing workflow.The three new CLI commands provide the complete offline signing workflow:
PrepareOneSidedTransactionForSigning: Hot wallet prepares unsigned transactionSignOneSidedTransaction: Cold wallet signs the transactionBroadcastSignedOneSidedTransaction: Hot wallet broadcasts signed transactionThis aligns perfectly with the airgap workflow described in the PR objectives.
190-212: Well-structured argument definitions for offline signing commands.The argument structures are well-designed:
PrepareOneSidedTransactionForSigningArgs: Includes amount, destination, payment ID, and output fileSignOneSidedTransactionArgs: Simple input/output file pair for the signing stepBroadcastSignedOneSidedTransactionArgs: Input file for the signed transactionThe use of
PathBuffor file operations is appropriate for cross-platform compatibility, and the payment ID default provides user-friendly behavior.base_layer/wallet/src/transaction_service/service.rs (4)
1204-1206: LGTM: Proper public key comparisonThe change correctly compares the public key component instead of the entire key object, which is the appropriate way to check if we're sending to ourselves.
2533-2542: Good enhancement to burn transaction return valueThe addition of
BurntProofto the return value provides essential information for L2 interactions, including the reciprocal claim public key, commitment, ownership proof, and range proof.
2607-2610: LGTM: Proper status check for broadcast eligibilityThe addition of
MinedUnconfirmedstatus to the broadcast eligibility check is correct, as transactions in this state may need to be rebroadcast.
3077-3078: Good optimization using contains_keyUsing
contains_keyinstead of checking values is more efficient and clearly expresses the intent of checking for transaction existence.base_layer/core/src/transactions/transaction_key_manager/wrapper.rs (4)
41-43: LGTM! Import additions support new functionality.The new imports for
PaymentIdandTariKeyIdare necessary for the offline signing methods being added. The import organization follows the existing structure.Also applies to: 48-48
64-65: Good formatting improvement.Moving the
whereclause to a new line improves readability and follows Rust formatting conventions.Also applies to: 94-95, 634-635
416-434: Script key management methods correctly implemented.Both
get_script_private_keyandget_script_offset_from_private_keyfollow the established wrapper pattern:
- Proper use of read locks for non-mutating operations
- Correct delegation to the inner implementation
- Appropriate async/await usage
- Consistent error propagation
These methods are essential for the offline signing workflow where script keys need to be managed separately.
611-629: Encryption/decryption methods properly implemented.All three encryption-related methods (
encrypt_key,get_encrypted_key,decrypt_key) correctly:
- Use read locks for accessing the inner implementation
- Follow the established delegation pattern
- Handle async operations properly
- Maintain consistent error handling with
KeyManagerServiceErrorThese methods enable secure key storage and retrieval for offline signing scenarios.
applications/minotari_console_wallet/src/automation/commands.rs (2)
54-54: LGTM! Formatting improvements enhance readability.The formatting changes to import statements and function calls improve code readability without introducing any functional changes.
Also applies to: 66-67, 86-87, 94-94, 110-116, 127-134, 138-145, 390-395, 586-589, 620-621, 646-648, 654-661
2697-2766: Excellent implementation of offline signing workflow.The three new commands successfully implement the offline signing workflow as described in the PR objectives:
- PrepareOneSidedTransactionForSigning: Creates unsigned transaction on hot wallet
- SignOneSidedTransaction: Signs transaction on cold wallet
- BroadcastSignedOneSidedTransaction: Broadcasts signed transaction from hot wallet
The integration with the existing command runner is clean and follows established patterns. The error handling is appropriate and consistent with the rest of the codebase.
base_layer/core/src/transactions/transaction_components/wallet_output.rs (3)
71-71: Good addition for offline signing support.The optional
script_signaturefield enables storing pre-computed signatures, which is essential for the offline signing workflow where signatures are computed on cold wallets.
119-119: Correct initialization of the new field.Initializing
script_signaturetoNonein all constructors is the right approach, maintaining backward compatibility.Also applies to: 157-157
198-216: Well-designed signature building method.The
build_script_signaturemethod properly extracts the signature building logic with good flexibility through the optionalspending_key_idparameter. This supports both regular signing (using the output's own key) and advanced scenarios where a different key might be used.base_layer/core/src/transactions/transaction_key_manager/interface.rs (2)
387-393: Well-designed trait methods for script key management.These new async methods provide the necessary functionality for offline signing:
get_script_private_key: Aggregates multiple script private keysget_script_offset_from_private_key: Computes script offset from a private keyThe method signatures follow Rust conventions and properly handle errors.
481-485: Essential encryption methods for secure offline signing.The encryption/decryption methods enable secure key transmission between hot and cold wallets:
encrypt_key: Encrypts private keys for safe storage/transmissionget_encrypted_key: Convenience method to encrypt by key IDdecrypt_key: Recovers the original private keyThe use of
Vec<u8>for encrypted data and proper error types is appropriate.base_layer/core/src/transactions/transaction_key_manager/inner.rs (2)
1043-1057: Correct implementation of script key aggregation.The method properly aggregates multiple script private keys by summing them, which is the standard approach in elliptic curve cryptography. The error handling for Ledger wallets is appropriate since they don't expose private keys directly.
1059-1079: Correct script offset calculation.The method properly computes the script offset using the formula:
script_offset = script_private_key - total_sender_offset_private_key. This is the correct mathematical operation for deriving offsets in the transaction protocol.base_layer/core/src/transactions/transaction_protocol/sender.rs (4)
99-100: LGTM! Good addition for offline signing support.The encrypted script private key field is appropriately added as an optional field to support offline signing scenarios.
626-642: LGTM! Well-implemented script key persistence.The method correctly calculates and encrypts the script private key for offline signing scenarios.
779-787: Excellent backward-compatible implementation.The script offset calculation correctly handles both encrypted and non-encrypted scenarios, maintaining backward compatibility while supporting offline signing.
917-937: LGTM! Clean implementation for retrieving encrypted keys.The method correctly retrieves encrypted keys for all inputs, which is essential for the offline signing workflow.
base_layer/wallet/src/transaction_service/offline_signing.rs (1)
1-390: Well-implemented offline signing module with room for minor improvements.The offline signing implementation is comprehensive and follows good practices. The version management ensures compatibility, and the separation between preparation and signing phases is clean. Consider the suggested improvements for better observability and validation.
base_layer/wallet/src/transaction_service/handle.rs (5)
44-44: LGTM - Import additions support the new functionality.The added imports are necessary for the offline signing feature and follow proper Rust conventions.
Also applies to: 61-61
151-164: LGTM - Well-structured request variants for offline signing workflow.The new enum variants follow established patterns and appropriately separate the three-step offline signing process. The use of String parameters for serialized transaction data supports the airgap transfer requirement.
350-363: LGTM - Display implementations maintain consistency.The display implementations follow the established pattern and provide appropriate debugging information for the new request variants.
464-465: LGTM - Appropriate response variants for offline signing.The response variants properly support the offline signing workflow with String types for serialized transaction data transfer.
736-785: LGTM - Well-implemented handle methods for offline signing.The new async methods follow the established patterns in
TransactionServiceHandle:
- Consistent error handling with proper service call management
- Appropriate parameter and return types for the offline signing workflow
- Correct response matching and unexpected response handling
The String parameters provide the necessary serialization flexibility for airgap transfers between hot and cold wallets.
applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
Outdated
Show resolved
Hide resolved
base_layer/core/src/transactions/transaction_protocol/sender.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
base_layer/wallet/src/transaction_service/offline_signing.rs (4)
41-43: Consider compile-time version validation.The
unwrap()onVersion::parse()could panic ifSUPPORTED_VERSIONis malformed. Since this is a constant, consider using a compile-time validation or aconstassertion to ensure the version string is always valid.-const SUPPORTED_VERSION: &str = "1.0.0"; - -fn get_supported_version() -> Version { - Version::parse(SUPPORTED_VERSION).unwrap() -} +const SUPPORTED_VERSION: &str = "1.0.0"; + +fn get_supported_version() -> Version { + Version::parse(SUPPORTED_VERSION).expect("SUPPORTED_VERSION must be a valid semver") +}
157-219: Consider refactoring this method to improve maintainability.The
prepare_one_sided_transaction_for_signingmethod is quite long (200+ lines) and handles multiple concerns including payment ID processing, address verification, key generation, and transaction preparation. Consider breaking this into smaller, focused helper methods to improve readability and testability.For example, you could extract:
- Payment ID processing logic (lines 170-192)
- Script preparation logic (lines 194-201)
- Key generation and shared secret creation (lines 224-262)
- Output building (lines 299-328)
170-174: Clarify the payment ID override comment.The comment "let override the payment_id if the address says we should" could be more descriptive about when and why this override occurs.
- // let override the payment_id if the address says we should + // Override the payment_id if the destination address contains embedded payment data if dest_address.features().contains(TariAddressFeatures::PAYMENT_ID) {
224-234: Document the protocol requirement more clearly.The comment explains why
build_single_round_messageis called but the result discarded. Consider making this requirement more explicit in the code structure to avoid confusion.// This call is needed to advance the state from `SingleRoundMessageReady` to `SingleRoundMessageReady`, - // but the returned value is not used. We have to wait until the sender transaction protocol creates a + // but the returned value is not used. The protocol requires this call to generate the // sender_offset_private_key for us, so we can use it to create the shared secret let key = self
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
applications/minotari_console_wallet/src/automation/commands.rs(11 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(4 hunks)base_layer/core/src/transactions/transaction_protocol/sender.rs(24 hunks)base_layer/wallet/src/transaction_service/offline_signing.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- applications/minotari_console_wallet/src/automation/commands.rs
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
- base_layer/wallet/src/transaction_service/service.rs
- base_layer/core/src/transactions/transaction_protocol/sender.rs
🔇 Additional comments (3)
base_layer/wallet/src/transaction_service/offline_signing.rs (3)
365-371: Good addition of input validation.The validation check for empty encrypted commitment mask keys addresses the previous review comment and provides appropriate error handling before proceeding with the signing process.
375-384:Details
❓ Verification inconclusive
Verify key import security.
The loop decrypts and imports multiple keys. Ensure that the
decrypt_keyandimport_keyoperations are secure and don't leak sensitive data in case of partial failures.
🏁 Script executed:
#!/bin/bash # Check if there are proper cleanup mechanisms for key operations ast-grep --pattern 'decrypt_key($$$)' ast-grep --pattern 'import_key($$$)'Length of output: 78
Unable to confirm secure cleanup after partial failures
I wasn’t able to locate the implementations of
decrypt_keyorimport_keyvia the automated search. Please manually review that both operations:
- Wipe or zeroize any plaintext key material immediately on errors or panics
- Don’t retain decrypted keys in memory longer than necessary
- Provide atomic import or rollback mechanisms so that partial failures can’t leak sensitive data
File: base_layer/wallet/src/transaction_service/offline_signing.rs (lines 375–384)
386-389:Details
❌ Incorrect review comment
Ensure atomic operation for signing persistence.
The two separate calls to
persist_input_script_signaturesandpersist_script_private_keyshould be atomic to prevent partial state in case of failure between the calls.
🏁 Script executed:
#!/bin/bash # Check if these persist operations are transactional ast-grep --pattern 'persist_input_script_signatures($$$)' -A 10 ast-grep --pattern 'persist_script_private_key($$$)' -A 10Length of output: 126
I wasn’t able to locate the implementations of these two methods—let’s find their definitions and inspect whether they enlist both operations in a single transaction.
🏁 Script executed:
#!/bin/bash # Find definitions and surrounding code for both persistence methods rg "fn persist_input_script_signatures" -n -A10 rg "fn persist_script_private_key" -n -A10Length of output: 2759
Let’s inspect the calls in offline_signing.rs and see how/when the updated
stpgets written back:
🏁 Script executed:
#!/bin/bash # Show context around the two persist calls rg "persist_input_script_signatures" -n -C5 base_layer/wallet/src/transaction_service/offline_signing.rs rg "persist_script_private_key" -n -C5 base_layer/wallet/src/transaction_service/offline_signing.rs # Find where the transaction protocol (stp) is saved after mutation rg "upsert_transaction" -n -C5 base_layer/wallet/src/transaction_service/offline_signing.rs rg "save_transaction" -n -C5 base_layer/wallet/src/transaction_service/offline_signing.rsLength of output: 1372
Let’s grab a wider slice of the surrounding logic—this should reveal how
stpis loaded and whether there’s a single DB commit after both calls:
🏁 Script executed:
#!/bin/bash # Show 25 lines around the persist calls to see loading & final save rg -n -C25 "stp.persist_input_script_signatures" base_layer/wallet/src/transaction_service/offline_signing.rsLength of output: 1750
No external persistence—atomicity not required
The two
persist_*calls only mutate the clonedSenderTransactionProtocolin memory—they don’t write to a database or other durable store. The fully updatedstpis then returned in one go, and any real persistence happens later in a single operation. There’s no risk of a “partial write” here.If the
persist_*naming feels misleading, consider renaming them to something likeapply_input_script_signaturesandapply_script_private_keyto reflect that they’re in-memory updates.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
base_layer/core/src/transactions/transaction_protocol/sender.rs (1)
620-646: Length validation correctly implemented.The method properly validates that
commitment_mask_key_idslength matches the number of inputs before proceeding with the zip operation, preventing silent truncation issues.
🧹 Nitpick comments (2)
base_layer/wallet/src/transaction_service/offline_signing.rs (2)
42-44: Consider adding error context for version parsing.While the
unwrap()is likely safe sinceSUPPORTED_VERSIONis a constant, consider making this more explicit for maintainability.fn get_supported_version() -> Version { - Version::parse(SUPPORTED_VERSION).unwrap() + Version::parse(SUPPORTED_VERSION) + .expect("SUPPORTED_VERSION constant should be valid semver") }
222-235: Clarify the purpose of the state advancement call.The comment mentions that the returned value is not used but the call is needed to advance state. Consider making this more explicit to avoid confusion.
- // This call is needed to advance the state from `SingleRoundMessageReady` to `SingleRoundMessageReady`, - // but the returned value is not used. We have to wait until the sender transaction protocol creates a - // sender_offset_private_key for us, so we can use it to create the shared secret + // This call advances the sender transaction protocol state and generates the sender_offset_private_key + // needed for shared secret creation. The returned message is discarded as we only need the state change. let key = self .resources .transaction_key_manager_service .get_next_key(TransactionKeyManagerBranch::OneSidedSenderOffset.get_branch_key()) .await?; stp.change_recipient_sender_offset_private_key(key.key_id)?; - let _single_round_sender_data = stp + let _unused_message = stp .build_single_round_message(&self.resources.transaction_key_manager_service) .await .map_err(|e| TransactionServiceProtocolError::new(tx_id, e.into()))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
applications/minotari_console_wallet/src/automation/commands.rs(1 hunks)applications/minotari_console_wallet/src/automation/error.rs(2 hunks)applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs(3 hunks)applications/minotari_console_wallet/src/lib.rs(3 hunks)applications/minotari_console_wallet/src/wallet_modes.rs(2 hunks)base_layer/core/src/transactions/transaction_components/wallet_output.rs(4 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(5 hunks)base_layer/core/src/transactions/transaction_key_manager/interface.rs(2 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(2 hunks)base_layer/core/src/transactions/transaction_protocol/sender.rs(5 hunks)base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs(1 hunks)base_layer/wallet/src/transaction_service/handle.rs(4 hunks)base_layer/wallet/src/transaction_service/mod.rs(1 hunks)base_layer/wallet/src/transaction_service/offline_signing.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(4 hunks)integration_tests/src/wallet_process.rs(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs
- base_layer/wallet/src/transaction_service/mod.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- applications/minotari_console_wallet/src/wallet_modes.rs
- integration_tests/src/wallet_process.rs
- applications/minotari_console_wallet/src/lib.rs
- applications/minotari_console_wallet/src/automation/error.rs
- base_layer/core/src/transactions/transaction_components/wallet_output.rs
- base_layer/core/src/transactions/transaction_key_manager/wrapper.rs
- applications/minotari_console_wallet/src/automation/commands.rs
- base_layer/core/src/transactions/transaction_key_manager/interface.rs
- base_layer/wallet/src/transaction_service/service.rs
- applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
- base_layer/wallet/src/transaction_service/handle.rs
🔇 Additional comments (16)
base_layer/core/src/transactions/transaction_key_manager/inner.rs (6)
25-25: LGTM: Appropriate cryptographic imports added.The new imports support the encryption/decryption functionality with good security practices:
- XChaCha20Poly1305 is a solid authenticated encryption choice
- Hidden and Zeroize help protect sensitive data in memory
Also applies to: 42-42, 83-83, 85-85
1065-1079: LGTM: Correct script private key aggregation.The method properly aggregates multiple script private keys through addition, which is mathematically correct for elliptic curve private keys. The Ledger wallet exclusion is appropriate for offline signing operations.
1081-1101: LGTM: Correct script offset calculation.The method properly calculates the script offset by subtracting the aggregated sender offset keys from the script private key. The mathematical operations are correct for elliptic curve arithmetic.
1746-1750: LGTM: Appropriate cipher initialization.Using the private view key as the cipher key is reasonable for the offline signing use case. XChaCha20Poly1305 provides strong authenticated encryption suitable for protecting private key material.
1752-1765: LGTM: Secure key encryption implementation.Good security practices implemented:
- Domain separation prevents confusion with other encrypted data types
- Hidden::hide protects sensitive key material in memory during encryption
- Proper error handling and propagation
1767-1780: LGTM: Excellent security practices in decryption.The implementation demonstrates strong security practices:
- Consistent domain separation matching encryption
- Proper error handling for both decryption and key parsing failures
- Zeroizing decrypted data after use prevents sensitive material from lingering in memory
base_layer/core/src/transactions/transaction_protocol/sender.rs (5)
114-115: LGTM: Clean field addition for offline signing support.The optional
encrypted_script_private_keyfield is well-designed for storing encrypted script private keys in the offline signing workflow.
648-663: LGTM: Secure script private key handling.The method properly delegates key calculation and uses the key manager for encryption, following good security practices for offline signing.
665-677: LGTM: Clean helper method implementation.The method correctly collects script key IDs from inputs and delegates to the key manager for aggregation.
800-808: LGTM: Clean integration of offline signing support.The conditional logic properly handles both encrypted (offline signing) and unencrypted (normal) scenarios while maintaining backward compatibility.
938-957: LGTM: Well-implemented encrypted key retrieval.The method correctly retrieves encrypted keys for inputs and properly restricts usage to appropriate transaction states for the offline signing workflow.
base_layer/wallet/src/transaction_service/offline_signing.rs (5)
172-175: Payment ID override logic is correct but complex.The logic correctly handles payment ID extraction from addresses containing the PAYMENT_ID feature. The debug logging helps with troubleshooting.
366-372: Validation logic correctly addresses past review feedback.The validation for empty encrypted commitment mask keys and debug logging have been properly implemented as requested in the previous review.
377-385: Key decryption and import loop is secure and correct.The implementation properly decrypts each encrypted key and imports it into the key manager, building the commitment_mask_key_ids vector for subsequent signature operations.
387-390: Script signature persistence completes the signing workflow.The calls to persist input script signatures and script private keys properly finalize the signing process by storing the necessary cryptographic components.
156-360: Comprehensive transaction preparation with proper security measures.The
prepare_one_sided_transaction_for_signingmethod implements a robust workflow that:
- Validates addresses and features
- Handles stealth addresses correctly
- Manages payment IDs appropriately
- Creates proper shared secrets and encryption keys
- Builds outputs with all required signatures and metadata
The method follows established cryptographic patterns and includes comprehensive error handling.
SWvheerden
left a comment
SWvheerden
left a comment
There was a problem hiding this comment.
Private keys should never leave the keymanager
We should not let any outside process need to handle the keys
| }) | ||
| } | ||
|
|
||
| pub async fn get_script_private_key(&self, script_key_ids: &[TariKeyId]) -> Result<PrivateKey, TransactionError> { |
There was a problem hiding this comment.
| pub async fn get_script_private_key(&self, script_key_ids: &[TariKeyId]) -> Result<PrivateKey, TransactionError> { | |
| async fn get_script_private_key(&self, script_key_ids: &[TariKeyId]) -> Result<PrivateKey, TransactionError> { |
There was a problem hiding this comment.
Removed get_script_private_key entirely.
| } | ||
| } | ||
|
|
||
| pub async fn get_script_offset_from_private_key( |
There was a problem hiding this comment.
| pub async fn get_script_offset_from_private_key( | |
| async fn get_script_offset_from_private_key( |
There was a problem hiding this comment.
Removed get_script_offset_from_private_key entirely.
| async fn get_script_private_key(&self, script_key_ids: &[TariKeyId]) -> Result<PrivateKey, TransactionError> { | ||
| self.transaction_key_manager_inner | ||
| .read() | ||
| .await | ||
| .get_script_private_key(script_key_ids) | ||
| .await | ||
| } | ||
|
|
||
| async fn get_script_offset_from_private_key( | ||
| &self, | ||
| script_private_key: PrivateKey, | ||
| sender_offset_key_ids: &[TariKeyId], | ||
| ) -> Result<PrivateKey, TransactionError> { | ||
| self.transaction_key_manager_inner | ||
| .read() | ||
| .await | ||
| .get_script_offset_from_private_key(script_private_key, sender_offset_key_ids) | ||
| .await | ||
| } | ||
|
|
There was a problem hiding this comment.
| async fn get_script_private_key(&self, script_key_ids: &[TariKeyId]) -> Result<PrivateKey, TransactionError> { | |
| self.transaction_key_manager_inner | |
| .read() | |
| .await | |
| .get_script_private_key(script_key_ids) | |
| .await | |
| } | |
| async fn get_script_offset_from_private_key( | |
| &self, | |
| script_private_key: PrivateKey, | |
| sender_offset_key_ids: &[TariKeyId], | |
| ) -> Result<PrivateKey, TransactionError> { | |
| self.transaction_key_manager_inner | |
| .read() | |
| .await | |
| .get_script_offset_from_private_key(script_private_key, sender_offset_key_ids) | |
| .await | |
| } |
The privatee keys should never leave the key manager
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
base_layer/wallet/src/transaction_service/offline_signing.rs (4)
45-47: Avoidunwrap()on constant parsing to stay panic-free
Version::parse(SUPPORTED_VERSION).unwrap()will panic if the constant is ever
changed to an invalid semver string.
Even though it is unlikely in normal operation, this is an unnecessary risk in
core wallet code.-fn get_supported_version() -> Version { - Version::parse(SUPPORTED_VERSION).unwrap() +fn get_supported_version() -> Version { + Version::parse(SUPPORTED_VERSION) + .expect("`SUPPORTED_VERSION` must be a valid semver literal") }
58-67: Version-compat check is too strict – blocks forwards-compatible readersComparing for full equality (
version != &supported_version) will reject files
that only differ in patch‐level (e.g.1.0.1). This makes forward / backward
compatibility brittle and breaks once any non-breaking fix is released.Consider using a
VersionReqsuch as^1.0.0or at least ignoring the patch
segment to allow minor hot-fixes.- let supported_version = get_supported_version(); - if version != &supported_version { + let supported_version = get_supported_version(); + // Accept any `1.x.y` file + if version.major != supported_version.major || version.minor != supported_version.minor { return Err(TransactionServiceError::SerializationError(format!( "Unsupported version. Expected '{}', got '{}'", supported_version.to_string(), version.to_string(), ))); }
261-266: Use anInvalidRequesterror instead ofNotSupportedAn empty
encrypted_commitment_mask_keysvector means the request is malformed
rather than the feature being unsupported. Surfacing it as
InvalidRequest(or introducing a dedicated variant) provides clearer intent
for API consumers and keeps the semantic distinction between unsupported and
invalid.- if request.encrypted_commitment_mask_keys.is_empty() { - return Err(TransactionServiceError::NotSupported( - "No encrypted commitment mask keys found in the request".to_string(), - )); + if request.encrypted_commitment_mask_keys.is_empty() { + return Err(TransactionServiceError::InvalidRequest( + "No encrypted commitment mask keys found in the request".to_string(), + )); }
288-292: Decrypting keys sequentially is IO-bound – considerjoin_allEach call to
decrypt_and_import_keyperforms an async round-trip to the key
manager. When plenty of inputs are present this tightforloop becomes the
critical path.use futures::future::join_all; let commitment_mask_key_ids = join_all( request.encrypted_commitment_mask_keys.into_iter().map(|k| self.decrypt_and_import_key(k)), ) .await .into_iter() .collect::<Result<Vec<_>, _>>()?;Small change, but it keeps latency bounded by the slowest decrypt instead of
their sum.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
base_layer/core/src/transactions/transaction_components/wallet_output.rs(4 hunks)base_layer/core/src/transactions/transaction_key_manager/inner.rs(4 hunks)base_layer/core/src/transactions/transaction_key_manager/interface.rs(1 hunks)base_layer/core/src/transactions/transaction_key_manager/wrapper.rs(1 hunks)base_layer/core/src/transactions/transaction_protocol/sender.rs(5 hunks)base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs(1 hunks)base_layer/wallet/src/transaction_service/offline_signing.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- base_layer/core/src/transactions/transaction_protocol/transaction_initializer.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- base_layer/core/src/transactions/transaction_key_manager/inner.rs
- base_layer/core/src/transactions/transaction_key_manager/interface.rs
- base_layer/core/src/transactions/transaction_key_manager/wrapper.rs
- base_layer/core/src/transactions/transaction_components/wallet_output.rs
- base_layer/core/src/transactions/transaction_protocol/sender.rs
| let script_spending_key = self | ||
| .resources | ||
| .transaction_key_manager_service | ||
| .stealth_address_script_spending_key(commitment_mask_key_id, dest_address.public_spend_key()) | ||
| .await?; | ||
| script = push_pubkey_script(&script_spending_key); | ||
| } | ||
|
|
||
| let output = WalletOutputBuilder::new(amount, commitment_mask_key_id.clone()) | ||
| .with_features(output_features) | ||
| .with_script(script.clone()) | ||
| .encrypt_data_for_recovery( | ||
| &self.resources.transaction_key_manager_service, | ||
| Some(&encryption_key), | ||
| payment_id.clone(), | ||
| ) | ||
| .await? | ||
| .with_input_data(Default::default()) | ||
| .with_sender_offset_public_key(sender_offset_public_key) | ||
| .with_script_key(TariKeyId::Zero) | ||
| .with_minimum_value_promise(minimum_value_promise) |
There was a problem hiding this comment.
Hard-coding script_key to TariKeyId::Zero breaks spendability
For stealth outputs the script spending key is freshly derived
(script_spending_key) but the builder later pins the script_key to
TariKeyId::Zero.
Down-stream validation will fail because the key manager cannot locate the real
private key for that script, rendering the output unspendable.
Proposed fix – import the derived key and use its TariKeyId:
- if use_stealth_address {
- let script_spending_key = self
- .resources
- .transaction_key_manager_service
- .stealth_address_script_spending_key(commitment_mask_key_id, dest_address.public_spend_key())
- .await?;
- script = push_pubkey_script(&script_spending_key);
- }
+ let mut script_key_id = TariKeyId::Zero;
+ if use_stealth_address {
+ let script_spending_key = self
+ .resources
+ .transaction_key_manager_service
+ .stealth_address_script_spending_key(commitment_mask_key_id, dest_address.public_spend_key())
+ .await?;
+ script = push_pubkey_script(&script_spending_key);
+ // Persist the spending key so the wallet can later sign the spend
+ script_key_id = self
+ .resources
+ .transaction_key_manager_service
+ .import_key(script_spending_key)
+ .await?;
+ }
...
- .with_script_key(TariKeyId::Zero)
+ .with_script_key(script_key_id)🤖 Prompt for AI Agents
In base_layer/wallet/src/transaction_service/offline_signing.rs between lines
337 and 357, the output builder sets the script_key to TariKeyId::Zero, which
breaks spendability for stealth outputs because the key manager cannot find the
correct private key. To fix this, replace the hard-coded TariKeyId::Zero with
the TariKeyId of the derived script_spending_key so that the correct key is used
for validation and spending.
…OneSidedTransactionForSigning
SWvheerden
left a comment
SWvheerden
left a comment
There was a problem hiding this comment.
small nit, but I think I am happy
| pub output_features: OutputFeatures, | ||
| pub script: TariScript, | ||
| pub address: TariAddress, | ||
| pub use_stealth_address: bool, |
There was a problem hiding this comment.
| pub use_stealth_address: bool, |
Enforce stealth, dont even make it an option.
non stealth is more legacy support than anything else
| let mut script = info.recipient.script.clone(); | ||
| if info.recipient.use_stealth_address { | ||
| let script_spending_key = self | ||
| .key_manager | ||
| .stealth_address_script_spending_key(&commitment_mask_key_id, info.recipient.address.public_spend_key()) | ||
| .await?; | ||
| script = push_pubkey_script(&script_spending_key); | ||
| } |
There was a problem hiding this comment.
always enforce stealth
There was a problem hiding this comment.
always enforce stealth
@SWvheerden Updated to always enforce stealth - 3f1d42b
Also, verified, that it works. Created 2 transactions on a brand new wallet.
|
testing it locally, there doesn't seem to be a way to unencumber the funds. i.e. if I call prepare-one-sided twice, it fails |
|
it also looks like you are literally putting in "" as the message when it is blank |
Test Results (Integration tests) 2 files 10 suites 1h 16m 56s ⏱️ For more details on these failures, see this check. Results for commit d5a70fc. ♻️ This comment has been updated with latest results. |
Test Results (CI) 3 files 126 suites 39m 53s ⏱️ Results for commit d5a70fc. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs (1)
193-196: Security concern: Imported keys lack explicit cleanup.The imported keys
commitment_mask_key_idandencryption_keycontain sensitive cryptographic material. While the key manager may handle cleanup internally, consider implementing explicit cleanup or ensuring the key manager provides guarantees about secure deletion of imported temporary keys.This is a known security concern that was previously identified. Consider implementing explicit cleanup patterns for sensitive cryptographic material.
🧹 Nitpick comments (3)
base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs (2)
94-98: Payment ID override behavior needs clearer documentation.The logic that overrides the payment ID when the destination address contains payment ID features could lead to unexpected behavior. Consider adding a warning log or more explicit documentation about when and why this override occurs.
// let override the payment_id if the address says we should if dest_address.features().contains(TariAddressFeatures::PAYMENT_ID) { - debug!(target: LOG_TARGET, "Address contains memo, overriding memo {} with {:?}", payment_id, dest_address.get_payment_id_user_data_bytes()); + warn!(target: LOG_TARGET, "Address contains payment ID features, overriding provided payment ID {} with address payment ID {:?}", payment_id, dest_address.get_payment_id_user_data_bytes()); payment_id = PaymentId::open(dest_address.get_payment_id_user_data_bytes(), TxType::PaymentToOther); }
239-254: Complex key derivation logic in make_key_id_export_safe.The nested key derivation for
TariKeyId::Derivedinvolves converting to string and back, which could be error-prone. The logic creates a derived key with an imported key as the inner key, which is unusual.Consider adding more detailed comments explaining why this specific transformation is necessary:
TariKeyId::Derived { key } => { + // Convert derived key to imported format for secure export + // This ensures the key can be safely transmitted without exposing derivation paths let inner_key = TariKeyId::from_str(key.to_string().as_str())?;base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs (1)
168-168: Consider reducing method complexity.The
sign_messagemethod is marked with#[allow(clippy::too_many_lines)]and performs many complex operations. While the functionality is correct, consider breaking it into smaller focused methods for better maintainability.Consider extracting logical segments like:
- Shared secret and key derivation (lines 174-196)
- Output creation and signing (lines 210-223)
- Signature aggregation (lines 230-277)
This would improve readability and make testing easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
base_layer/wallet/src/transaction_service/handle.rs(5 hunks)base_layer/wallet/src/transaction_service/offline_signing/marshal_output_pair.rs(1 hunks)base_layer/wallet/src/transaction_service/offline_signing/mod.rs(1 hunks)base_layer/wallet/src/transaction_service/offline_signing/models.rs(1 hunks)base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs(1 hunks)base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs(1 hunks)base_layer/wallet/src/transaction_service/service.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- base_layer/wallet/src/transaction_service/offline_signing/mod.rs
- base_layer/wallet/src/transaction_service/offline_signing/marshal_output_pair.rs
- base_layer/wallet/src/transaction_service/offline_signing/models.rs
- base_layer/wallet/src/transaction_service/service.rs
- base_layer/wallet/src/transaction_service/handle.rs
🧰 Additional context used
🧠 Learnings (1)
base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs (1)
Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
🔇 Additional comments (4)
base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs (1)
182-186: Potential relation to funds encumbrance issue mentioned in PR objectives.The
confirm_pending_transactioncall at the end of preparation might be related to the funds encumbrance issue mentioned in the PR objectives, where callingprepare-one-sided-transaction-for-signingtwice fails because funds remain locked. Verify this doesn't prevent subsequent transaction preparations when the first transaction isn't broadcast.#!/bin/bash # Search for other usages of confirm_pending_transaction to understand the encumbrance behavior rg -A 5 -B 5 "confirm_pending_transaction"base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs (3)
204-208: Stealth address enforcement improves privacy.The code now always enforces stealth addresses by deriving a script spending key and using it in the push_pubkey_script. This is a good privacy enhancement that prevents address reuse and improves transaction unlinkability.
362-362: Script keys collection is correct by design.Based on retrieved learnings, the script_keys vector correctly includes only input script keys (
input.output_pair.output.script_key_id). This is by design for the one-sided transaction protocol and should not include output, change, or recipient script keys.
465-497: Change output payment ID locking mechanism.The
lock_sent_output_in_payment_idmethod properly updates the change output to include the sent output hash in the payment ID, then re-encrypts the recovery data. This ensures the change output is linked to the sent transaction for recovery purposes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs (3)
94-94: Fix grammatical error in comment.The comment contains a grammatical error.
- // let override the payment_id if the address says we should + // Let's override the payment_id if the address says we should
209-266: Consider extracting key conversion logic to a separate utility.The
make_key_id_export_safemethod is quite long and complex. Consider extracting this to a separate utility module or trait to improve testability and reusability across the codebase.
238-238: Avoid unnecessary string conversion.The conversion
key.to_string().as_str()is redundant whenTariKeyIdlikely implementsFromStrdirectly.- let inner_key = TariKeyId::from_str(key.to_string().as_str())?; + let inner_key = TariKeyId::from_str(&key.to_string())?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
applications/minotari_console_wallet/src/lib.rs(3 hunks)base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- applications/minotari_console_wallet/src/lib.rs
🔇 Additional comments (3)
base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs (3)
195-207: LGTM: Clean transaction signing implementation.The signing method is well-structured with appropriate error handling and clear separation of concerns by delegating to the
OneSidedSigner.
115-115: Clarify the use of default public key script.Using
Default::default()for the public key in the script may not be intentional. Consider adding a comment explaining why a default/zero public key is appropriate here, or verify this is the correct approach for one-sided transactions.#!/bin/bash # Description: Check how push_pubkey_script with default key is used elsewhere in the codebase # Expected: Find similar usage patterns or documentation explaining this approach rg -A 3 -B 3 "push_pubkey_script.*Default::default"
182-186: Address potential funds encumbrance issue.Based on PR feedback, calling
prepare_one_sided_transaction_for_signingmultiple times fails because funds become encumbered with no way to release them. Theconfirm_pending_transactioncall here might be contributing to this issue. Consider if there should be a mechanism to release encumbered funds when a transaction preparation is abandoned.#!/bin/bash # Description: Check for methods to release or cancel pending transactions # Expected: Find methods that can unencumber funds from failed/abandoned preparations rg -A 5 "cancel.*transaction|release.*encumber|unencumber" --type rust
base_layer/wallet/src/transaction_service/offline_signing/offline_signer.rs
Show resolved
Hide resolved
* development: chore: fix regression bug (tari-project#7276) feat!: expand gRPC readiness status to contain current processed block info (tari-project#7262) fix!: payref migration and indexes, add grpc query via output hash (tari-project#7266) feat: auto zero value coinbase reward calculation (tari-project#7259) feat!: improve grpc token supply (tari-project#7261) chore: new release v4.7.0-pre.0 (tari-project#7268) fix: get_all_completed_transactions limit issues (tari-project#7267) fix: ledger builds (tari-project#7260) feat: offline signing (tari-project#7122) test: verify accumulated difficulty (tari-project#7243)
3 participants
Description
Adds the following features.
CLI
--skip-recoveryoption.Cold wallets need to run in environments without Internet connection. Console wallet created from seed words will require full initial recovery to proceed with other functionality.
This switch allows to skip the recovery step.
"prepare-one-sided-transaction-for-signing" CLI command
Supposed to be run on hot side. This will create an unsigned transaction request, which needs to be signed on cold side.
"sign-one-sided-transaction" CLI command
Supposed to be run on cold side. Signs the transaction using private spend key.
"broadcast-signed-one-sided-transaction" CLI command
Supposed to be run on hot side. Broadcasts the signed transaction to Tari network (mempool)
GRPC methods to be run on hot side
Motivation and Context
Exchanges are requesting a way to have offline signing.
Basically, they want two wallets:
The signing process would look like follows.
How Has This Been Tested?
Created hot and cold wallets as described in this document.
Created an unsigned transaction on hot wallet.
What process can a PR reviewer use to test or verify this change?
Reviewer can follow steps I used to test it above.
Breaking Changes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores