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

Remove Data field from serialized Transactions #6079

Merged
merged 11 commits into from
Sep 11, 2023
Merged

Conversation

emlautarom1
Copy link
Contributor

@emlautarom1 emlautarom1 commented Sep 6, 2023

Partially addresses #5161

Changes

  • Do not serialize the Data field, but use it as alias of Input when deserializing.

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

  • Test suite has been updated to reflect the changes
  • Tested syncing Sepolia with Prysm v4.0.8

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

Remarks

While working on #5161 we found that the Data field is not part of the spec (https://ethereum.github.io/execution-apis/api-documentation/), so we removed it on #6036.

As documented on that PR, this could introduce issues with users that relied on this non-conformant field. During internal testing, we found that the Prysm CL relied on this so we rolled back the change on #6067.

After discussing it with the team, we decided to compromise in the following fashion: we will accept Data as an alias for Input when reading transactions from other applications, but we will not include such field when sending them.

// Accept during deserialization, ignore during serialization
// See: https://github.com/NethermindEth/nethermind/pull/6067
[JsonProperty(nameof(Data))]
private byte[]? Data { set { Input = value; } }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "trick" used to support deserialization but ignore serialization is described here: https://stackoverflow.com/questions/43714050/multiple-jsonproperty-name-assigned-to-single-property/43715009#43715009

Copy link
Contributor

@smartprogrammer93 smartprogrammer93 left a comment

Choose a reason for hiding this comment

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

I like this way of doing it

@LukaszRozmej
Copy link
Member

Context: ethereum/go-ethereum#28077

Copy link
Member

@benaadams benaadams left a comment

Choose a reason for hiding this comment

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

Would be good for me if this was merged sooner rather than later; as will be additional merge conflicts 😅

@emlautarom1 emlautarom1 merged commit 5a2f2ff into master Sep 11, 2023
61 checks passed
@emlautarom1 emlautarom1 deleted the fix/tx_data_field branch September 11, 2023 13:37
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