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

accounts/external, internal/ethapi: make ext signer sign legacy #23274

Merged
merged 1 commit into from Jul 29, 2021

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Jul 27, 2021

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.

Tests

Using clef as a backend.

Legacy tx

> eth.signTransaction({ to: a, from:a , value:1, gasPrice: 1, gas: 22000, nonce: 0} )
{
  raw: "0xf85f80018255f0948a8eafb1cf62bfbeb1741769dae1a9dd4799619201802aa0d7af07f62836a1908b423a1274731a02e673640adb9581bc869f6356397187e0a0420e1aaf99a8772db5b7b29b0ff6d420c7962ee42cbf32bde5bc2a9d9a433ae3",
  tx: {
    gas: "0x55f0",
    gasPrice: "0x1",
    hash: "0x5ad46f61b0a976aa6123088df4f2dfabc66857f34fd723637111769d929120d5",
    input: "0x",
    maxFeePerGas: null,
    maxPriorityFeePerGas: null,
    nonce: "0x0",
    r: "0xd7af07f62836a1908b423a1274731a02e673640adb9581bc869f6356397187e0",
    s: "0x420e1aaf99a8772db5b7b29b0ff6d420c7962ee42cbf32bde5bc2a9d9a433ae3",
    to: "0x8a8eafb1cf62bfbeb1741769dae1a9dd47996192",
    type: "0x0",
    v: "0x2a",
    value: "0x1"
  }
}

Access list tx:

> eth.signTransaction({ to: a, from:a , value:1, gasPrice: 1, gas: 22000, nonce: 0, accessList:[]} )
{
  raw: "0x01f8600380018255f0948a8eafb1cf62bfbeb1741769dae1a9dd479961920180c001a0c73c18d7f32900081cdd093aebf62b4ef0fdfe1a8f05241ae8fd2b4f27872d4c9f780156fa7e37b25c2dca7a990ccfd3f93f8ccfc71e3b930f55466c23cf8fe5",
  tx: {
    accessList: [],
    chainId: "0x3",
    gas: "0x55f0",
    gasPrice: "0x1",
    hash: "0xbd4075faddb50e4f98497380c4bb315b588b74039ea23450e4e0f2a98ebaf245",
    input: "0x",
    maxFeePerGas: null,
    maxPriorityFeePerGas: null,
    nonce: "0x0",
    r: "0xc73c18d7f32900081cdd093aebf62b4ef0fdfe1a8f05241ae8fd2b4f27872d4c",
    s: "0x780156fa7e37b25c2dca7a990ccfd3f93f8ccfc71e3b930f55466c23cf8fe5",
    to: "0x8a8eafb1cf62bfbeb1741769dae1a9dd47996192",
    type: "0x1",
    v: "0x1",
    value: "0x1"
  }
}

1559 tx:

> eth.signTransaction({ to: a, from:a , value:1, gas: 22000, nonce: 0, accessList:[], maxFeePerGas: 1, maxPriorityFeePerGas: 1} )
{
  raw: "0x02f862038001018255f0948a8eafb1cf62bfbeb1741769dae1a9dd479961920180c080a092c02592bdf7cf2221f7e5023169c0540bfe0e4fcadf5646f48e8d7095c5015ca025cbd2168af56145d26bcd2a3fc779fff0b7675b0986c95318d9c1423d553051",
  tx: {
    accessList: [],
    chainId: "0x3",
    gas: "0x55f0",
    gasPrice: null,
    hash: "0x8d49e0c46228caee0715940f1e7b91ef7fe4d1690a417327187178bc98e8c7a8",
    input: "0x",
    maxFeePerGas: "0x1",
    maxPriorityFeePerGas: "0x1",
    nonce: "0x0",
    r: "0x92c02592bdf7cf2221f7e5023169c0540bfe0e4fcadf5646f48e8d7095c5015c",
    s: "0x25cbd2168af56145d26bcd2a3fc779fff0b7675b0986c95318d9c1423d553051",
    to: "0x8a8eafb1cf62bfbeb1741769dae1a9dd47996192",
    type: "0x2",
    v: "0x0",
    value: "0x1"
  }
}

@holiman holiman linked an issue Jul 27, 2021 that may be closed by this pull request
@holiman holiman added this to the 1.10.7 milestone Jul 27, 2021
@fjl fjl merged commit 5c13012 into ethereum:master Jul 29, 2021
@fjl
Copy link
Contributor

fjl commented Jul 29, 2021

Note: I didn't test this.

sidhujag pushed a commit to sidhujag/go-ethereum that referenced this pull request Aug 1, 2021
…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
@karalabe
Copy link
Member

If I specify both maxFee and maxPriorityFee, but the priority fee is > max fee, this PR will accept it. The original code rejected it

		if args.MaxFeePerGas.ToInt().Cmp(args.MaxPriorityFeePerGas.ToInt()) < 0 {
			return fmt.Errorf("maxFeePerGas (%v) < maxPriorityFeePerGas (%v)", args.MaxFeePerGas, args.MaxPriorityFeePerGas)
		}

^this check doesn't get called any more

@karalabe
Copy link
Member

Note: I didn't test this.

Then don't merge it

reds pushed a commit to reds/go-ethereum that referenced this pull request Aug 28, 2021
…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 pull request Sep 10, 2021
…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 pull request Oct 15, 2021
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cannot sign non-eip 1559 with clef using external.ExternalSigner
3 participants