-
Notifications
You must be signed in to change notification settings - Fork 21.2k
Block access list changes - BAL construction, execution and validation #32263
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
base: master
Are you sure you want to change the base?
Conversation
A lot of the comments in the code are straight-up wrong/misleading. There's a lot of notes/reminders I made as I was implementing this, and not all of them have been removed/corrected at this point. |
I've pushed parallel execution changes here. Still failing 2 tests (other than modexp repricing ones which will be fixed with a rebase on master):
I've been trying to debug these but it's proving to be exceedingly difficult with the parallel execution enabled. The tests seem to relate to behavior of some system contracts on the fork boundaries, so I will just proceed with gathering numbers on mainnet performance for the meantime. |
ab1c562
to
8f200db
Compare
Broke the block count on the insertion log statement here. Will try to fix tomorrow. |
Getting some empty accounts in the BAL. example:
|
869b300
to
e14a463
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far, just a few comments.
var resCh chan *ProcessResultWithMetrics | ||
resCh, err = bc.processor.ProcessWithAccessList(block, statedb, bc.cfg.VmConfig) | ||
if err != nil { | ||
// TODO: okay to pass nil here as execution result? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine, we just use it to get out the receipts (if it is non nil).
We decided on the EIP breakout call to keep account/storage reads in for now. Basically, without having this information it is possible that a transaction can be crafted which does a ton of storage reads and slow down the processing of the block significantly. So until we can evaluate worst-cases here, account/storage reads are staying in the spec. |
bc.prefetcher.Prefetch(block, throwaway, vmCfg, &interrupt) | ||
if block.Body().AccessList == nil { | ||
// only use the state prefetcher for non-BAL blocks. | ||
bc.prefetcher.Prefetch(block, throwaway, vmCfg, &interrupt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I previously implemented a state prefetcher for BALs. I deleted it so that the import benchmarks I am running will capture the BAL block processing speed equivalent to a synced node executing blocks at chain head.
We should probably add back the BAL prefetcher and measure the import performance.
Also TODO: need to implement validation of storage/account reads against the BAL. |
// The tx context and all occurred logs in the scope of transaction. | ||
thash common.Hash | ||
txIndex int | ||
sender common.Address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove trhis.
// The tx context and all occurred logs in the scope of transaction. | ||
thash common.Hash | ||
txIndex int | ||
sender common.Address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover, remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be committed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's leftover test data for a unit test that no longer exists (but should be updated and added back in).
…hen enabled, post-Cancun blocks which lack access lists will have them constructed on execution during import. When importing blocks which contain access lists, transaction execution and state root calculation is performed in parallel.
7d3b230
to
93b4210
Compare
…slice before decoding in decoder impl.
- Add BlockAccessList to ExecutableData with custom JSON marshaling - Calculate BlockAccessListHash when constructing blocks - Fix WithWitness to preserve accessList field - Accept Amsterdam in ForkchoiceUpdatedV3
Use PayloadV4 after Amsterdam fork
WIP. currently passes blockchain spec tests (tests that make use of balances >16 bytes are excluded).
Change summary:
--experimentalbal
is enabled:Deviations from EIP-7928:
txindex=0
correspond to state reads/changes which occur from system contract execution before transactions.txindx=1..len(block.transactions)
correspond to state reads/changes occurring for each transaction.txindex=len(block.transactions)+1
correspond to the post-block system contracts and EIP-4895 withdrawals.TODO: