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

Implement EIP-7805 (FOCIL) #8003

Draft
wants to merge 57 commits into
base: master
Choose a base branch
from
Draft

Implement EIP-7805 (FOCIL) #8003

wants to merge 57 commits into from

Conversation

Marchhill
Copy link
Contributor

@Marchhill Marchhill commented Jan 3, 2025 •
edited
Loading

Changes

  • Implement EIP-7805
  • engine api changes
    • implement engine_getInclusionList
    • add inclusion lists to PayloadAttributes and newPayload parameters
  • validate that valid IL txs were included up to gas limit (see here)
  • use IL in block building

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

Documentation

Requires documentation update

  • Yes
  • No

Requires explanation in Release Notes

  • Yes
  • No

@matthewkeil matthewkeil mentioned this pull request Jan 14, 2025
Closed
@Marchhill
fix engine rpc registration, fix old tests, add test
72204a1
@Marchhill
formatting
3789381
@Marchhill
change getInclusionList type to bytes
2f2abaf
@Marchhill
fix some tests
d7b3dab
@Marchhill
fix some things and add engineapi tests for inclusion lists
dd73442
@Marchhill
tidy
6cf18ad
@Marchhill
refactor inclusion list validation to block validator
99e0963
@Marchhill
add test for inclusion list validation, fix check if tx contained in …
c26ba5e 
…block
@Marchhill
improve efficiency of checking block tx hashes
da6e88f
@Marchhill
fix IL block production and add test
5e79e5b
@Marchhill
refactor tests and tidy
ebd456e
@Marchhill
use txpooltxsource for inclusion list
2300836
@Marchhill
whitespace
4791cb0
@Marchhill
Merge remote-tracking branch 'upstream/master' into feature/eip-7805
fd56978
@Marchhill
remove merge conflicts
274a563
@Marchhill
remove syncthreadtests
9d58825
@Marchhill
small test change
efecc21
@Marchhill
fix aura tests
acc500f
@Marchhill
move inclusion list validator to separate class
33b6412
@Marchhill
remove transactionProcessor from blockValidator
1a00e1c
@Marchhill
formatting
180f85a
@Marchhill
tidy up old tests
8afa7e6
LukaszRozmej
LukaszRozmej requested changes Feb 13, 2025
View reviewed changes
src/Nethermind/Nethermind.Consensus/Producers/BlockProducerEnvFactory.cs Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Producers/PayloadAttributes.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Transactions/InclusionListTxSource.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Validators/IInclusionListValidator.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Core/Block.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Handlers/EngineRpcCapabilitiesProvider.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Merge.Plugin/Handlers/GetInclusionListTransactionsHandler.cs Outdated
{
// todo: limit size of IL?
IEnumerable<Transaction> txs = inclusionListTxSource.GetTransactions(blockTree.Head!.Header, long.MaxValue);
byte[][] txBytes = [.. txs.Select(tx => Rlp.Encode(tx).Bytes)];
Copy link
Member

Choose a reason for hiding this comment

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

We could potentially use ArrayPoolList to avoid allocation, ResultWrapper should Dispose it correctly after being serialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we're returning the bytes I don't see that we can use ArrayPoolList here, maybe I'm missing something?

Copy link
Member

Choose a reason for hiding this comment

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

you can return ArrayPoolList<byte[]>, you could return ArrayPoolList<ArrayPoolList> even.

Also Rlp.Encode is potentially not the best either due to allocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I returned this then I would have to have ArrayPoolList<byte[]> as the return value in the engine api signature, this seems wrong to me, you think it's ok?

I don't know of a way to encode without allocating so if I did something like ArrayPoolList<ArrayPoolList<byte>> then I would have to allocate anyway and then use ToPooledList()

src/Nethermind/Nethermind.Merge.Plugin/Handlers/NewPayloadHandler.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Validators/InclusionListValidator.cs Outdated Show resolved Hide resolved
src/Nethermind/Nethermind.Consensus/Validators/InclusionListValidator.cs
continue;
}

bool couldIncludeTx = _transactionProcessor.BuildUp(tx, new(block.Header, spec), NullTxTracer.Instance);
Copy link
Member

Choose a reason for hiding this comment

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

Would be probably good to Snapshot and Revert WorldState?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it needs to be reset as there is this bit in the EIP:

Also note that we do not need to reset the state to S, since the only way for a transaction to alter the state is for it to execute successfully, in which case the block is invalid, and so the block will not be applied to the state.

Also with BuildUp the new state is uncommitted so should be reverted right?

Copy link
Member

Choose a reason for hiding this comment

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

Need to double check. Comment from EIP doesn't have to work well with Nethermind as we have a bit different way of operating on state.

Marchhill and others added 23 commits February 13, 2025 13:51
@Marchhill
limit IL size to 8kb
0339125
@Marchhill
avoid recalculating header hash
7ccde5e
@Marchhill
refactor and decode IL txs earlier
d5e7fad
@Marchhill
Merge remote-tracking branch 'upstream/master' into feature/eip-7805
86ea63a
@rubo
Fix formatting
b4a285f
@Marchhill
don't decode inclusion list again in txsource
8f97620
@Marchhill
cleanup IL validator
439e725
@Marchhill
IL satisfied if no gas for simple transfer
00fec89
@Marchhill
use txpooltxsource explicitly in GetInclusionListHandler
e1f004b
@Marchhill
max IL size 8192, use ILDecode for encoding, comment on ordering by p…
f4e0894 
…riority fee
@Marchhill
avoid allocating for creating IL trie, move constants to file
f1a359c
@Marchhill
make txpooltxsource nullable
8f1bb99
@Marchhill
add 7805 constants file
e2fae0c
@Marchhill
Merge remote-tracking branch 'upstream/master' into feature/eip-7805
4b7cfd1
@Marchhill
check if InclusionListTransactions null in payloadattributes
b038b4d
@Marchhill
fix payloadattributes syntax
b3d1c9b
@Marchhill
fix formatting
3c6f62a
@Marchhill
fix inclusion list satisfied test
54f3baa
@Marchhill
fit as many txs as possible into inclusion list
9b34e14
@Marchhill
use upper bound estimate for IL packing
e10df74
@Marchhill
rename getInclusionList to have V1
9fe0f74
@Marchhill
add dummy version updatePayloadWithInclusionList
7acb872
@Marchhill
add new file
e8080b9
@Marchhill Marchhill marked this pull request as draft February 18, 2025 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukaszRozmej LukaszRozmej LukaszRozmej requested changes

@MarekM25 MarekM25 Awaiting requested review from MarekM25

@marcindsobczak marcindsobczak Awaiting requested review from marcindsobczak

Requested changes must be addressed to merge this pull request.

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

Successfully merging this pull request may close these issues.
3 participants
@Marchhill @LukaszRozmej @rubo