search

summa-tx / coins

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

ledger: panic + poisoned mutex after new updated #128

Closed naps62 closed 10 months ago

naps62 commented 10 months ago

My app originally relied on my proposal (#125 ), which was closed in favor of a larger rewrite in the latest version.

I'm trying to run my app using this rewrite (by overriding the dependency to force ethers-rs to use it. since it's a minor version bump I assume no intended breaking changes?)

But any attempt to use it result in an immediate panic followed by PoisonError on every future attempt:

thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:143:42:
Error getting api_mutex: Hid(InitializationError)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }
thread 'tokio-runtime-worker' panicked at /home/naps62/.cargo/git/checkouts/coins-a4bf2ba8095a65ef/9eff708/ledger/src/transports/native/hid.rs:142:47:
Could not lock api wrapper: PoisonError { .. }

One thing I should note, and that already happened even before all these fixes, is that I was already getting sporadic errors of the form hid_error is not implemented yet, which seems to come directly from hid.c, so unfortunately hard to debug. I had these requests around a retry block, since they would randomly fail. But this new update means any failure leads to a poisoned mutex

prestwich commented 10 months ago

is it possible to make a minimal repro? I've never encountered the hid_error is not implemented yet error.

by overriding the dependency to force ethers-rs to use it. since it's a minor version bump I assume no intended breaking changes

we're pre-1.0, so minor version bumps are still eligible for breaking changes. For this particular change, if you're using use coins_ledger::Ledger there should be no migration required. if you're using lower-level types, there may be breakage

naps62 commented 10 months ago

@prestwich here you go: https://github.com/naps62/coins_ledger-issue-128/tree/main

after narrowing this down, actually found it to be 2 different issues, though they're probably related. see the comments on main() for details. this reproduces both issues consistently on my system (Manjaro Linux, Ledger Nano X) tested with both 0.9.0 and 0.9.1

prestwich commented 10 months ago

thanks! looking now :)

prestwich commented 10 months ago

So this update makes it no longer panic. Instead a useful error message is added: https://github.com/summa-tx/coins/pull/129

Can you check that this resolves your issues? I ran it against your repo and it prevents panicking, although i'm not sure I completely understood your Issue 1

ffr, you can use the following in your cargo.toml to patch subdeps:

[patch.crates-io]
coins-ledger = { git = "...", tag = "..." }

https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html

naps62 commented 10 months ago

ffr, you can use the following in your cargo.toml to patch subdeps:

was wondering why you pointed this out, since I'm aware. just noticed I didn't properly override this dep on my repo above. was late at night, sorry :smile: