-
Notifications
You must be signed in to change notification settings - Fork 307
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
Process execution payload #9112
base: focil
Are you sure you want to change the base?
Conversation
* add missing test spec config
* fix spec config
* fix Focil spec loader
* fix acceptance tests
* add inclusion list containers
* add signed inclusion list gossip layer
* add inclusion list pool skeleton
* Merge branch master
* implement inclusion list by committee indices rpc logic
…fy-new-payload # Conflicts: # ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/inclusionlist/InclusionListManager.java
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
do you really want to merge this on master? :) |
@@ -60,7 +63,8 @@ BeaconState processAndValidateBlock( | |||
SignedBeaconBlock signedBlock, | |||
BeaconState blockSlotState, | |||
IndexedAttestationCache indexedAttestationCache, | |||
Optional<? extends OptimisticExecutionPayloadExecutor> payloadExecutor) | |||
Optional<? extends OptimisticExecutionPayloadExecutor> payloadExecutor, | |||
Function<SlotAndBlockRoot, Optional<List<InclusionList>>> inclusionListSupplier) |
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.
Is there a reason for using a supplier here? We could directly get the ILs as Optional<List<InclusionList>>
from the store since we have the slot and block root when running a fork choice on a block (the ForkChoice::onBlock
has the slot and the block root
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.
So my choice to use the supplier here was more in terms of making sure that we only try to fetch the IL when we're assembly the payload, but I think at this stage the block has been already formed so yeah we can probably just retrieve the IL at ForkChoice::onBlock
. Thanks for the review
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.
LGTM
Just a nit regarding the supplier choice
…using supplier Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
# Conflicts: # ethereum/spec/src/main/java/tech/pegasys/teku/spec/logic/versions/eip7805/block/BlockProcessorEip7805.java
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
Signed-off-by: Gabriel Fukushima <[email protected]>
PR Description
This PR was built on top of #9111
Adds a new validation to the ProcessExecutionPayload to ensure that the number of tx in the IL in no larger than the number defined in the spec.
Fixed Issue(s)
Part of #8937
Documentation
doc-change-required
label to this PR if updates are required.Changelog