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.

Sign up for GitHub

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

Jump to bottom

accounts/usbwallet: support dynamic tx for ledger #30180

Merged
merged 2 commits into from
Nov 8, 2024
Merged

accounts/usbwallet: support dynamic tx for ledger #30180

merged 2 commits into from
Nov 8, 2024
+24 −5

Conversation

shrimalmadhur
Copy link
Contributor

@shrimalmadhur shrimalmadhur commented Jul 17, 2024 •
edited
Loading

Fixes #30173

  • Add support for EIP 1559 Dynamic tx using ledger

shrimalmadhur
shrimalmadhur commented Jul 17, 2024
View reviewed changes
accounts/usbwallet/ledger.go
@@ -353,7 +367,9 @@ func (w *ledgerDriver) ledgerSign(derivationPath []uint32, tx *types.Transaction
// Chunk size selection to mitigate an underlying RLP deserialization issue on the ledger app.
// https://github.com/LedgerHQ/app-ethereum/issues/409
chunk := 255
for ; len(payload)%chunk <= ledgerEip155Size; chunk-- {
if tx.Type() == types.LegacyTxType {
Copy link
Contributor Author

@shrimalmadhur shrimalmadhur Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I am not sure if this is need for DynamicTx. Does this bug exist in DynamicTx too or it only exist in EIP155?

Copy link
Contributor

Choose a reason for hiding this comment

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

The workaround can be left in place for all tx types. It's just a way of modifying the transfer so it doesn't trigger a certain bug in the firmware.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fjl this doesn't seems to work for other tx. This could very well be due to the way typed tx are formed.

if tx.Type() == types.DynamicFeeTxType {
	if txrlp, err = rlp.EncodeToBytes([]interface{}{chainID, tx.Nonce(), tx.GasTipCap(), tx.GasFeeCap(), tx.Gas(), tx.To(), tx.Value(), tx.Data(), tx.AccessList()}); err != nil {
		return common.Address{}, nil, err
	}
	// append type to transaction
	txrlp = append([]byte{tx.Type()}, txrlp...)
}

I am not sure how to handle this bug here.
Ledger throws error

transaction type not supported

Copy link
Contributor

@holiman holiman Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So, I was going to ask you to show some snippet from ledger-docs or something, to demonstrate that they do accept this format, where you prepend the type into the payload like this.

But, seeing as the ledger throws transaction type not supported -- doesn't this mean that this PR is moot, and that the ledger (at least with the software version you are using) does not support signing non-legacy transactions?

Copy link
Contributor Author

@shrimalmadhur shrimalmadhur Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@holiman So the error transaction type not supported appears when I apply for ; len(payload)%chunk <= ledgerEip155Size; chunk-- logic for non legacy tx. If I don't do it - Ledger signing works. I have tested with my Ledger nano S. Let me find relevant ledger code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here you go @holiman https://github.com/LedgerHQ/app-ethereum/blob/develop/src/ethUstream.c#L300 - from what I understand it supports it.

@shrimalmadhur shrimalmadhur changed the title usbwallet: support dynamic tx accounts/usbwallet: support dynamic tx Jul 17, 2024
@shrimalmadhur
Copy link
Contributor Author

I am also not sure why the ci test is failing. doesn't seem related? How can I rerun it?

@gballet gballet self-assigned this Jul 18, 2024
@shrimalmadhur
Copy link
Contributor Author

Hey @gballet I saw you assigned this task to yourself. Wondering when you can take a look at it?

@shrimalmadhur shrimalmadhur requested a review from fjl July 25, 2024 04:21
@fjl
Copy link
Contributor

fjl commented Aug 29, 2024

@gballet was assigned because he has a Ledger to test this with.

@holiman
Copy link
Contributor

holiman commented Aug 29, 2024

@gballet was assigned because he has a Ledger to test this with.

Yeah I also have a few, for testing, so I can also give this a spin

@shrimalmadhur
Copy link
Contributor Author

@gballet was assigned because he has a Ledger to test this with.

Yeah I also have a few, for testing, so I can also give this a spin

awesome! thanks. let me know how your testing goes.

@holiman holiman force-pushed the madhur/support-dynamic-tx-ledger branch from 370f1a9 to 6680d76 Compare November 8, 2024 13:40
@holiman
Copy link
Contributor

holiman commented Nov 8, 2024 •
edited
Loading

First attempt, invoking signTransaction, I get to verify the details on the
ledger, and click Approve. Then an error occurred:

> eth.accounts // trigger derivation
> acc="0x9858EfFD232B4033E47d90003D41EC34EcaEda94"
> eth.signTransaction({from:acc, to:acc, value:1, maxFeePerGas:1, maxPriorityFeePerGas:1,  nonce:0 , gas:1 })
WARN [11-08|14:21:03.662] Served eth_signTransaction               reqid=13 duration=14.165008741s err="transaction type not supported"
Error: transaction type not supported
        at web3.js:6386:9(39)
        at send (web3.js:5115:62(29))
        at <eval>:1:20(17)


> eth.signTransaction({from:acc, to:acc, value:1, gasPrice:1, nonce:0 , gas:1 })

After fixing that, it now works. Below is testing with type 2 and 0 (legacy):

INFO [11-08|14:36:19.876] New wallet appeared                      url=ledger://1-1:1.0              status="Ethereum app v1.9.19 online"
> acc="0x9858EfFD232B4033E47d90003D41EC34EcaEda94"
"0x9858EfFD232B4033E47d90003D41EC34EcaEda94"
> eth.accounts
INFO [11-08|14:36:26.369] Skipping tracking first account on legacy path, use personal.deriveAccount(<url>,<path>, false) to track url=ledger://1-1:1.0 path=m/44'/60'/0'/0 address=0xB8Fd42000d00202DCbCF5e18d6640d656345FD6A
INFO [11-08|14:36:26.900] USB wallet discovered new account        url=ledger://1-1:1.0 address=0x9858EfFD232B4033E47d90003D41EC34EcaEda94 path=m/44'/60'/0'/0/0 balance=0 nonce=0
["0x8a8eafb1cf62bfbeb1741769dae1a9dd47996192", "0x64f951c2c1b289638b5c7fb6eb6657ee359dd984", "0x45af000871446ef5af43c3d2028860a21cb83abd", "0x19e7e376e7c213b7e7e7e46cc70a5dd086daff2a", "0x19e7e376e7c213b7e7e7e46cc70a5dd086daff2a", "0x19e7e376e7c213b7e7e7e46cc70a5dd086daff2a", "0x78d1ad571a1a09d60d9bbf25894b44e4c8859595", "0x966cfcd792fc39fce5f0e5c4bc96f965fc9e7cf2", "0x8db06da7351fce327860219aa7c617033d2ae9e2", "0x084e39f61973d2e39c14efcbd98eddbda74fe51e", "0xf80019facf2341a1fa124c210e456f02733217d9", "0x3e435db198c3acef58e0d67ac3d672e8bdac1db8", "0x2dfc01cd2f877e5eea18a1d6c13b8db31b18fd5e", "0xfd0fb88d8976eb6d766f742a47fa7fc850531269", "0xb9a900968016e3b693efa4129963efade1a840b9", "0x9160dc9105f7de5dc5e7f3d97ef11da47269bda6", "0x9858effd232b4033e47d90003d41ec34ecaeda94"]
> eth.signTransaction({from:acc, to:acc, value:1, maxFeePerGas:1, maxPriorityFeePerGas:1,  nonce:0 , gas:1 })
  raw: "0x02f8600180010101949858effd232b4033e47d90003d41ec34ecaeda940180c001a04891f68eb8bed9982521f211a35f268ddb3e6c19837d1be5528f89984d649a76a0273a79b624ae2f0f326eea7ca994ad3b9dbecdc72fdd932c716f42765a365278",
  tx: {
    accessList: [],
    chainId: "0x1",
    gas: "0x1",
    gasPrice: null,
    hash: "0xc55e6da8bc64ef22c9d908c2b256b4be11cddb30082086b603cd43cd27230c0d",
    input: "0x",
    maxFeePerGas: "0x1",
    maxPriorityFeePerGas: "0x1",
    nonce: "0x0",
    r: "0x4891f68eb8bed9982521f211a35f268ddb3e6c19837d1be5528f89984d649a76",
    s: "0x273a79b624ae2f0f326eea7ca994ad3b9dbecdc72fdd932c716f42765a365278",
    to: "0x9858effd232b4033e47d90003d41ec34ecaeda94",
    type: "0x2",
    v: "0x1",
    value: "0x1",
    yParity: "0x1"
  }
}
> eth.signTransaction({from:acc, to:acc, value:1, gasPrice:1, nonce:0 , gas:1 })
  raw: "0xf85d800101949858effd232b4033e47d90003d41ec34ecaeda94018026a08b68e70dfbca25a03bd9fb447ee432d7d495f6d82c6e7d76591bf8b89d17ae47a011a44757bffcca3fab3ff6ac4f82948c9d121cad0efce7222fd557224166c49a",
  tx: {
    chainId: "0x1",
    gas: "0x1",
    gasPrice: "0x1",
    hash: "0x37fbb8acd2f36753ffa898d92852eb4021796df006c38a89870d6355fbc6057f",
    input: "0x",
    maxFeePerGas: null,
    maxPriorityFeePerGas: null,
    nonce: "0x0",
    r: "0x8b68e70dfbca25a03bd9fb447ee432d7d495f6d82c6e7d76591bf8b89d17ae47",
    s: "0x11a44757bffcca3fab3ff6ac4f82948c9d121cad0efce7222fd557224166c49a",
    to: "0x9858effd232b4033e47d90003d41ec34ecaeda94",
    type: "0x0",
    v: "0x26",
    value: "0x1"
  }
}

holiman
holiman approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM

@holiman holiman added this to the 1.14.12 milestone Nov 8, 2024
@holiman holiman merged commit a6037d0 into ethereum:master Nov 8, 2024
1 of 2 checks passed
@holiman holiman changed the title accounts/usbwallet: support dynamic tx accounts/usbwallet: support dynamic tx for ledger Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@holiman holiman holiman approved these changes

@karalabe karalabe Awaiting requested review from karalabe karalabe is a code owner

@fjl fjl Awaiting requested review from fjl

Assignees

@gballet gballet

Labels
None yet
Projects
None yet
Milestone
1.14.12
Development

Successfully merging this pull request may close these issues.

Support LondonSigner in Ledger USBWallet
4 participants
@shrimalmadhur @fjl @holiman @gballet