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

core/types: add derived chain ID to LegacyTx JSON encoding #27452

Merged
merged 5 commits into from Jun 13, 2023

Conversation

freeman-jiang
Copy link
Contributor

When marshalling a legacy type transaction to JSON, the chain ID field is always omitted, despite the fact that it can be derived using the signature values after EIP-155.

This change adds the chain ID to a legacy transaction's JSON encoding when it can be derived.

If the transaction is not signed or is pre EIP-155, the chain ID will be omitted.

Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Couple nits, but I think this change is okay. The api spec currently has chainid as an optional field for transaction objects. So this will still be within spec. Since it is only adding a field, it should be backward compatible.

core/types/transaction_marshalling.go Outdated Show resolved Hide resolved
core/types/transaction_test.go Outdated Show resolved Hide resolved
@lightclient
Copy link
Member

Adding to triage. Main question I have is now legacy transactions don't check if the ChainID field matches the signature. I'm not sure if it is worth adding this check, but wanted to make a note of it.

@fjl fjl removed the status:triage label Jun 13, 2023
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

LGTM, if this feature is something we want

@fjl fjl merged commit 8bbaf88 into ethereum:master Jun 13, 2023
1 of 2 checks passed
@fjl fjl added this to the 1.12.1 milestone Jun 13, 2023
yperbasis added a commit to ledgerwatch/erigon that referenced this pull request Jun 14, 2023
For legacy transactions ChainID is optional (missing in
pre-[EIP155](https://eips.ethereum.org/EIPS/eip-155) transactions) and
is derived from `V` anyway.

Also, cherry pick ethereum/go-ethereum#27452.
antonydenyer pushed a commit to antonydenyer/go-ethereum that referenced this pull request Jul 28, 2023
…27452)


Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
MoonShiesty pushed a commit to MoonShiesty/go-ethereum that referenced this pull request Aug 30, 2023
…27452)


Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
…27452)


Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
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.

None yet

4 participants