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

Add RLP encoding and hashing for Blob txs #5593

Merged
merged 17 commits into from
Jun 2, 2023
Merged

Add RLP encoding and hashing for Blob txs #5593

merged 17 commits into from
Jun 2, 2023

Conversation

flcl42
Copy link
Contributor

@flcl42 flcl42 commented Apr 19, 2023 •
edited
Loading

Encoding for shard blob transactions

Changes

  • Add SSZ RLP encoding and hashing for blob tx

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
  • Other: Description

Testing

Requires testing

  • Yes
  • No

If yes, did you write tests?

  • Yes
  • No

Notes on testing

Requires Cancun to be turned on for tests, #4558 has some help for that

@flcl42 flcl42 mentioned this pull request Apr 19, 2023
Closed
@flcl42 flcl42 force-pushed the feature/eip-4844-ssz branch 8 times, most recently from 29a01a3 to bb81773 Compare April 21, 2023 13:31
@flcl42 flcl42 marked this pull request as ready for review April 21, 2023 13:43
@flcl42 flcl42 force-pushed the feature/eip-4844-ssz branch from 3801dd6 to 47041d1 Compare April 26, 2023 11:19
@flcl42 flcl42 force-pushed the feature/eip-4844-jsonrpc branch from c27b96e to 4d89363 Compare April 26, 2023 12:34
@flcl42 flcl42 force-pushed the feature/eip-4844-ssz branch from 47041d1 to a675858 Compare April 26, 2023 12:46
@flcl42 flcl42 force-pushed the feature/eip-4844-jsonrpc branch 3 times, most recently from 1147355 to 6cd7555 Compare April 26, 2023 14:41
Base automatically changed from feature/eip-4844-jsonrpc to master April 26, 2023 17:46
@flcl42 flcl42 force-pushed the feature/eip-4844-ssz branch 4 times, most recently from 3ccb719 to 87c9443 Compare April 27, 2023 10:55
@flcl42 flcl42 force-pushed the feature/eip-4844-ssz branch from 87c9443 to a3bab50 Compare April 28, 2023 10:27
@flcl42 flcl42 requested a review from marcindsobczak May 4, 2023 12:47
marcindsobczak
marcindsobczak approved these changes May 9, 2023
View reviewed changes
Copy link
Contributor

@marcindsobczak marcindsobczak left a comment

Choose a reason for hiding this comment

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

Looks good - great, amazing work!

@flcl42 flcl42 marked this pull request as draft May 16, 2023 12:46
@flcl42 flcl42 changed the title Add SSZ encoding and hashing for Blob txs Add RLP encoding and hashing for Blob txs May 24, 2023
@flcl42 flcl42 force-pushed the feature/eip-4844-ssz branch 2 times, most recently from 1101fee to 97fdab9 Compare May 24, 2023 11:12
@flcl42 flcl42 force-pushed the feature/eip-4844-ssz branch from 97fdab9 to 11d43c4 Compare May 24, 2023 12:29
@flcl42 flcl42 force-pushed the feature/eip-4844-ssz branch from 3c1748e to 7ba30a6 Compare May 24, 2023 15:26
@flcl42 flcl42 marked this pull request as ready for review May 24, 2023 15:34
MarekM25
MarekM25 reviewed May 29, 2023
View reviewed changes
Copy link
Contributor

@MarekM25 MarekM25 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 great to verify unit test coverage for TxDecoder and TxValidator. Other than that I added minor comments and I did extremely cosmetic changes in one commit

src/Nethermind/Nethermind.Consensus/Validators/TxValidator.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Core.Test/AssertionsSetup.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Core/Transaction.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Crypto/TransactionExtensions.cs Show resolved Hide resolved
src/Nethermind/Nethermind.JsonRpc/Data/TransactionForRpc.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Serialization.Rlp/BlockDecoder.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs
int networkWrapperCheck = 0;
if ((rlpBehaviors & RlpBehaviors.InNetworkForm) == RlpBehaviors.InNetworkForm && transaction.HasNetworkForm)
{
int networkWrapperLength = decoderContext.ReadSequenceLength();
Copy link
Contributor

Choose a reason for hiding this comment

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

I put breakpoint here and I ran TxDecoder tests - looks like it wasn't covered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ShardBlobTxDecoderTests is another piece that tests decoding, however negative cases are not covered by it

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right - it is covered!

src/Nethermind/Nethermind.Serialization.Rlp/TxDecoder.cs
transaction.ChainId = rlpStream.DecodeULong(allowLeadingZeroBytes: false);
transaction.Nonce = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false);
transaction.GasPrice = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false); // gas premium
transaction.DecodedMaxFeePerGas = rlpStream.DecodeUInt256(allowLeadingZeroBytes: false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I put breakpoint here and I ran TxDecoder tests - looks like it wasn't covered by tests

@flcl42 flcl42 mentioned this pull request May 31, 2023
Merged
12 tasks
@MarekM25
Copy link
Contributor

MarekM25 commented Jun 1, 2023

Amazing work - approved!

@MarekM25 MarekM25 self-requested a review June 1, 2023 08:26
LukaszRozmej
LukaszRozmej reviewed Jun 1, 2023
View reviewed changes
src/Nethermind/Nethermind.Core/Transaction.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Core/Transaction.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Crypto/KzgPolynomialCommitments.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Data/BlobsBundleV1.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Serialization.Rlp/RlpBehaviors.cs
/// Mempool form: TX_TYPE || [[chain_id, nonce, ...], <mempool wrapper fields> ].
/// See https://eips.ethereum.org/EIPS/eip-4844#networking
/// </summary>
InMempoolForm = 64,
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to have separated TxDecoders for MemPool form? We are already too deep into parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You need to consider wrapping and detect tx type before the decoding, a separated decoding seems to require code duplication with no big profit. Have no better idea for now yet

Copy link
Member

Choose a reason for hiding this comment

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

Can we avoid code duplication with base abstract class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can try to #5753 Refactor transaction decoder in the future

@flcl42 flcl42 merged commit 9dd93df into master Jun 2, 2023
@flcl42 flcl42 deleted the feature/eip-4844-ssz branch June 2, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benaadams benaadams benaadams left review comments

@marcindsobczak marcindsobczak marcindsobczak approved these changes

@LukaszRozmej LukaszRozmej LukaszRozmej approved these changes

@MarekM25 MarekM25 MarekM25 approved these changes

@rubo rubo Awaiting requested review from rubo

@Demuirgos Demuirgos Awaiting requested review from Demuirgos

Assignees
No one assigned
Labels
eip-4844
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@flcl42 @MarekM25 @benaadams @LukaszRozmej @marcindsobczak