Util: Allow `v` to be `0` or `1` for EIP1559 transactions #1905

ernestognw · 2022-05-25T00:35:44Z

Fixes #1904

holgerd77 · 2022-05-25T08:28:24Z

Ah, just seeing that you already opened a PR on this.

So for context (sorry, this is likely not obvious, we likely want to better document): we are doing work on the current version of the packages towards master, on develop we are long-term-preparing the next round of breaking releases.

Let's anyhow continue here with discussing the things I raised in the issue, this would be the first step here.

codecov · 2022-05-25T13:28:28Z

Codecov Report

Merging #1905 (5f6bcc4) into master (2f42dcf) will increase coverage by 0.55%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	`85.57% <ø> (ø)`
blockchain	`83.82% <ø> (ø)`
client	`76.95% <ø> (ø)`
common	`94.19% <ø> (ø)`
devp2p	`82.62% <ø> (+0.26%)`	⬆️
ethash	`90.76% <ø> (ø)`
trie	`80.02% <ø> (ø)`
tx	`88.28% <100.00%> (+0.05%)`	⬆️
util	`92.63% <100.00%> (?)`
vm	`81.02% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

ernestognw · 2022-05-25T17:53:47Z

Ah, just seeing that you already opened a PR on this.

So for context (sorry, this is likely not obvious, we likely want to better document): we are doing work on the current version of the packages towards master, on develop we are long-term-preparing the next round of breaking releases.

Let's anyhow continue here with discussing the things I raised in the issue, this would be the first step here.

Thanks for context, I changed the target branch to master and rebased accordingly. I replied about the details on the issue, just right here

holgerd77

LGTM, reasoning and justification see separate comment, will merge.

holgerd77 · 2022-05-30T11:47:35Z

packages/util/src/signature.ts

@@ -46,6 +46,9 @@ export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: any): any {

 function calculateSigRecovery(v: BNLike, chainId?: BNLike): BN {
  const vBN = toType(v, TypeOutput.BN)
+
+  if (vBN.eqn(0) || vBN.eqn(1)) return toType(v, TypeOutput.BN)


Ok, this is the central line of this PR (this is just an additional transparency explanation for everyone to follow including the team). So this allows v to directly pass through if 0 or 1. The chainId parameter is ignored in this case.

The function here is not exported. Effects are indirect, to all methods below this comment where additional NOTE: line comments are added. I've checked all the methods and do not see security or other implications or side effects.

Namely:

ecrecover: This will then allow with this update to directly pass in an EIP-1559 (or any other typed) tx with v being 0 or 1 and will recover the correct PK in these cases as well. No side effects on other (legacy) txs.

toRpcSig: Now allows EIP-1559 txs to be converted in eth_sign format, no side effects

toCompactSig: Now allows EIP-1559 txs to be converted in eth_sign format, no side effects (was first a bit unsure about the 0 case but this is included as well respectively not affected)

isValidSignature: Now allows EIP-1559 txs to be converted in eth_sign format, no side effects

So I would give this a go and merge this and would include in the very imminent last round of master releases before we merge in develop.

We can then continue on develop with do some further signature code cleanup - with some inspiration from the breaking-release-TODOs - in favor of a complete direct removal.

Hmm I think I just realized a cleaner method of fixing the precompiles than doing this guard in the precompiles. What if we throw here if v = 0 or v = 1 and a chainId is provided? If you would now pass in a chainId and v = 1 or v = 0 then ecrecover will not throw, while it would throw before (since this signature recovery method returns a negative value in that case).

EDIT nvm this will not fix the precompiles. (But it might be cleaner to throw here if chainId is provided and v is 0 or 1?)

holgerd77 · 2022-05-30T11:50:42Z

Oh, respectively tests are failing, then we have to figure out before. This is likely related to the merge of #1900, @jochem-brouwer do you have an idea what happens? 🤔

jochem-brouwer · 2022-05-30T12:11:07Z

Hi @holgerd77, I don't think it is related to #1900 since ecrecover tests are failing. I'll take a look now 😄

holgerd77 · 2022-05-30T12:21:50Z

Hi @holgerd77, I don't think it is related to #1900 since ecrecover tests are failing. I'll take a look now 😄

Ah, interesting, I thought tests would have been passed before here, but also not 100% sure.

jochem-brouwer · 2022-05-30T12:42:05Z

There are errors in blockchain tests;

Errors thrown in file: GeneralStateTests/stPreCompiledContracts2/CALLCODEEcrecover1.json test: CALLCODEEcrecover1_d0g0v0_London:
	Error: invalid receiptTrie (vm hf=london -> block number=1 hash=0x7d96d00345e783092e7b862aaab79d0b08b041d0d91ce8720546c1214895bc91 hf=london baseFeePerGas=10 txs=1 uncles=0)
	correct last header block
Errors thrown in file: GeneralStateTests/stPreCompiledContracts2/CallEcrecover1.json test: CallEcrecover1_d0g0v0_London:
	Error: invalid receiptTrie (vm hf=london -> block number=1 hash=0xf94a90c5e3158e156ad8218bc49e8a59cb17be564648808466302e01040abe49 hf=london baseFeePerGas=10 txs=1 uncles=0)
	correct last header block
Errors thrown in file: GeneralStateTests/stStaticCall/static_CallEcrecover1.json test: static_CallEcrecover1_d0g0v0_London:
	Error: invalid receiptTrie (vm hf=london -> block number=1 hash=0x92f8983a4d37035279956e921f45a17e8b28146591e1dbf031cfffba11a23c2f hf=london baseFeePerGas=10 txs=1 uncles=0)
	correct last header block

These all have to do with the ecrecover precompile. (This is also the reason why vm-api CI fails)

Take a look at where ecrecover is called:

ethereumjs-monorepo/packages/vm/src/evm/precompiles/01-ecrecover.ts

Line 24 in 2f42dcf

publicKey = ecrecover(msgHash, new BN(v), r, s)

The input to these tests have v = 1. The precompile calls into util ecrecover. This calls into calculateSigRecovery (which is edited in this PR):

ethereumjs-monorepo/packages/util/src/signature.ts

Line 73 in 2f42dcf

const recovery = calculateSigRecovery(v, chainId)

In this PR, calculateSigRecovery immediately returns v = 1. However, without this PR, and with chainId as argument set as undefined (as is the case in the precompile!) it returns v - 27. This yields a negative number if v=1 and is thus invalid, the precompile throws. However, in this PR, because v = 1 it immediately returns 1, which is a valid value, and thus the call succeeds (and does not throw).

I don't see a clean way how to fix this, it would need an extra argument to ecrecover in util to signal that we are using a precompile and we thus force v = v - 27. (I did not check but I think that the call to ethereum-cryptography expects v of either 0 or 1, EDIT: yes, it does; https://github.com/paulmillr/noble-secp256k1/blob/9e1353def2be5ee7a0a87b515a66382acb547716/index.ts#L458).

jochem-brouwer · 2022-05-30T12:51:41Z

Actually, we could also do these v checks in the precompile? Would that work? I'm a bit hesitant to merge this PR because this has to do with signatures and could have big (security) implications..?

jochem-brouwer · 2022-05-30T20:14:56Z

Tx tests will fail, we should figure out why this v.eqn(0) check is in the _validateTxV method.

holgerd77 · 2022-05-31T09:05:42Z

I am on it.

holgerd77 · 2022-05-31T09:07:03Z

packages/tx/src/legacyTransaction.ts

+          throw new Error(`Non-EIP155 txs need either v = 27 or v = 28, got v = ${this.v}`)
+        }
+      }
+    }


I wouldn't tie this to "after the _validateTxV() analysis" but rather do before respectively directly in the method, think this will get more robust (less ocassion for the analysis to go wrong).

Will try this and update the tests.

…55 determination logic

holgerd77 · 2022-05-31T09:43:47Z

@jochem-brouwer ok, I would have a tendency to give this a go, but I would wait for your final confirmation.

jochem-brouwer

One question, otherwise LGTM.

jochem-brouwer · 2022-05-30T20:14:03Z

packages/tx/test/legacy.spec.ts

@@ -416,7 +412,7 @@ tape('[Transaction]', function (t) {
      st.notEqual(signedWithEIP155.v?.toString('hex'), '1c')
      st.notEqual(signedWithEIP155.v?.toString('hex'), '1b')

-      let signedWithoutEIP155 = Transaction.fromTxData(<any>fixtureTxSignedWithEIP155.toJSON(), {


For these tests we should review if there was any reason why we used this toJSON logic for already-signed txs.

packages/tx/src/legacyTransaction.ts

jochem-brouwer · 2022-05-30T20:18:36Z

packages/util/src/signature.ts

@@ -46,6 +46,9 @@ export function ecsign(msgHash: Buffer, privateKey: Buffer, chainId: any): any {

 function calculateSigRecovery(v: BNLike, chainId?: BNLike): BN {
  const vBN = toType(v, TypeOutput.BN)
+
+  if (vBN.eqn(0) || vBN.eqn(1)) return toType(v, TypeOutput.BN)


Hmm I think I just realized a cleaner method of fixing the precompiles than doing this guard in the precompiles. What if we throw here if v = 0 or v = 1 and a chainId is provided? If you would now pass in a chainId and v = 1 or v = 0 then ecrecover will not throw, while it would throw before (since this signature recovery method returns a negative value in that case).

EDIT nvm this will not fix the precompiles. (But it might be cleaner to throw here if chainId is provided and v is 0 or 1?)

jochem-brouwer · 2022-05-31T13:00:03Z

packages/tx/src/legacyTransaction.ts

+    if (v !== undefined) {
+      // v is 1. not matching the EIP-155 chainId included case and...
+      // v is 2. not matching the classic v=27 or v=28 case
+      if (v.ltn(37) && !v.eqn(27) && !v.eqn(28)) {


If v = 36 then we get a chainId = 0, right? Why is this not ok?

In case of v = 35, then v - 35 (thus 0, which is odd) means we subtract 36, so we end up with a negative chain id which is obviously wrong. But for 36 this logic seems to work..? Unless chain id 0 is special?

TBH I don't find any reference about chain ID 0 and I'm unsure if this theoretically a valid chain ID. I would rather assume for now that it is not, mainnet starts with 1 and all other chain IDs I came across were some arbitrary higher value. In doubt I would tend to leave "as is", if someone demands we could still add later on.

Yup, I agree

jochem-brouwer

LGTM, thanks @ernestognw 😄

ernestognw · 2022-05-31T15:24:52Z

LGTM, thanks @ernestognw 😄

Thanks to you folks! Many things happened here that I would've not been able to address, I appreciate it.
Please let me know if I can add something from my side.

ernestognw · 2022-05-31T15:32:34Z

packages/tx/src/legacyTransaction.ts

      !v.eqn(27) &&
      !v.eqn(28)


Are these becoming unreachable code?

holgerd77

LGTM

ernestognw changed the base branch from develop to master May 25, 2022 17:42

ernestognw changed the base branch from master to develop May 25, 2022 17:45

ernestognw changed the base branch from develop to master May 25, 2022 17:48

ernestognw force-pushed the signature/eip1559 branch from a59b51d to f57dd58 Compare May 25, 2022 17:51

Util: Allow v to be 0 or 1 for EIP1559 transactions

8168d3f

holgerd77 force-pushed the signature/eip1559 branch from 4953c7b to 8168d3f Compare May 30, 2022 11:25

holgerd77 previously approved these changes May 30, 2022

View reviewed changes

jochem-brouwer added PR state: WIP type: test all hardforks This special label enables VM state and blockchain tests for all hardforks on the respective PR. package: util labels May 30, 2022

jochem-brouwer added 2 commits May 30, 2022 21:41

vm: fix ecrecover precompile for v=0 and v=1

f2dbdaa

tx/legacyTransaction: add v guard for non-EIP155 txs

98e7f36

jochem-brouwer dismissed holgerd77’s stale review via 98e7f36 May 30, 2022 20:10

holgerd77 reviewed May 31, 2022

View reviewed changes

tx: switch to earlier v validation and throwing before v common/EIP-1…

a74572e

…55 determination logic

jochem-brouwer reviewed May 31, 2022

View reviewed changes

jochem-brouwer previously approved these changes May 31, 2022

View reviewed changes

jochem-brouwer dismissed their stale review via 5f6bcc4 May 31, 2022 14:20

tx: remove v==0 check which always defaults to the default common

5f6bcc4

ernestognw commented May 31, 2022

View reviewed changes

holgerd77 approved these changes May 31, 2022

View reviewed changes

holgerd77 merged commit 30ca12e into ethereumjs:master May 31, 2022

holgerd77 added PR state: needs review package: tx package: vm and removed PR state: WIP labels Jun 1, 2022

holgerd77 mentioned this pull request Jun 7, 2022

Develop rebased on 3e4e7be #1928

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Util: Allow `v` to be `0` or `1` for EIP1559 transactions #1905

Util: Allow `v` to be `0` or `1` for EIP1559 transactions #1905

ernestognw commented May 25, 2022

holgerd77 commented May 25, 2022

codecov bot commented May 25, 2022 •

edited

Loading

ernestognw commented May 25, 2022

holgerd77 left a comment

holgerd77 May 30, 2022

jochem-brouwer May 30, 2022

holgerd77 commented May 30, 2022

jochem-brouwer commented May 30, 2022

holgerd77 commented May 30, 2022

jochem-brouwer commented May 30, 2022 •

edited

Loading

jochem-brouwer commented May 30, 2022 •

edited

Loading

jochem-brouwer commented May 30, 2022

holgerd77 commented May 31, 2022

holgerd77 May 31, 2022

holgerd77 commented May 31, 2022

jochem-brouwer left a comment

jochem-brouwer May 30, 2022

jochem-brouwer May 30, 2022

jochem-brouwer May 31, 2022

holgerd77 May 31, 2022

jochem-brouwer May 31, 2022

jochem-brouwer left a comment

ernestognw commented May 31, 2022

ernestognw May 31, 2022

holgerd77 left a comment

Util: Allow v to be 0 or 1 for EIP1559 transactions #1905

Util: Allow v to be 0 or 1 for EIP1559 transactions #1905

Conversation

ernestognw commented May 25, 2022

holgerd77 commented May 25, 2022

codecov bot commented May 25, 2022 • edited Loading

Codecov Report

ernestognw commented May 25, 2022

holgerd77 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented May 30, 2022

jochem-brouwer commented May 30, 2022

holgerd77 commented May 30, 2022

jochem-brouwer commented May 30, 2022 • edited Loading

jochem-brouwer commented May 30, 2022 • edited Loading

jochem-brouwer commented May 30, 2022

holgerd77 commented May 31, 2022

Choose a reason for hiding this comment

holgerd77 commented May 31, 2022

jochem-brouwer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jochem-brouwer left a comment

Choose a reason for hiding this comment

ernestognw commented May 31, 2022

Choose a reason for hiding this comment

holgerd77 left a comment

Choose a reason for hiding this comment

Util: Allow `v` to be `0` or `1` for EIP1559 transactions #1905

Util: Allow `v` to be `0` or `1` for EIP1559 transactions #1905

codecov bot commented May 25, 2022 •

edited

Loading

jochem-brouwer commented May 30, 2022 •

edited

Loading

jochem-brouwer commented May 30, 2022 •

edited

Loading