summa-tx / coins

Rust implementations of BIP32/39 and Ledger device comms
Other
90 stars 31 forks source link

Upgrade Rust Crytpo asymmetric dependencies #96

Closed agostbiro closed 2 years ago

agostbiro commented 2 years ago

I'm trying to upgrade dependencies in ethers-rs and came across a problem where dependency conflicts with coins-bip32 are blocking the upgrade. I've tried to upgrade relevant packages in coins-bip32, but can't seem to get one of the macros right. I'm adding more detail in the attached PR.

Diff in ethers-rs:

diff --git a/ethers-core/Cargo.toml b/ethers-core/Cargo.toml
index 00fa752..ae96486 100644
--- a/ethers-core/Cargo.toml
+++ b/ethers-core/Cargo.toml
@@ -16,9 +16,9 @@ arrayvec = { version = "0.7.2", default-features = false }
 rlp-derive = { version = "0.1.0", default-features = false }
 -elliptic-curve = { version = "0.11.12", default-features = false }
+elliptic-curve = { version = "0.12.1", default-features = false }
-k256 = { version = "0.10.4", default-features = false, features = ["keccak256", "ecdsa", "std"] }
+k256 = { version = "0.11.2", default-features = false, features = ["keccak256", "ecdsa", "std"] }

Conflict in ethers-rs:

error: failed to select a version for `signature`.
    ... required by package `ecdsa v0.13.3`
    ... which satisfies dependency `ecdsa-core = "^0.13"` of package `k256 v0.10.0`
    ... which satisfies dependency `k256 = "^0.10"` of package `coins-bip32 v0.6.0`
    ... which satisfies dependency `coins-bip32 = "^0.6.0"` of package `coins-bip39 v0.6.0`
    ... which satisfies dependency `coins-bip39 = "^0.6.0"` of package `ethers-signers v0.13.0 (/Users/abiro/repos/ethers-rs/ethers-signers)`
    ... which satisfies path dependency `ethers-signers` (locked to 0.13.0) of package `ethers v0.13.0 (/Users/abiro/repos/ethers-rs)`
    ... which satisfies path dependency `ethers` (locked to 0.13.0) of package `ethers-wasm v0.1.0 (/Users/abiro/repos/ethers-rs/examples/ethers-wasm)`
versions that meet the requirements `>=1.3.1, <1.5` are: 1.4.0, 1.3.2, 1.3.1

all possible versions conflict with previously selected packages.

  previously selected package `signature v1.5.0`
    ... which satisfies dependency `signature = "^1.5"` of package `ecdsa v0.14.2`
    ... which satisfies dependency `ecdsa-core = "^0.14"` of package `k256 v0.11.2`
    ... which satisfies dependency `k256 = "^0.11.2"` of package `ethers-core v0.13.0 (/Users/abiro/repos/ethers-rs/ethers-core)`
    ... which satisfies path dependency `ethers-core` (locked to 0.13.0) of package `ethers v0.13.0 (/Users/abiro/repos/ethers-rs)`
    ... which satisfies path dependency `ethers` (locked to 0.13.0) of package `ethers-wasm v0.1.0 (/Users/abiro/repos/ethers-rs/examples/ethers-wasm)`

failed to select a version for `signature` which could resolve this conflict
agostbiro commented 2 years ago

I've managed to upgrade the coins-bip32 crate to k256 = "0.11" and digest = "0.10", but I'm running into trouble upgrading the hashers in coins-core to satisfy the new trait bounds required by DigestSigner in k256 = "0.11". There are two blockers:

1. Potential panic

digest::VariableOutput::finalize_variable now returns a Result, but digest::FixedOutput methods are infallible, which means we have to unwrap the Result and potentially panic.

Original code:

impl digest::FixedOutput for Blake2b256 {
    fn finalize_into(self, out: &mut DigestOutput<Self>) {
        digest::VariableOutput::finalize_variable(self.0, |res| {
            AsMut::<[u8]>::as_mut(out).copy_from_slice(&res[..32])
        });
    }
}

Code with digest = "0.10.3":

impl digest::FixedOutput for Blake2b256 {
    fn finalize_into(self, out: &mut DigestOutput<Self>) {
        digest::VariableOutput::finalize_variable(self.0, out).unwrap();
    }
}

I don't understand well the uses of Blake2b256 in the project, but I'm wondering if it'd be possible to use a fixed output variant as the inner hasher which would allow sidestepping this issue.

2. Unsatisfied trait bounds

DigestSigner now requires the digest::core_api::CoreProxy, which in turn requires the private digest::core_api::wrapper::sealed::Sealed trait. I've opened an issue in the digest repo to resolve this.

agostbiro commented 2 years ago

Thanks for #101 @mattsse ! I'm a little bit concerned about these lines:

https://github.com/summa-tx/bitcoins-rs/blob/a1739f867e3fd6bef9cb170d6ed1e041a719db7c/core/src/hashes/mod.rs#L218

https://github.com/summa-tx/bitcoins-rs/blob/a1739f867e3fd6bef9cb170d6ed1e041a719db7c/core/src/hashes/mod.rs#L228

If I'm not mistaken a potential error is being ignored there as digest::VariableOutput::finalize_variable now returns a Result.

--

Btw we've relaxed the DigestSigner bounds in the ~digest~ k256 crate in-between so there is potential for simplification in the hash implementations in the future:

https://github.com/RustCrypto/elliptic-curves/pull/613/files

prestwich commented 2 years ago

According to the docs for variable output, that method errors if the buffer is of incorrect size, which we know not to be the case

agostbiro commented 2 years ago

Btw I've noticed that the DigestSigner update has been released in k256 = 0.11.3 and the PR is actually using that version already, so I'm closing this issue.

mattsse commented 2 years ago

noticed I didn't delete the old impl docs... used expect rather than silently dropping the result https://github.com/summa-tx/bitcoins-rs/pull/103