accounts/usbwallet: support dynamic tx for ledger #30180

shrimalmadhur · 2024-07-17T21:36:07Z

Add support for EIP 1559 Dynamic tx using ledger

shrimalmadhur · 2024-07-17T21:36:31Z

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 {


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

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.

@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

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?

@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

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

shrimalmadhur · 2024-07-18T00:45:58Z

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

shrimalmadhur · 2024-07-20T06:35:49Z

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

fjl · 2024-08-29T07:59:13Z

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

holiman · 2024-08-29T08:12:06Z

@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 · 2024-08-29T22:57:44Z

@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 · 2024-11-08T13:40:49Z

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

LGTM

Adds support non-legacy transaction-signing using ledger --------- Co-authored-by: Martin Holst Swende <martin@swende.se>

shrimalmadhur requested a review from karalabe as a code owner July 17, 2024 21:36

shrimalmadhur commented Jul 17, 2024

View reviewed changes

shrimalmadhur changed the title ~~usbwallet: support dynamic tx~~ accounts/usbwallet: support dynamic tx Jul 17, 2024

gballet self-assigned this Jul 18, 2024

shrimalmadhur requested a review from fjl July 25, 2024 04:21

shrimalmadhur and others added 2 commits November 8, 2024 14:12

usbwallet: support dynamic tx

54cc1e6

accounts/usbwallet: fix sender derivation for new types

6680d76

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

holiman approved these changes Nov 8, 2024

View reviewed changes

holiman added this to the 1.14.12 milestone Nov 8, 2024

holiman merged commit a6037d0 into ethereum:master Nov 8, 2024
1 of 2 checks passed

holiman changed the title ~~accounts/usbwallet: support dynamic tx~~ accounts/usbwallet: support dynamic tx for ledger Nov 8, 2024

holiman added a commit that referenced this pull request Nov 19, 2024

accounts/usbwallet: support dynamic tx (#30180)

f476702

Adds support non-legacy transaction-signing using ledger --------- Co-authored-by: Martin Holst Swende <martin@swende.se>

BrewTestBot mentioned this pull request Nov 19, 2024

ethereum 1.14.12 Homebrew/homebrew-core#198233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

accounts/usbwallet: support dynamic tx for ledger #30180

accounts/usbwallet: support dynamic tx for ledger #30180

shrimalmadhur commented Jul 17, 2024 •

edited

Loading

shrimalmadhur Jul 17, 2024 •

edited

Loading

fjl Jul 24, 2024

shrimalmadhur Jul 25, 2024

holiman Aug 28, 2024 •

edited

Loading

shrimalmadhur Aug 28, 2024 •

edited

Loading

shrimalmadhur Aug 28, 2024

shrimalmadhur commented Jul 18, 2024

shrimalmadhur commented Jul 20, 2024

fjl commented Aug 29, 2024

holiman commented Aug 29, 2024

shrimalmadhur commented Aug 29, 2024

holiman commented Nov 8, 2024 •

edited

Loading

holiman left a comment

accounts/usbwallet: support dynamic tx for ledger #30180

accounts/usbwallet: support dynamic tx for ledger #30180

Conversation

shrimalmadhur commented Jul 17, 2024 • edited Loading

shrimalmadhur Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

fjl Jul 24, 2024

Choose a reason for hiding this comment

shrimalmadhur Jul 25, 2024

Choose a reason for hiding this comment

holiman Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

shrimalmadhur Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

shrimalmadhur Aug 28, 2024

Choose a reason for hiding this comment

shrimalmadhur commented Jul 18, 2024

shrimalmadhur commented Jul 20, 2024

fjl commented Aug 29, 2024

holiman commented Aug 29, 2024

shrimalmadhur commented Aug 29, 2024

holiman commented Nov 8, 2024 • edited Loading

holiman left a comment

Choose a reason for hiding this comment

shrimalmadhur commented Jul 17, 2024 •

edited

Loading

shrimalmadhur Jul 17, 2024 •

edited

Loading

holiman Aug 28, 2024 •

edited

Loading

shrimalmadhur Aug 28, 2024 •

edited

Loading

holiman commented Nov 8, 2024 •

edited

Loading