-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: greatly simplify rusb usage #129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
leaving this open so that @naps62 and @xJonathanLEI can check it before merge 👍 |
nevermind. there's still a panic if I try to instantiate a ledger twice while the device is locked (1st call fails with Other than that, there's still a less serious wrinkle. the code below, without the #[tokio::main]
async fn main() {
// first call, works
let _ = dbg!(call_ledger().await);
// tokio::time::sleep(tokio::time::Duration::from_millis(10)).await;
// second call, if done without sleep first, fails with `hid_error is not implemented yet`
let _ = dbg!(call_ledger().await);
}
pub(crate) async fn call_ledger() -> Result<Ledger, LedgerError> {
let path: String = "m/44'/60'/0'/0/0".into();
Ledger::new(HDPath::Other(path), 1).await
} including the sleep makes this work consistently.
|
Will check whether this works on Android tomorrow and revert. |
Are you sure the line number is correct? that line is currently
This sounds like it might be a race condition between instnatiating the new task and releasing the resource in the old task? I'll play around with it |
not sure what I was on about with the panic thing. seems gone now, can't reproduce even pointing to the same 2nd issue (two successive calls without waiting) still exists, although I can work with it already, if you feel you need to merge this and research that one later thanks very much for the support btw 🙏 |
Just tested 9f86de4 on Android and it works perfectly. So far no issue found. |
Actually, no. I made a mistake and tested the wrong code. It actually no longer compiles on Android. I will look into it. |
Just sent #130 to fix the compilation error. It's trivial. |
However, even with the compilation fix, it still doesn't work properly on Android. I'm testing this crate through Specifically, dumping Ethereum addresses from paths works fine. However, when requesting a transaction signature, I'm getting this error:
Again I'm not even sure whether (Just to make sure, I also tested against the current |
I also get these randomly from time to time (even before this PR). have you tried multiple times, to make sure it's specifically from this commit, and not a fluke run? |
Yeah I tried multiple times. In fact, I've never encountered this error even once before this PR. |
is there an easy way for me to replicate your test environment without buying an android device @xJonathanLEI? Would be nice to check things locally 😅 |
I have an android, and am willing to do some debugging to narrow it down |
@prestwich Usually it's possible to check compilation etc via NDK, but since this is hardware stuff it might be hard to do without the actual hardware 😅. @naps62 Are you able to reproduce the same bug? I can make a temp repo/gist for this if that's helpful. To make it easier I will use |
I saw this bug a few times, but seems mostly at random, and never frequently enough to debug. I guess there's a lot of variance depending on hardware in all this (so far I tested only on Linux) |
So for clarity, this indicates that the read from the hid device was successful, but no data was available. It actually indicates that the device returned a succesful APDU answer with 0 bytes. If there was not a succesful answer, or if reading of that answer had failed, we would expect a So it is strange to me that we're getting |
Working on the reproducer and noticed that like dumping addresses, signing messages also works perfectly. |
@naps62 The reproducer is now available: xJonathanLEI/coins-android-bug-repro.
(Edit: I re-read your comment. Yes it's indeed the case that the native transport is successfully returning an empty answer. The error comes from Again note that this issue only occurs when signing a transaction but not a message. I originally coded the reproducer to sign a "hellow world" message but it ended up succeeding both on |
Alright, so i can repro without android. Will be diving into this today 🎉 |
okay I tracked this down to an off-by-one in the write buffer that was sending the device incorrect data. I am still unsure why this was significant only for certain instructions, but it seems to be fixed in the latest commit. Would you mind verifying on android for me? :D |
@prestwich I updated |
Tested dumping addresses and signing a "hello world" message - both work perfectly. All good on my side now. Thanks! |
thanks y'all! I've just published at 0.9.2 and yanked 0.9.1 and 0.9.0 🎉 |
FYI, still transient errors with two consecutive invocations. but something I can live with, and probably debug myself later 👍 thanks for the release |
I'll address the race condition in 0.9.3 👍 |
No description provided.