fix: ledger builds #6817
fix: ledger builds #6817
Conversation
WalkthroughThis pull request introduces numerous configuration and code refactorings across the project. The CI workflow upgrades the ledger app builder Docker image, and various modules update error handling, dependency versions, and static initialization by replacing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WalletHandler as Wallet Handler
participant PCFactory as PedersenCommitmentFactory
participant CPKSignature as CommitmentAndPublicKeySignature
User->>WalletHandler: Request script signature
WalletHandler->>PCFactory: Generate commitment (commit)
PCFactory-->>WalletHandler: Return commitment
WalletHandler->>CPKSignature: Sign using commitment and secret keys (sign)
CPKSignature-->>WalletHandler: Return signature
WalletHandler-->>User: Deliver script signature
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (17)
applications/minotari_console_wallet/src/automation/error.rs (1)
98-98: Change from direct error type to string representationThis changes the
FailedSignaturevariant from directly containing aSchnorrSignatureErrorto holding a string representation instead.This approach decouples the direct dependency on the
SchnorrSignatureErrortype, which is good for maintainability if the upstream error type changes. However, keep in mind that converting to a string may lose some structured error context that could be useful for debugging.applications/minotari_ledger_wallet/wallet/src/handlers/get_one_sided_metadata_signature.rs (1)
193-220: Reduced error detail
Replacing the original error-specific message with"Invalid challenge"unifies the user-facing output but may hamper debugging for invalid challenges. Consider logging or capturing detailed error info behind the scenes without exposing sensitive data.applications/minotari_ledger_wallet/wallet/src/handlers/get_schnorr_signature.rs (3)
37-42: Error message for invalid data length
Providing a uniform message is user-friendly, but consider including expected vs. actual lengths to ease debugging.
75-89: Generic error message
Simplifying to"Invalid Challange"standardizes user feedback but removes context. Balance user-facing security with developer needs by logging or capturing the underlying cause.
135-148: Consistent signing error message
Similar to the prior metadata signature handler, returning"Invalid Challange"unifies behavior. For debugging, consider selectively logging the original error.applications/minotari_ledger_wallet/wallet/src/tari_crypto/hashing.rs (2)
9-50:DomainSeparationtrait
Provides a clear, reusable approach to domain-tagging, including versioning. Encouraging thorough naming conventions for domain strings keeps collision risks low.
68-112:DomainSeparatedHasher
Neatly encapsulates domain separation logic by prefixing lengths and domain tags. This fosters safer hashing usage across the codebase.applications/minotari_ledger_wallet/wallet/src/utils.rs (2)
30-35: New hash domains appear consistent.
Defining custom domains forLedgerHashDomainandKeyManagerTransactionsHashDomainis a good practice. Consider adding unit tests to confirm domain label correctness and test collisions.
210-210: Clarify the error message.
The partial string “Err: raw key >>...” might confuse users if the context is insufficient. Consider making the message more descriptive and consistent with other error outputs.- msg.push_str("Err: raw key >>..."); + msg.push_str("Error deriving raw key from path: ");applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs (1)
198-198: Consider caching the pre-initialized PedersenCommitmentFactory.
Creating the factory on each call is acceptable, but caching it might slightly improve performance if this path is called frequently.applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_factory.rs (1)
43-52: Multiscalar multiplication code duplication.
Both theifandelsebranches perform the samemultiscalar_muloperations. You can unify them to simplify the code.- let c = if (self.G, self.H) == (RISTRETTO_PEDERSEN_G, ristretto_pedersen_h()) { - RistrettoPoint::multiscalar_mul(&[v.0, k.0], &[self.H, self.G]) - } else { - RistrettoPoint::multiscalar_mul(&[v.0, k.0], &[self.H, self.G]) - }; + let c = RistrettoPoint::multiscalar_mul(&[v.0, k.0], &[self.H, self.G]);applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_and_public_key_signature.rs (4)
26-33: Consider secure handling of ephemeral secrets.
Storingu_a,u_x, andu_yinside a public struct may be acceptable for ephemeral usage, but ensure these values are handled securely (e.g., by zeroizing them after use if necessary).
147-225: Convenient additive composition for signatures.
Implementing theAddtrait reduces boilerplate when combining partial signatures. As a minor nit, consider renaming variables likeephemeral_pubkey_sum_sumto something more readable (e.g.,combined_pubkey).
227-237: Defaulting to zeroed fields can be risky.
Returning an all-zero signature is convenient, but it may lead to accidental usage in production code if not clearly documented. Consider making default usage more explicit or warnings more visible.
239-289: Non-constant-time comparisons.
Relying on plaincmpandEqfor secret keys (viaas_bytes()calls) can introduce timing side-channel risks. If this component is used in high-security contexts, consider a constant-time equality check.applications/minotari_ledger_wallet/wallet/src/tari_crypto/schnorr.rs (2)
118-171: Verification logic is straightforward but consider constant-time comparisons.
The checks use standard arithmetic to compare points. Ensure the underlyingRistrettoPublicKeycomparison is constant-time if security demands it.
206-242: Default and ordering may pose subtle security concerns.
A default all-zero signature or direct ordering on secret bytes could be used improperly if not documented carefully. Consider clarifying these risks in the codebase or documentation.
📜 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 (26)
.github/workflows/build_ledger_wallet.yml(1 hunks)applications/minotari_console_wallet/src/automation/error.rs(1 hunks)applications/minotari_ledger_wallet/comms/Cargo.toml(1 hunks)applications/minotari_ledger_wallet/comms/src/accessor_methods.rs(2 hunks)applications/minotari_ledger_wallet/comms/src/error.rs(1 hunks)applications/minotari_ledger_wallet/comms/src/ledger_wallet.rs(2 hunks)applications/minotari_ledger_wallet/wallet/.cargo/config.toml(1 hunks)applications/minotari_ledger_wallet/wallet/Cargo.toml(2 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_dh_shared_secret.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_one_sided_metadata_signature.rs(6 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_public_key.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_public_spend_key.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_schnorr_signature.rs(4 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_script_offset.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs(7 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_view_key.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/hashing.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/main.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_and_public_key_signature.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_factory.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/hashing.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/keys.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/mod.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/schnorr.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/utils.rs(7 hunks)
✅ Files skipped from review due to trivial changes (6)
- applications/minotari_ledger_wallet/comms/src/error.rs
- applications/minotari_ledger_wallet/wallet/src/handlers/get_view_key.rs
- .github/workflows/build_ledger_wallet.yml
- applications/minotari_ledger_wallet/wallet/src/handlers/get_script_offset.rs
- applications/minotari_ledger_wallet/wallet/src/tari_crypto/mod.rs
- applications/minotari_ledger_wallet/wallet/src/handlers/get_dh_shared_secret.rs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
- GitHub Check: clippy
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (90)
applications/minotari_ledger_wallet/wallet/src/handlers/get_public_spend_key.rs (2)
9-18: Import reorganization aligns with tari_crypto dependency removal.The imports have been reorganized to accommodate the custom tari_crypto implementation that's been added to the project. The
ByteArraytrait fromtari_utilitiesis now imported directly, while theRistrettoPublicKeyis imported from the localtari_crypto::keysmodule rather than an external dependency.
27-27: Minor formatting improvement.This whitespace addition improves code readability by separating the conditional error handling blocks.
applications/minotari_ledger_wallet/wallet/.cargo/config.toml (1)
9-9: Key dependency change supporting no_std environment.Removing "std" from build-std is a critical change that aligns with the PR objective of making the ledger build compatible with the no_std environment. The ledger build now only includes "core" and "alloc" standard libraries, which matches the requirements for ledger hardware that doesn't support the full standard library.
This change is essential for ensuring the wallet application can run on the ledger hardware without relying on the standard library.
applications/minotari_ledger_wallet/wallet/src/main.rs (2)
10-10: Added custom tari_crypto module to replace external dependency.This line adds the new custom implementation of tari_crypto mentioned in the PR objectives. This is a critical addition that enables the ledger wallet to function in a no_std environment since the external tari_crypto dependency no longer supports no_std.
The addition of this module is essential for the PR's stated goal of creating "a manual implementation of tari crypto" that works in the no_std environment.
210-210: Minor formatting improvement.This whitespace addition improves code readability by separating the variable declaration from the conditional block that follows.
applications/minotari_ledger_wallet/comms/src/ledger_wallet.rs (2)
23-26: Updated imports to use std::sync::LazyLock.The imports have been modernized to use
LazyLockfrom the standard library instead ofLazyfrom an external crate (likely once_cell). This aligns with modern Rust practices and reduces external dependencies.
65-66: Modernized static initialization with LazyLock.The static
HID_MANAGERinitialization has been updated to use the standard library'sLazyLockinstead of a third-party implementation. This is a good modernization that utilizes features available in the standard library.-static HID_MANAGER: Lazy<Mutex<HidManager>> = - Lazy::new(|| Mutex::new(HidManager::new().expect("Failed to initialize HidManager"))); +static HID_MANAGER: LazyLock<Mutex<HidManager>> = + LazyLock::new(|| Mutex::new(HidManager::new().expect("Failed to initialize HidManager")));applications/minotari_console_wallet/src/automation/error.rs (1)
103-107: Manual implementation of From trait replaces automatic conversionThis implementation manually converts
SchnorrSignatureErrortoCommandError::FailedSignatureusingto_string().The manual implementation preserves the behavior that was previously handled by the
#[from]attribute, maintaining backward compatibility while providing more control over the error conversion process.applications/minotari_ledger_wallet/comms/src/accessor_methods.rs (2)
23-23: Update import to use stdlib LazyLockImporting
LazyLockfromstd::syncinstead ofLazyfromonce_cell::sync.Using the standard library implementation reduces external dependencies, which is a good practice as it minimizes dependency maintenance and potential compatibility issues.
59-59: Replace once_cell::Lazy with std::sync::LazyLockThe static variable initialization now uses
LazyLockfrom the standard library instead ofLazyfrom the once_cell crate.
LazyLock(available since Rust 1.70) provides equivalent functionality toonce_cell::Lazy, ensuring thread-safe, one-time initialization of the mutex. This change aligns with the dependency updates in the Cargo.toml file where the once_cell dependency has been removed.applications/minotari_ledger_wallet/comms/Cargo.toml (2)
19-20:✅ Verification successful
Update ledger-rs dependency revisions
The Git revision for ledger-transport and ledger-transport-hid dependencies has been updated.
The revision has been updated from
20e2a20to4ed4cfdef0ae8b40e8997d849a3262bcf00c7d3c. Verify that this newer version is stable and compatible with the rest of the codebase, especially since this PR aims to fix ledger builds.
🏁 Script executed:
#!/bin/bash # Fetch information about the Git commit that's being referenced curl -s -H "Accept: application/vnd.github.v3+json" \ "https://api.github.com/repos/Zondax/ledger-rs/commits/4ed4cfdef0ae8b40e8997d849a3262bcf00c7d3c" | \ jq '{message: .commit.message, date: .commit.author.date, author: .commit.author.name}'Length of output: 336
Ledger-rs Dependency Revision Update Verified
The dependencies in
applications/minotari_ledger_wallet/comms/Cargo.tomlhave been updated to commit4ed4cfdef0ae8b40e8997d849a3262bcf00c7d3cfor bothledger-transportandledger-transport-hid. The commit (authored by Juan Leni on 2025-01-02 with the message "Update FUNDING.json") has been fetched and verified. There are no immediate indications of issues with stability or compatibility based on the commit details. Please ensure that the updated revision continues to resolve the ledger build issues by running the necessary integration tests.
13-13:✅ Verification successful
Enable default features for tari_crypto
The
default-features = falseoption has been removed from the tari_crypto dependency.This change enables all default features of the tari_crypto crate. Verify that this doesn't introduce any unnecessary dependencies or functionality that could affect performance or compatibility, especially since the PR is addressing ledger builds.
🏁 Script executed:
#!/bin/bash # Check the default features of tari_crypto and whether they might impact the ledger build rg -A 10 '\[features\]' | grep -B 20 'default'Length of output: 7063
Verified: Default features for tari_crypto are enabled without introducing unwanted dependencies or impacting ledger builds.
The change in
applications/minotari_ledger_wallet/comms/Cargo.toml—removingdefault-features = falseso that default features are enabled—is consistent with similar patterns seen throughout the codebase. No evidence was found that the default features of tari_crypto introduce unnecessary dependencies or functionality that could affect performance or ledger compatibility.
- Action Required: No further changes needed. Please ensure that any future dependency or build issues related to ledger builds be re-evaluated if default features change in tari_crypto.
applications/minotari_ledger_wallet/wallet/src/handlers/get_public_key.rs (2)
20-20: Added blank line for better code formattingA blank line has been added after the error handling block for better code readability.
This small formatting change improves code readability by visually separating the conditional blocks.
9-11:❓ Verification inconclusive
Update import paths for cryptographic types
The imports have been reorganized:
ByteArrayis now imported directly fromtari_utilitiesandRistrettoPublicKeyis imported from a local implementation incrate::tari_crypto::keys.This change suggests a reorganization of cryptographic functionality, potentially to implement a custom version of the
RistrettoPublicKeytype. Verify that the local implementation maintains the same behavior as the original from the externaltari_cryptocrate.
🏁 Script executed:
#!/bin/bash # Check the implementation of the local RistrettoPublicKey fd -t f "tari_crypto.*keys\.rs"Length of output: 33
Attention: Manual Verification of Local
RistrettoPublicKeyImplementationThe updated import paths now pull
RistrettoPublicKeyfrom the local module (crate::tari_crypto::keys) instead of the external crate. However, our initial file search did not locate a file matching the expected pattern (e.g., a file liketari_crypto/keys.rs).Please verify manually that the local implementation:
- Exists in the expected location: Confirm that the module defining
RistrettoPublicKeyis present (it might be in a different file or structured as an inline module inmod.rs).- Maintains expected behavior: Ensure that the local version replicates the functionality of the external
tari_cryptocrate’s implementation.Once you confirm these points, we can fully approve the change.
applications/minotari_ledger_wallet/wallet/src/hashing.rs (4)
1-1: Updated copyright year
The shift from 2024 to 2025 for copyright is appropriate for reflecting the current year.
10-10: Local crate reference
Switching touse crate::tari_crypto::hashing::DomainSeparation;clarifies module ownership and scoping within this project.
14-14: Selective import
Introducingdigest::Outputaligns seamlessly with the new domain-separated hash output pattern.
78-93: AddedDomainSeparatedHash
Encapsulating the final hash output within a dedicated struct improves clarity and prevents accidental misuse of raw output bytes. Consider adding unit tests for edge cases and integration tests for multi-domain scenarios.applications/minotari_ledger_wallet/wallet/src/handlers/get_one_sided_metadata_signature.rs (3)
27-27: Added import forByteArray
This import indicates usage of byte-based utilities for key or signature conversion. The approach is consistent with typical cryptographic workflows.
33-47: Refactored imports for new cryptographic structures
Consolidating references (pedersen commitments, new signature structs, domain-separated hasher, and key utilities) simplifies code organization and ensures consistent cryptographic operations.
155-155:PedersenCommitmentFactory::default()usage
Usingdefault()for the commitment factory is idiomatic. Confirm that any specialized parameters needed for specific network settings are addressed elsewhere.applications/minotari_ledger_wallet/wallet/src/handlers/get_schnorr_signature.rs (4)
11-11: ImportedByteArray
Enabling straightforward key and signature byte manipulations helps maintain clarity in cryptographic routines.
15-16: Domain-based hashing
Usinghash_domainwithSchnorrSignatureimproves maintainability and security by segmenting signature creation into distinct cryptographic domains.
24-28: New domain & type aliases
hash_domain!(SchnorrSigChallenge, ...)and the type aliases for Schnorr signatures reduce boilerplate and enforce domain constraints.
121-121: Line 121 context
The snippet flags this line but offers limited detail. Verify whether this aligns with the intended key or parameter usage.applications/minotari_ledger_wallet/wallet/src/tari_crypto/hashing.rs (8)
1-5: Core imports for no_std
These imports (alloc,core,digest) are typical building blocks for cryptographic functionality without relying on std. Looks good.
52-67:byte_to_decimal_ascii_bytes
Implementation correctly converts a byte to a decimal string. Testing edge values like 0 and 255 ensures correctness in all scenarios.
114-120: Eq/PartialEq
Operators only check thelabel. This is fine if internal digest state or partial hashing is irrelevant. Otherwise, confirm domain collision is impossible.
122-132: DelegatedUpdate
Deferring updates to the inner digest is straightforward. No further remarks needed.
136-146:FixedOutput
Implementation ensures finalization integrates with domain separation logic. This consistency is important for cryptographic correctness.
148-200:impl Digest
Seamlessly integrates domain-tagging features throughout the digest lifecycle, including reset. This fully enforces domain separation.
202-214:MacDomain
Provides a dedicated domain for MAC derivation. Keep version increments aligned with major domain or format changes.
216-238:hash_domain!macro
Automates domain separation struct creation, reducing boilerplate. Validate domain uniqueness to prevent accidental overlaps.applications/minotari_ledger_wallet/wallet/src/utils.rs (5)
18-18: Import addition looks good.
No issues spotted with importingByteArray; it's consistently used in the file.
23-24: Domain separation macros are clear and well-structured.
The introduction ofhash_domainusage here is consistent with the rest of the codebase’s domain-separated hashing approach.
158-159: Ensure bip32_derive arguments align with intended usage.
Switching toNonefor the last argument inbip32_derivelooks correct. However, verify that omitting the chain code or child key metadata aligns with all downstream usage and does not introduce regressions.
167-169: Error handling is fine. Check for sensitive data in display.
Usingcx_error_to_string(e)for user-facing messages is acceptable, but confirm that the error string does not expose sensitive internal data.
331-332: TransactionHashDomain addition is consistent.
The newly added domain separation macro for transactions aligns well with existing naming conventions.applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs (3)
17-22: Refined imports match new commitment and signature logic.
You have switched toPedersenCommitmentFactoryandCommitmentAndPublicKeySignature, which aligns with the revised cryptographic approach.
181-181: Updated return type requires downstream verification.
Changing the function signature to returnCommitmentAndPublicKeySignaturemay break existing call sites dependent on the old type. Ensure that all references in the codebase are updated and tested.
213-220: Verify domain separation consistency in signature generation.
CommitmentAndPublicKeySignature::signuses the provided challenge. Confirm all inputs are domain-separated and tested thoroughly, especially for ledger-based signing.applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_factory.rs (3)
17-20: Constants defined for Ristretto-based Pedersen commitments.
TARI_HandRISTRETTO_PEDERSEN_Gare clearly documented. Just ensure they correspond to the correct generator points expected in your system.
30-35: Factory constructor is straightforward.
Usage ofnewto setGandHis consistent. Keep in mind that if you allow customGorH, it may need thorough testing to avoid invalid points.
74-76: Decompression panic handling.
Callingexpect("Failed to decompress TARI_H")can crash. Consider explicit error handling to surface a custom error ifdecompress()fails.applications/minotari_ledger_wallet/wallet/src/tari_crypto/keys.rs (19)
1-25: File Header and Imports Look GoodNo issues found with licensing, module documentation, and import statements. The usage of
alloc,core, and cryptographic crates (e.g.,blake2,curve25519_dalek,subtle) appears consistent with a no_std environment.
26-53: Macro for Add Variants is Well-StructuredThe
define_add_variants!macro cleanly handles ownership and borrowing for variousAddoperations, ensuring consistency across types and references. This approach reduces repetitive code and helps avoid mistakes in trait implementation.
54-82: Macro for Sub Variants Mirrors Add Logic CorrectlyLikewise, the
define_sub_variants!macro parallels the additions macro. The code is clear, with no apparent logical pitfalls. Good job maintaining consistency.
83-111: Macro for Mul Variants is ConsistentSimilar to the Add/Sub macros, the
define_mul_variants!macro ensures that multiplication operators handle references as intended. The patterns look consistent and correct for cryptographic usage.
112-131: RistrettoSecretKey Struct and Borsh SerializationThe introduction of
RistrettoSecretKeywith#[derive(Clone, Default, Zeroize, ZeroizeOnDrop)]is helpful for memory safety. The Borsh (de)serialization logic properly returns errors on invalid input. This is an excellent pattern to avoid accidentally creating invalid keys.
134-160: ByteArray Implementation Ensures CorrectnessThe
from_canonical_bytesmethod verifies the 32-byte length, usesfrom_canonical_bytes, and returns an error on failure. Zeroizing the intermediate buffer is prudent for security. Overall, this helps prevent subtle key-corruption issues.
162-181: Constant-Time Equality and Hash TraitsUsing
ConstantTimeEqfor equality checks and derivingHash,PartialEq,Eqhelps ensure correctness and avoid timing attacks. This is a standard best practice in cryptographic code.
184-225: RistrettoSecretKey Methods and Debug Output
- The
invert()method returningNoneon zero is a good guard.- The
random()andfrom_uniform_bytes()methods handle generation well, including length checks.- Debug formatting redacts actual key values, which helps avoid accidental secrets leakage in logs.
Overall, this is carefully implemented.
227-232: ConstantTimeEq for RistrettoPublicKey Also GoodApplying
ConstantTimeEqto public keys is consistent with the secret-key approach. Although public keys are usually not as sensitive as secret keys, having consistent timing-safe operations is still a commendable practice.
235-275: Addition, Subtraction, Multiplication on SecretKeyThese operator overloads align with standard cryptographic usage. By returning
RistrettoPublicKeyorRistrettoSecretKeyas appropriate, the code remains type-safe. This shortens code in higher-level features.
279-292: From and Borrow ImplementationsProviding these conversions can help in code that needs to parse small numeric constants as scalars. This seems straightforward and correct.
296-336: RistrettoPublicKey Struct and Core Methods
- Encapsulating the
RistrettoPointand itsCompressedRistrettois a nice approach.from_secret_key()method usesRISTRETTO_BASEPOINT_TABLEconsistently, aligning with typical cryptographic patterns.- The wide-scope usage of references indicates an awareness of memory constraints.
338-373: (De)Serialization, Zeroize, and Hashing
- Borsh (de)serialization ensures consistent wire formats.
- Zeroizing the point and the compressed point is crucial if public keys were to include sensitive ephemeral data.
- Implementing
Hashablewith Blake2b is standard and well-done.
377-429: Default, Display, and Debug Implementations
Defaultreturning the identity point is conventional.- The display logic with width-based truncation is a nice user-friendly approach.
- Hiding sensitive details in
Debugwithto_hex()is also acceptable for public keys.
431-451: Ordering and Equality for PublicKey
- Implementing
PartialEq,Eqviact_eqis correct.- Using compressed bytes for
Ordensures consistent ordering.
No issues identified.
453-477: ByteArray for RistrettoPublicKey
- Validates length as 32, preventing panics from the underlying library.
- Decodes with
CompressedRistretto, returning an error if invalid.as_bytes()references the compressed representation for consistent usage.
481-507: Add, Sub, and Mul Implementations for PublicKeyOverloading these traits is convenient for cryptographic arithmetic. The usage of references to minimize copying is a plus.
508-536: Additional Multiplication Variants for SecretKey PairsAllowing
RistrettoSecretKey * RistrettoSecretKey -> RistrettoSecretKeycan be valuable in advanced proof systems. Looks consistent and type-safe.
540-556: Conversions From Key Structs
- Conversions from
RistrettoSecretKeytoScalar, andRistrettoPublicKeytoRistrettoPoint, are straightforward.- They facilitate interoperability with the underlying dalek library.
No concerns identified.applications/minotari_ledger_wallet/wallet/Cargo.toml (5)
11-11: New Dependency: tari_utilitiesAdding
tari_utilities("0.8") withdefault-features = falseappears intentional for no_std or minimal usage. Validate that the pinned version meets your functional and security needs.
16-17: Updated DependenciesUpgrading
include_gifto "1.2" andledger_device_sdkto "1.21" should be confirmed for compatibility. Check for any known regressions or breaking changes in these versions.
20-21: Introducing curve25519-dalek and subtleThe pinned versions for
curve25519-dalekandsubtlewithdefault-features = falseare in line with cryptographic best practices. Ensure the additional features (alloc,rand_core,precomputed-tables,zeroize) are indeed required in your build environment.
24-24: Clearing the ignored list
ignored = []indicates that no crates are currently excluded from cargo-machete checks. This is fine, but ensure that newly introduced crates (likesubtle) are tested thoroughly.
56-57: New Lint ConfigurationAdding
[lints.rust]withunexpected_cfgsat level = "warn" is a useful step to catch unknown target OS configurations. This can help ensure minimal environment-based surprises.applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment.rs (12)
1-15: PedersenCommitment Struct Introduction
- The struct
PedersenCommitmentwraps aRistrettoPublicKeywith a default.- This is a common pattern for hierarchical cryptographic structures.
No immediate issues spotted.
17-27: Public Key Conversion Routines
from_public_keyis straightforward, simply cloning the key. This is a typical pattern if you need a typed “commitment” wrapper. Implementation is fine.
29-38: ByteArray for PedersenCommitment
- Validates canonical bytes via
RistrettoPublicKey::from_canonical_bytes.as_bytes()delegates to the inner public key.
Implementation looks consistent with existing patterns.
40-51: Ordering Implementations
PartialOrdandOrdrely on theRistrettoPublicKeyordering (compressed byte comparison). This is correct for consistent sorting, though actual numeric ordering in an elliptic curve context is mostly for housekeeping.
53-60: Add for PedersenCommitmentThe comment about not checking that the bases match is apt—this is typical in Pedersen commitments, but it’s good that the code explicitly warns about it.
62-69: Add for PedersenCommitmentAllows combining a public key with a known commitment. Also doesn’t check base equality, which is expected per your comment.
71-78: Sub for PedersenCommitmentAs with
Add, no base checks are performed. The code is minimal and consistent.
80-88: Mul for PedersenCommitmentMultiplication by a secret key is a typical cryptographic operation. Delegating the multiplication to
rhs * &self.0is consistent with theRistrettoPublicKeyoperator overloads. No issues found.
89-93: Hash ImplementationUses the byte representation. This is symmetrical with how RistrettoPublicKey hashing is done. Looks good.
95-102: Equality CheckingReferences the underlying public key equality. Straightforward, with no side-channel concerns for commitments specifically.
103-109: Borsh DeserializationDeserializes an inner
RistrettoPublicKey. Error handling is consistent with other code in this crate.
110-114: Borsh SerializationDelegates to the inner key’s serialization. Maintains consistency in wire format.
No concerns identified here.applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_and_public_key_signature.rs (4)
20-24: Solid error handling for invalid challenges.
The custom error enum effectively captures invalid challenge scenarios, making the code self-documenting and robust.
35-51: Constructor is straightforward and clear.
Thenewmethod neatly encapsulates the signature components with minimal overhead.
53-89: Robust signature creation.
Good job validating the challenge for non-zero values and catching errors fromfrom_uniform_bytes. This ensures safer cryptographic behavior.
91-145: Exposed getters might risk sensitive data leaks.
While these accessors are convenient, be mindful that returning raw secret keys (u_a,u_x,u_y) can expose sensitive data. Confirm that this is intended.applications/minotari_ledger_wallet/wallet/src/tari_crypto/schnorr.rs (6)
27-32: Clear error definition.
TheSchnorrSignatureErrorenum helps communicate invalid challenge states, making error handling more explicit.
35-40: Effective use of PhantomData for domain separation.
Using a generic type parameter for domain separation is a clean approach. It keeps your signatures context-aware.
59-73: Uniform challenge parsing looks correct.
Thesign_raw_uniformmethod correctly handlesfrom_uniform_bytesand gracefully returns an error on invalid data. Verify if a zerosecretkey scenario needs explicit handling.
75-89: Canonical challenge parsing is well-handled.
The approach here parallels the uniform variant while ensuring canonical byte usage. Good consistency.
91-116: Well-structured signing with domain separation.
signandsign_with_nonce_and_messageelegantly incorporate domain-separated hashing, which improves signature uniqueness and security.
182-204: Add trait facilitates partial signature combining.
This feature is beneficial for multi-party signing flows. The concise implementation is appreciated.
applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs
Outdated
Show resolved
Hide resolved
Test Results (CI) 3 files 129 suites 36m 23s ⏱️ Results for commit 0d0666c. ♻️ This comment has been updated with latest results. |
Test Results (Integration tests)18 tests 18 ✅ 7m 0s ⏱️ For more details on these parsing errors, see this check. Results for commit 0d0666c. ♻️ This comment has been updated with latest results. |
…ript_signature.rs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
SWvheerden
force-pushed
the
sw_fix_ledger
branch
from
014c37b to
0d0666c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
applications/minotari_ledger_wallet/wallet/src/utils.rs (4)
18-18: Simplify import usage if not widely used.You are importing
ByteArray, but it’s only referenced in a few places. Confirm that it’s essential for your logic or consider using a more localized import to reduce global namespace clutter.
167-169: Consistent error handling approach.Forwarding the error via
cx_error_to_stringaligns with your existing pattern. However, consider including additional context if repeated errors become too generic.
210-210: Ensure message is conveyed clearly.The string
"Err: raw key >>..."is short. If you want the user to debug further, consider clarifying the possible cause in the message.
273-273: UI feedback string logic.Similar to the above messages, ensure that relevant error details are included for quick troubleshooting on device screens.
applications/minotari_ledger_wallet/wallet/Cargo.toml (1)
20-20: Sanity-check new curve library.Adding
curve25519-dalek = "4"with features[ "alloc", "rand_core", "precomputed-tables", "zeroize"]is typical for advanced cryptography. Confirm no performance regressions on constrained devices.applications/minotari_ledger_wallet/wallet/src/tari_crypto/keys.rs (4)
83-111: Multiplication operator expansions.Expanding
_mul_variantsensures consistent coverage. This is generally good, though an alternative could be generic macros if you foresee further expansions for other operators.
134-161: Implementing ByteArray.Well-structured approach for canonical byte arrays. The zeroization of copies is a good security practice. Just ensure that every error path is tested.
296-301: RistrettoPublicKey struct encapsulation.Storing both
pointand itscompressedversion can be convenient for performance. Just be mindful to keep them in sync if you consider lazy-compression approaches.
454-477: Public key ByteArray implementation.Returning an error on invalid length is correct, but consider logging or returning domain-specific error codes for better debugging if needed.
applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_factory.rs (2)
45-50: Remove the redundant conditional check to simplify the code.The
ifblock at lines 46–49 is effectively performing the same multiscalar multiplication in both branches. Consider removing theifstatement altogether to streamline logic:Apply this diff to remove the redundant conditional:
- let c = if (self.G, self.H) == (RISTRETTO_PEDERSEN_G, ristretto_pedersen_h()) { - RistrettoPoint::multiscalar_mul(&[v.0, k.0], &[self.H, self.G]) - } else { - RistrettoPoint::multiscalar_mul(&[v.0, k.0], &[self.H, self.G]) - }; + let c = RistrettoPoint::multiscalar_mul(&[v.0, k.0], &[self.H, self.G]);
74-75: Consider handling decompression fails more gracefully.The call to
expect("Failed to decompress TARI_H")will panic ifTARI_Hfails to decompress. WhileTARI_His presumably always valid, it may be safer to handle or bubble up errors more explicitly in production builds.applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment.rs (1)
52-77: Document or guard against mixing commitments with different generators.The addition, subtraction, and public key combination methods do not check that the same Pedersen base points are used, which can lead to incorrect commitments when bases differ. Although this may be intentional, please add clear documentation or guardrails to prevent accidental misuse.
Also applies to: 61-69
applications/minotari_ledger_wallet/wallet/src/tari_crypto/schnorr.rs (1)
91-101: Consider validating that the generated nonce is non-zero.Using a zero ephemeral nonce can degrade security. While extremely unlikely, adding a safeguard ensures the resulting public nonce never becomes the identity point. For additional safety, fail early or regenerate when a zero key is produced.
📜 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 (26)
.github/workflows/build_ledger_wallet.yml(1 hunks)applications/minotari_console_wallet/src/automation/error.rs(1 hunks)applications/minotari_ledger_wallet/comms/Cargo.toml(1 hunks)applications/minotari_ledger_wallet/comms/src/accessor_methods.rs(2 hunks)applications/minotari_ledger_wallet/comms/src/error.rs(1 hunks)applications/minotari_ledger_wallet/comms/src/ledger_wallet.rs(2 hunks)applications/minotari_ledger_wallet/wallet/.cargo/config.toml(1 hunks)applications/minotari_ledger_wallet/wallet/Cargo.toml(2 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_dh_shared_secret.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_one_sided_metadata_signature.rs(6 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_public_key.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_public_spend_key.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_schnorr_signature.rs(4 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_script_offset.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs(7 hunks)applications/minotari_ledger_wallet/wallet/src/handlers/get_view_key.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/hashing.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/main.rs(2 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_and_public_key_signature.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_factory.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/hashing.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/keys.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/mod.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/tari_crypto/schnorr.rs(1 hunks)applications/minotari_ledger_wallet/wallet/src/utils.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- applications/minotari_ledger_wallet/comms/src/error.rs
- applications/minotari_ledger_wallet/wallet/.cargo/config.toml
- applications/minotari_ledger_wallet/comms/src/ledger_wallet.rs
- applications/minotari_ledger_wallet/wallet/src/handlers/get_script_offset.rs
- applications/minotari_ledger_wallet/wallet/src/handlers/get_view_key.rs
- applications/minotari_ledger_wallet/wallet/src/main.rs
- applications/minotari_ledger_wallet/wallet/src/handlers/get_public_spend_key.rs
- applications/minotari_ledger_wallet/comms/src/accessor_methods.rs
- applications/minotari_ledger_wallet/wallet/src/handlers/get_dh_shared_secret.rs
- applications/minotari_ledger_wallet/comms/Cargo.toml
- applications/minotari_ledger_wallet/wallet/src/handlers/get_public_key.rs
- applications/minotari_console_wallet/src/automation/error.rs
- applications/minotari_ledger_wallet/wallet/src/handlers/get_schnorr_signature.rs
- .github/workflows/build_ledger_wallet.yml
- applications/minotari_ledger_wallet/wallet/src/tari_crypto/mod.rs
- applications/minotari_ledger_wallet/wallet/src/tari_crypto/commitment_and_public_key_signature.rs
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: Cucumber tests / FFI
- GitHub Check: clippy
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (33)
applications/minotari_ledger_wallet/wallet/src/hashing.rs (4)
1-1: Copyright year updated to 2025.The file has been updated with the latest copyright year.
10-10: Import path updated to use local tari_crypto module.The import for
DomainSeparationnow uses the crate's local implementation fromtari_cryptomodule instead of an external dependency.
14-15: Added necessary import for Output type.The
digest::Outputimport supports the new domain-separated hash structure.
79-93: Well-designed domain-separated hash implementation.The new
DomainSeparatedHashstruct provides a clean wrapper for hash outputs with domain separation, along with a constructor andAsRef<[u8]>implementation for easier handling in cryptographic contexts.This implementation enhances the security of cryptographic operations by providing proper domain separation, which helps prevent hash collision attacks across different contexts.
applications/minotari_ledger_wallet/wallet/src/handlers/get_one_sided_metadata_signature.rs (3)
27-47: Updated imports with reorganized tari_crypto module structure.The imports now reflect the reorganized code structure with consolidated crypto functionality in the
tari_cryptomodule. This includes specific imports for commitment, commitment signatures, commitment factory, key types, and utility functions.
155-155: Simplified commitment factory usage.Replaced
ExtendedPedersenCommitmentFactorywithPedersenCommitmentFactory, which aligns with the PR's objective of creating a manual implementation of tari crypto for no_std environments.
193-220: Updated signature generation and error handling.The signature generation now uses
CommitmentAndPublicKeySignature::signinstead ofRistrettoComAndPubSig::sign, with standardized error handling that uses a static error message "Invalid challenge" instead of capturing specific error details.While this change standardizes error output, it does trade off detailed error information that might be useful for debugging. This appears to be a deliberate choice to provide consistent user-facing messages without leaking implementation details.
applications/minotari_ledger_wallet/wallet/src/tari_crypto/hashing.rs (6)
11-53: Well-designed DomainSeparation trait.The
DomainSeparationtrait provides a robust foundation for domain-separated cryptographic hashing, with methods for version numbers, domain labels, and tag generation. The implementation ensures proper formatting of domain separation tags based on version and label information.
55-70: Correct low-level byte manipulation for version formatting.The
byte_to_decimal_ascii_bytesfunction correctly handles the conversion of byte values to their ASCII decimal representation for use in domain separation tags.
71-93: Clean domain-separated hasher implementation.The
DomainSeparatedHasherstruct provides a well-structured implementation for domain-separated hashing with proper initialization and label handling.
147-203: Comprehensive Digest trait implementation.The implementation of the
Digesttrait provides all necessary methods for the hasher to be used with standard cryptographic libraries. The implementation correctly manages domain separation tags during reset operations.
205-217: MAC domain implementation for cryptographic message authentication.The
MacDomainstruct provides a specific implementation ofDomainSeparationfor MAC algorithms with appropriate version and domain values.
219-240: Useful macro for creating domain separation structs.The
hash_domain!macro simplifies the creation of domain separation structs with specified domains and versions, following the DRY principle and making it easier to create consistent domain separation implementations across the codebase.applications/minotari_ledger_wallet/wallet/src/handlers/get_script_signature.rs (4)
17-34: Updated imports to use reorganized tari_crypto module.The imports have been updated to use the new
tari_cryptomodule structure, including specific imports for cryptographic primitives and utility functions.
181-181: Updated return type for get_script_signature function.The return type has been changed to
CommitmentAndPublicKeySignatureto align with the new cryptographic structure in the codebase.
198-198: Simplified commitment factory instantiation.Replaced
ExtendedPedersenCommitmentFactorywithPedersenCommitmentFactory, consistent with the changes in other files.
213-240: Updated signature generation and error handling.The signature generation now uses
CommitmentAndPublicKeySignature::signwith standardized error handling that uses a static error message "Invalid Challenge" instead of capturing specific error details.The previously reported spelling mistake ("Invalid Challange") has been fixed to "Invalid Challenge".
applications/minotari_ledger_wallet/wallet/src/utils.rs (4)
23-24: Validate domain separation usage.You introduced
hash_domainandtari_crypto::{hashing::DomainSeparatedHasher, keys::RistrettoSecretKey}. Verify that the domain and label strings accurately reflect their intended usage to avoid collisions in future expansions.
30-35: Domain names look consistent.Great job defining
LedgerHashDomainandKeyManagerTransactionsHashDomain. These logically convey distinct hashing domains. No further issues spotted.
158-159: Confirm correctness of passingNonefor param.Previously, a placeholder slice might have been used. Ensure that using
Noneis correct with the library’s updated API, so no random or uninitialized memory region is required.
331-332: New domain for transactions.Defining
TransactionHashDomainhere is consistent with your domain-separated hashing strategy. Looks good overall.applications/minotari_ledger_wallet/wallet/Cargo.toml (6)
11-11: Check tari_utilities version alignment.Ensure that "0.8" is compatible with the rest of the project to avoid version conflicts with other crates that may need a different version.
16-16: Confirm the updated version for include_gif.Increasing from "1.0.1" to "1.2" can introduce changes. Verify that any old functionalities used in "1.0.1" are still present or properly migrated.
17-17: Ensure ledger_device_sdk version is stable.The upgrade from "1.15" to "1.21" might contain breaking changes. Double-check that all relevant API calls remain valid.
21-21: Subtle usage.
subtleis commonly used for constant-time comparisons. Ensure you leverage it consistently for vulnerability prevention across all cryptographic operations.
24-24: Recheck the ignore list.
ignored = []no longer excludes any dependencies; confirm that you truly want all dependencies to be checked or built. If you intended to exclude something, re-add it.
55-56: Lints configuration can help catch OS-specific pitfalls.The new
[lints.rust]section is a good addition to warn about unexpected OS targets. Ensure you handle or filter out the warnings if you intentionally target unlisted OS versions.applications/minotari_ledger_wallet/wallet/src/tari_crypto/keys.rs (6)
26-53: Macro approach for add-variants.Your macros reduce boilerplate for overloading operators. This is convenient, but keep in mind that longer expansions can affect compile times. The logic here looks sound.
54-82: Similar operator macro for subtraction.Duplicating the approach from the add macro is consistent. There’s no immediate issue, but remember to maintain these macros if you ever refactor operator logic.
112-114: Encapsulation of RistrettoSecretKey.Marking
Scalaraspub(crate)inside preserves internal control. Good approach to keep external crates from inadvertently mutating or reading the scalar.
117-131: Borsh serialization logic.Encoding and decoding with Borsh for
RistrettoSecretKeyis straightforward. However, consider if partial reads can occur, and ensure you always read exactly 32 bytes when deserializing.
338-352: Borsh for RistrettoPublicKey.Similar to the secret key, the 32-byte check is crucial. The error message is sufficiently descriptive, though you might want optional logging if repeated frequently.
533-536: Secret-key multiplied by secret-key logic.Line 512 already handles
(secret * secret) -> secret. This additional define_mul_variants ensures consistent usage across references. Looks good.
Description
Upgraded ledger to now build again on the latest versions of ledger.
Made a manual implementation of tari crypto as tari crypto does not support no_std anymore
Summary by CodeRabbit
Chores
Refactor
New Features