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

cannot sign non-eip 1559 with clef using external.ExternalSigner #23273

Closed
ralph-pichler opened this issue Jul 27, 2021 · 0 comments · Fixed by #23274
Closed

cannot sign non-eip 1559 with clef using external.ExternalSigner #23273

ralph-pichler opened this issue Jul 27, 2021 · 0 comments · Fixed by #23274
Assignees
@holiman
Labels
type:bug

Comments

@ralph-pichler
Copy link

System information

Geth version: 1.10.6 (or any >= 1.10.4)

Expected behaviour

If SignTx (on external.ExternalSigner) is called with a tx that is a types.Transaction of type legacy then it should send a account_signTransaction rpc request to clef for a legacy transaction.

Actual behaviour

It always sends a request for a eip 1559 transaction.

There is this code in SignTx which seem to be the code responsible for handling this.

if tx.GasFeeCap() != nil {
  args.MaxFeePerGas = (*hexutil.Big)(tx.GasFeeCap())
  args.MaxPriorityFeePerGas = (*hexutil.Big)(tx.GasTipCap())
} else {
  args.GasPrice = (*hexutil.Big)(tx.GasPrice())
}

However both LegacyTx and AccessListTx return the gas price for GasFeeCap() so it is never nil and a eip 1559 transaction is always requested.
@ralph-pichler ralph-pichler mentioned this issue Jul 27, 2021
Merged
@holiman holiman self-assigned this Jul 27, 2021
holiman added a commit to holiman/go-ethereum that referenced this issue Jul 27, 2021
@holiman
accounts/external, internal/ethapi: make ext signer sign legacy + sig…
dd8580e 
…n london if all fields set. Fixes ethereum#23273
@holiman holiman mentioned this issue Jul 27, 2021
Merged
@holiman holiman linked a pull request Jul 27, 2021 that will close this issue
accounts/external, internal/ethapi: make ext signer sign legacy #23274
Merged
fjl pushed a commit that referenced this issue Jul 29, 2021
@holiman
accounts/external, internal/ethapi: fixes for London tx signing (#23274)
5c13012 
Ticket #23273 found a flaw where we were unable to sign legacy-transactions
using the external signer, even if we're still on non-london network. That's
fixed in this PR.

Additionally, I found that even when supplying all parameters, it was impossible
to sign a london-transaction on an unsynched node. It's a pretty common usecase
that someone wants to sign a transaction using an unsynced 'vanilla' node,
providing all necessary data. Our setDefaults, however, insisted on checking the
current block against the config. This PR therefore adds a case, so that if both
MaxPriorityFeePerGas and MaxFeePerGas are provided, we accept them as given.

OBS This PR fixes a regression -- on current master, we are unable to sign a
london-transaction unless the node is synched, which may break scenarios where
geth (or clef) is used as a cold wallet.

Fixes #23273
sidhujag pushed a commit to sidhujag/go-ethereum that referenced this issue Aug 1, 2021
@holiman
accounts/external, internal/ethapi: fixes for London tx signing (ethe…
c9ae14b 
…reum#23274)

Ticket ethereum#23273 found a flaw where we were unable to sign legacy-transactions
using the external signer, even if we're still on non-london network. That's
fixed in this PR.

Additionally, I found that even when supplying all parameters, it was impossible
to sign a london-transaction on an unsynched node. It's a pretty common usecase
that someone wants to sign a transaction using an unsynced 'vanilla' node,
providing all necessary data. Our setDefaults, however, insisted on checking the
current block against the config. This PR therefore adds a case, so that if both
MaxPriorityFeePerGas and MaxFeePerGas are provided, we accept them as given.

OBS This PR fixes a regression -- on current master, we are unable to sign a
london-transaction unless the node is synched, which may break scenarios where
geth (or clef) is used as a cold wallet.

Fixes ethereum#23273
reds pushed a commit to reds/go-ethereum that referenced this issue Aug 28, 2021
@holiman
accounts/external, internal/ethapi: fixes for London tx signing (ethe…
bc15577 
…reum#23274)

Ticket ethereum#23273 found a flaw where we were unable to sign legacy-transactions
using the external signer, even if we're still on non-london network. That's
fixed in this PR.

Additionally, I found that even when supplying all parameters, it was impossible
to sign a london-transaction on an unsynched node. It's a pretty common usecase
that someone wants to sign a transaction using an unsynced 'vanilla' node,
providing all necessary data. Our setDefaults, however, insisted on checking the
current block against the config. This PR therefore adds a case, so that if both
MaxPriorityFeePerGas and MaxFeePerGas are provided, we accept them as given.

OBS This PR fixes a regression -- on current master, we are unable to sign a
london-transaction unless the node is synched, which may break scenarios where
geth (or clef) is used as a cold wallet.

Fixes ethereum#23273
i-norden pushed a commit to cerc-io/go-ethereum that referenced this issue Sep 10, 2021
@holiman @i-norden
accounts/external, internal/ethapi: fixes for London tx signing (ethe…
ce5e8c4 
…reum#23274)

Ticket ethereum#23273 found a flaw where we were unable to sign legacy-transactions
using the external signer, even if we're still on non-london network. That's
fixed in this PR.

Additionally, I found that even when supplying all parameters, it was impossible
to sign a london-transaction on an unsynched node. It's a pretty common usecase
that someone wants to sign a transaction using an unsynced 'vanilla' node,
providing all necessary data. Our setDefaults, however, insisted on checking the
current block against the config. This PR therefore adds a case, so that if both
MaxPriorityFeePerGas and MaxFeePerGas are provided, we accept them as given.

OBS This PR fixes a regression -- on current master, we are unable to sign a
london-transaction unless the node is synched, which may break scenarios where
geth (or clef) is used as a cold wallet.

Fixes ethereum#23273
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this issue Oct 15, 2021
@holiman @atif-konasl
accounts/external, internal/ethapi: fixes for London tx signing (ethe…
50961c0 
…reum#23274)

Ticket ethereum#23273 found a flaw where we were unable to sign legacy-transactions
using the external signer, even if we're still on non-london network. That's
fixed in this PR.

Additionally, I found that even when supplying all parameters, it was impossible
to sign a london-transaction on an unsynched node. It's a pretty common usecase
that someone wants to sign a transaction using an unsynced 'vanilla' node,
providing all necessary data. Our setDefaults, however, insisted on checking the
current block against the config. This PR therefore adds a case, so that if both
MaxPriorityFeePerGas and MaxFeePerGas are provided, we accept them as given.

OBS This PR fixes a regression -- on current master, we are unable to sign a
london-transaction unless the node is synched, which may break scenarios where
geth (or clef) is used as a cold wallet.

Fixes ethereum#23273
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@holiman holiman

Labels
type:bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

accounts/external, internal/ethapi: make ext signer sign legacy holiman/go-ethereum
2 participants
@holiman @ralph-pichler