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

account/usbwallet: make ledger x discoverable on macos14 #28516

Closed
wants to merge 4 commits into from
Closed

account/usbwallet: make ledger x discoverable on macos14 #28516

wants to merge 4 commits into from

Conversation

felipe-op
Copy link

@felipe-op felipe-op commented Nov 13, 2023

Hi,

I tried using this code to interface with a Ledger X device and I bumped into this error:

hidapi: failed to open device

This started happened in my Mac M2 with macos 14 (sonoma). Debugging it I noticed the usb.DeviceInfo path got an empty string, which then failed when Open is called.

Debugging it I found it actually enumerate 3 interfaces, but go-ethereum usbwallet hub arbitrarily picks one based on the usageID. So I decided to change to pick the other two. It eventually worked.

To match additional usageIDs, I converted the field into a slice and ignored matches with empty Path as seems like they can't be opened anyways.

Here are the three enumerated devices printed with a simple fmt.Printf("enumerate: %#v", infos)

enumerate: []usb.DeviceInfo{

usb.DeviceInfo{Path:"2c97:4015:01", VendorID:0x2c97, ProductID:0x4015, Release:0x0,   Serial:"",     Manufacturer:"",       Product:"",       UsagePage:0x0,    Usage:0x0, Interface:2,  rawDevice:(*usb._Ctype_struct_libusb_device)(0x6000003cc3c0), rawPort:(*uint8)(0x140002e84a8), rawReader:(*uint8)(0x140002e8498), rawWriter:(*uint8)(0x140002e8499)}, 

usb.DeviceInfo{Path:"",             VendorID:0x2c97, ProductID:0x4015, Release:0x201, Serial:"0001", Manufacturer:"Ledger", Product:"Nano X", UsagePage:0xffa0, Usage:0x1, Interface:-1, rawDevice:interface {}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}, 

usb.DeviceInfo{Path:"",             VendorID:0x2c97, ProductID:0x4015, Release:0x201, Serial:"0001", Manufacturer:"Ledger", Product:"Nano X", UsagePage:0xf1d0, Usage:0x1, Interface:-1, rawDevice:interface {}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}

}

Test

Tested only on macos 14 with a Ledger X.
Would be interesting if someone can validate this is not breaking other OS and devices before merging. i.e. not sure how Linux and Windows would handle the empty usb.DeviceInfo Path.

@felipe-op
Copy link
Author

@karalabe appreciate if you can take a look

@@ -93,30 +94,30 @@ func NewLedgerHub() (*Hub, error) {
0x4011, /* HID + WebUSB Ledger Nano X */
0x5011, /* HID + WebUSB Ledger Nano S Plus */
0x6011, /* HID + WebUSB Ledger Nano FTS */
}, 0xffa0, 0, newLedgerDriver)
}, []uint16{0xffa0, 0}, 2, newLedgerDriver)
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: here the endpointID was changed to 2. Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, there is a usageID of zero in the array here?

Copy link
Author

Choose a reason for hiding this comment

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

Those values matches what was enumerated on my device.

Copy link
Member

Choose a reason for hiding this comment

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

On macos13 I don't see any difference after removing the usageID of zero and setting endpointID to 0.

@mds1
Copy link

mds1 commented Dec 11, 2023

Just ran into the same issue today so would love to see this resolved :)

@lightclient
Copy link
Member

I can confirm that master is not working on macos 13 with a ledger x. Going to update to 14 and check again.

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

I confirm it is working on macos14 with ledger nano also. I lean towards removing the additional usageID of zero and setting endpointID back to 0, but it seems to work either way.

@holiman
Copy link
Contributor

holiman commented Dec 15, 2023

I lean towards removing the additional usageID of zero and setting endpointID back to 0, but it seems to work either way.

@lightclient but is there anything left of the PR at this point? Are you saying it's moot?

EDIT: I guess this change still remains: info.Path != "" &&

@holiman
Copy link
Contributor

holiman commented Dec 15, 2023

I ran the master code, with the following diff

diff --git a/accounts/usbwallet/hub.go b/accounts/usbwallet/hub.go
index e67942dbc1..53a0463967 100644
--- a/accounts/usbwallet/hub.go
+++ b/accounts/usbwallet/hub.go
@@ -23,6 +23,7 @@ import (
 	"sync/atomic"
 	"time"
 
+	"fmt"
 	"github.com/ethereum/go-ethereum/accounts"
 	"github.com/ethereum/go-ethereum/event"
 	"github.com/ethereum/go-ethereum/log"
@@ -183,6 +184,11 @@ func (hub *Hub) refreshWallets() {
 	}
 	hub.enumFails.Store(0)
 
+	fmt.Printf("enumerated:\n")
+	for i, info := range infos {
+		fmt.Printf("%d. %#v\n", i, info)
+	}
+	fmt.Printf("-----\n")
 	for _, info := range infos {
 		for _, id := range hub.productIDs {
 			// Windows and Macos use UsageID matching, Linux uses Interface matching

On linux, Ledger Nano X, it sees three

0. usb.DeviceInfo{Path:"2c97:4015:01", VendorID:0x2c97, ProductID:0x4015, Release:0x0, Serial:"", Manufacturer:"", Product:"", UsagePage:0x0, Usage:0x0, Interface:2, rawDevice:(*usb._Ctype_struct_libusb_device)(0x743404009590), rawPort:(*uint8)(0xc00440e310), rawReader:(*uint8)(0xc00440e300), rawWriter:(*uint8)(0xc00440e301)}
1. usb.DeviceInfo{Path:"0001:0002:00", VendorID:0x2c97, ProductID:0x4015, Release:0x201, Serial:"0001", Manufacturer:"Ledger", Product:"Nano X", UsagePage:0x0, Usage:0x0, Interface:0, rawDevice:interface {}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}
2. usb.DeviceInfo{Path:"0001:0002:01", VendorID:0x2c97, ProductID:0x4015, Release:0x201, Serial:"0001", Manufacturer:"Ledger", Product:"Nano X", UsagePage:0x0, Usage:0x0, Interface:1, rawDevice:interface {}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}

On linux, Ledger Nano S, only one:

0. usb.DeviceInfo{Path:"0001:0003:00", VendorID:0x2c97, ProductID:0x1, Release:0x200, Serial:"0001", Manufacturer:"Ledger", Product:"Nano S", UsagePage:0x0, Usage:0x0, Interface:0, rawDevice:interface {}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}

And linux, Ledger Nano S Plus, also three:

0. usb.DeviceInfo{Path:"2c97:5015:01", VendorID:0x2c97, ProductID:0x5015, Release:0x0, Serial:"", Manufacturer:"", Product:"", UsagePage:0x0, Usage:0x0, Interface:2, rawDevice:(*usb._Ctype_struct_libusb_device)(0x77f7a4009590), rawPort:(*uint8)(0xc003d6fa18), rawReader:(*uint8)(0xc003d6fa08), rawWriter:(*uint8)(0xc003d6fa09)}
1. usb.DeviceInfo{Path:"0001:0004:00", VendorID:0x2c97, ProductID:0x5015, Release:0x201, Serial:"0001", Manufacturer:"Ledger", Product:"Nano S Plus", UsagePage:0x0, Usage:0x0, Interface:0, rawDevice:interface {}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}
2. usb.DeviceInfo{Path:"0001:0004:01", VendorID:0x2c97, ProductID:0x5015, Release:0x201, Serial:"0001", Manufacturer:"Ledger", Product:"Nano S Plus", UsagePage:0x0, Usage:0x0, Interface:1, rawDevice:interface {}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}

So from what I can tell, changing the discovery to require non-empty path should not affect linux.

@holiman
Copy link
Contributor

holiman commented Dec 15, 2023

On my machine, it selected this one:

Hub (usageId: 0xffa0, endpointId: 0x0) Selecting on id 0x4015: usb.DeviceInfo{Path:"0001:0005:00", VendorID:0x2c97, ProductID:0x4015, Release:0x201, Serial:"0001", Manufacturer:"Ledger", Product:"Nano X", UsagePage:0x0, Usage:0x0, Interface:0, rawDevice:interface {}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}

Which seems to work. I don't know if or why this one is any better than the other two.

@lightclient
Copy link
Member

So from what I can tell, changing the discovery to require non-empty path should not affect linux.

It appears it will affect macos though. Running your patch with a nano x, I get

0. usb.DeviceInfo{Path:"2c97:4015:03", VendorID:0x2c97, ProductID:0x4015,Release:0x0, Serial:"", Manufacturer:"", Product:"", UsagePage:0x0, Usage:0x0, Interface:2, rawDevice(*usb._Ctype_struct_libusb_device)(0x600001dfc460), rawPort:(*uint8)(0x14000e89258), rawReader:(*uint8)(0x14000e89248), rawWriter:(*uint8)(0x14000e89249)}
1. usb.DeviceInfo{Path:"", VendorID:0x2c97, ProductID:0x4015, Release:0x201, Serial:"0001", Manufacturer:"Ledger", Product:"Nano X", UsagePage:0xf1d0, Usage:0x1, Interface:-1, rawDevice:interface{}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}
2. usb.DeviceInfo{Path:"", VendorID:0x2c97, ProductID:0x4015, Release:0x201, Serial:"0001", Manufacturer:"Ledger", Product:"Nano X", UsagePage:0xffa0, Usage:0x1, Interface:-1, rawDevice:interface{}(nil), rawPort:(*uint8)(nil), rawReader:(*uint8)(nil), rawWriter:(*uint8)(nil)}

I lean towards removing the additional usageID of zero and setting endpointID back to 0

I must have confused this by removing each individually and not together. As you can see, if you send the endpointID back to 0, the code will still match the usageID of 0. And if you remove that usageID, it will match on the endpointID 2. Seems like you still need one or the other.

But the issue is that the official ledger library does it like we already have on master. So I wonder if there is another issue somewhere farther up the usb stack.

https://github.com/LedgerHQ/ledgercomm/blob/5e344823de03838553b87181b5b5c7ce3e1a8ea4/ledgercomm/interfaces/hid_device.py#L80-L112

@lightclient
Copy link
Member

Also foundry imports coins_ledger which matches using usage_page == 0xffa0 for mac (like we do on master), and I can use my ledger with it on mac. So it seems like our HID library is having an issue.

@holiman
Copy link
Contributor

holiman commented Dec 22, 2023

According to @lightclient , this PR karalabe/usb#43 which contains an updated hid-library, also fixes the problems on macosx14.

Once we that working on windows, we'll merge that + update the usb library, and we don't need this PR. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants