Skip to content
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

[ledger-go] upgrade ledger-go to v1.0.0 #8872

Closed
wants to merge 1 commit into from

Conversation

Galadrin
Copy link
Contributor

@Galadrin Galadrin commented Dec 6, 2024

following Ledger firmware upgrade, the ledger-go need to be upgraded.

What is the purpose of the change

followig Ledger firmware upgrade, the Osmosis client is no more able to communicate with Ledger device
Error: ledger nano S: LedgerHID device (idx 0) not found. Ledger LOCKED OR Other Program/Web Browser may have control of device.

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes?
  • Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • Specification (x/{module}/README.md)
  • Osmosis documentation site
  • Code comments?
  • N/A

following Ledger firmware upgrade, the ledger-go need to be upgraded.

Signed-off-by: David Pierret <[email protected]>
@PaddyMc
Copy link
Collaborator

PaddyMc commented Dec 9, 2024

Thanks for the PR, will need to do some backwards compatibility testing, but will try and get this merged ASAP 🙇

@rootulp
Copy link
Contributor

rootulp commented Dec 10, 2024

@Galadrin can you please provide more info on what Ledger firmware upgrade broke for you? I confirmed that Ledger firmware 2.4.0 and Ledger Live 2.92.1 still work for Celestia. We don't depend on ledger-go directly but indirectly we use v0.14.3. See celestiaorg/celestia-app#4106

@Galadrin
Copy link
Contributor Author

Hi @rootulp, I have this issue with a Ledger Nano S+ firmware 1.3.0 and cosmos app 2.35.26

@rootulp
Copy link
Contributor

rootulp commented Dec 11, 2024

Thanks! Do you have any more information on the bug? I couldn't find an issue for it on:

  1. https://github.com/Zondax/ledger-go
  2. https://github.com/cosmos/ledger-cosmos
  3. https://github.com/cosmos/ledger-cosmos-go

I'm trying to determine if Celestia needs to bump a dependency in order to pick up the fix but I don't have a Ledger NanoS+ to test with so I can't easily reproduce the issue.

@deividaspetraitis
Copy link
Contributor

Hey @Galadrin, thanks for reporting! Have tested using Ledger Nano S device running OS version 2.1.0 and Atom App Version 2.35.26, however was not able to reproduce this bug, I think firmware is too old. @PaddyMc will try to test with newer firmware next week and will get back to you.

@pablin-10
Copy link
Contributor

I was able to reproduce the issue and test the fix.

I had a Ledger Nano S+ pre 1.3 firmware and it was working, I tested a simple osmosisd bank send

After upgrading the firmware to v 1.3, I saw same error @Galadrin reported:
Error: ledger nano S: LedgerHID device (idx 0) not found. Ledger LOCKED OR Other Program/Web Browser may have control of device.

Compiled this code with make build-reproducible-amd64:
https://github.com/crosnest/osmosis/tree/upgrade_ledger

And now I see my ledger working again 🚀

The only relevant thing is now I see this difference:

confirm transaction before signing and broadcasting [y/N]: y
2024/12/12 18:57:38 Sending command: 5500XXXXXXXXXXX
2024/12/12 18:57:38 Received response: 0002XXXXXXXXXXX
2024/12/12 18:57:38 Sending command: 5500XXXXXXXXXXX
2024/12/12 18:57:38 Received response: 0002XXXXXXXXXXX
2024/12/12 18:57:38 Sending command: 5504XXXXXXXXXXX
2024/12/12 18:57:39 Received response: 0207XXXXXXXXXXX
2024/12/12 18:57:39 Sending command: 5500XXXXXXXXXXX
2024/12/12 18:57:39 Received response: 0002XXXXXXXXXXX
2024/12/12 18:57:39 Sending command: 5500XXXXXXXXXXX
2024/12/12 18:57:39 Received response: 0002XXXXXXXXXXX
2024/12/12 18:57:39 Sending command: 5504XXXXXXXXXXX
2024/12/12 18:57:40 Received response: 0207XXXXXXXXXXX
2024/12/12 18:57:40 Sending command: 5502XXXXXXXXXXX
2024/12/12 18:57:40 Received response: 900XXXXXXXXXXX
2024/12/12 18:57:40 Sending command: 5502XXXXXXXXXXX
2024/12/12 18:57:40 Received response: 900XXXXXXXXXXX
2024/12/12 18:57:40 Sending command: 5502XXXXXXXXXXX
2024/12/12 18:57:49 Received response: 3044XXXXXXXXXXX

not sure if this is expected or someone left some debug-mode somewhere.

PS: @rootulp I tried to understand where does this error (or fix) might be coming from, same as you.

The only maybe-relevant change I saw was this:
Zondax/ledger-go@33eff3c#diff-a737b51fd59564ea01408afcbc036951b19940cc915ff30f40d7ff9e13419507L48

@pablin-10 pablin-10 self-requested a review December 12, 2024 22:27
Copy link
Contributor

@pablin-10 pablin-10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with a Ledger Nano S Plus.

@tbruyelle
Copy link

@pablin-10 I was able to reproduce the bug and test the fix as you did but for AtomOne (atomone-hub/atomone#59), and I also noticed the new log output. I created an issue in ledger-go for that (Zondax/ledger-go#41).

Copy link
Contributor

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

@github-actions github-actions bot added the Stale label Dec 22, 2024
@github-actions github-actions bot closed this Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants