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

anvil/eth: Use the raw v signature value instead of bool #7918

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

anvil/eth: Use the raw v signature value instead of bool #7918

wants to merge 3 commits into from

Conversation

ngotchac
Copy link
Contributor

Fixes #7898

Motivation

Cf. the linked issue. The v value (which should be deprecated for some tx types) was wrongly set as the y parity byte converted into a uint.

Solution

Instead of converting the boolean corresponding to the y parity byte to a u64/U256, use the raw v value for the v field when available.

mattsse
mattsse reviewed May 13, 2024
View reviewed changes
crates/anvil/core/src/eth/transaction/mod.rs Outdated
Comment on lines 404 to 410
fn v_parity_to_u64(v: Parity) -> u64 {
match v {
Parity::Eip155(v) => v,
Parity::NonEip155(b) => (b as u64) + 27,
Parity::Parity(b) => b as u64,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@prestwich is this something we should add as function instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

impl From<Parity> for u64 would be clean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be in alloy then, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, in this file

Copy link
Contributor Author

@ngotchac ngotchac May 14, 2024 •
edited

Choose a reason for hiding this comment

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

Actually, the method to_u64() already existed ; I completely missed it 🙈 Updated the PR!

@ngotchac
anvil/eth: Use the raw v signature value instead of bool
2091dd9 
Instead of converting the boolean corresponding to the y parity byte to
a u64/U256, use the raw `v` value for the `v` field when available.
@ngotchac
Copy link
Contributor Author

I added a 2nd commit that fixes invalid (from geth's PoV) dummy signatures. The issue was that it was using v == 0, while this snippet in geth'd code: https://github.com/ethereum/go-ethereum/blob/d2f00cb54edc4486314c25d9e6c5b739009c2201/core/types/transaction.go#L234-L238 was computing plainV = byte(v.Uint64() - 27) which made it invalid.

By using Parity::NonEip155(false), v is actually encoded as 27 instead of 0, which makes the validation pass.

@ngotchac
Copy link
Contributor Author

OK and one last commit, to actually use the provided signature when creating a transaction. This resolves an issue where the computed transaction hash in anvil (computed once, reused everywhere afterward) wouldn't match a computed hash from the transactions' details (eg. from a eth_getBlock call). In geth's Go library, a transaction hash is (almost) never parsed from the RPC response, but recomputed. This caused mismatches and all kinds of troubles.

This is a bit tangential to the initial issue, so I could if needed open another PR for it.

mattsse
mattsse approved these changes May 14, 2024
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

@mattsse
Copy link
Member

mattsse commented May 14, 2024

I think the failing tests are related

@ngotchac
Copy link
Contributor Author

ngotchac commented May 15, 2024 •
edited

I think the failing tests are related

You mean unrelated? The only failing ones are integration tests which pass after retry. My bad, I thought they were retried twice, but it seems the tests are just split in 3. I'll look into it.

@ngotchac
anvil/eth: Use proper Parity signature in impersonate
f608b70 
The correct `v` value must be used when using a dummy signature,
depending on the transaction type (the Legacy transactions being the
ones needing a special case).
@ngotchac
anvil/eth: Use proper signature for creating txs
a66a0cf 
This fixes wrong hash being computed for transactions.
@ngotchac
Copy link
Contributor Author

OK, I found the issue! So TIL, for all transactions that are not legacy, the v value for the signature should actually just be the parity byte (so 0 or 1). This means that the impersonate signature cannot be constant, but depends on the transaction type. I removed some code (2nd commit) that did seem really relevant, and simplified things so that the impersonate signature is creating within the api.rs file, dependent on the transaction type.

To see the issue, cf. the signature change in the failing test. The previous signature wasn't actually valid. This can be checked here, for example: https://toolkit.abdk.consulting/ethereum#rlp,transaction (the v value was set to 62710 while it was an EIP-1559 tx).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prestwich prestwich prestwich left review comments

@mattsse mattsse mattsse approved these changes

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

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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

eth_sendRawTransaction returns invalid transaction v, r, s values
3 participants
@ngotchac @mattsse @prestwich