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

When on PoS the head can be only be updated by ForkchoiceUpdate #3994

Closed
wants to merge 5 commits into from

Conversation

fab-10
Copy link
Contributor

@fab-10 fab-10 commented Jun 20, 2022

When executing a newPayload or build a new block, we do not need to move
the chain head and update the world state, since this will be done by a
following forkchoice update call, but we still need to validate the block
and doing so we can also prepare everything for the future call, so we do
not need to re-execute everything, but only update the pointers, so that the
response to the forkchoice update call is quick.

Moreover on block proposal do only a validation of the block without storing or remembering nothing, to avoid saving data that could be stale, for example the empty block that is always proposed, is useless if another better block is produced later.
Signed-off-by: Fabio Di Fabio fabio.difabio@consensys.net

PR description

Fixed Issue(s)

fixes #3957

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@fab-10 fab-10 force-pushed the newPayload_wo_changing_head branch from d34b222 to 5c2fc61 Compare June 20, 2022 13:44
@fab-10 fab-10 changed the title Do not update chain state and world state in MergeCoordinator.execute… Do not update chain state and world state in MergeCoordinator.executeBlock Jun 20, 2022
@fab-10 fab-10 changed the title Do not update chain state and world state in MergeCoordinator.executeBlock When on PoS the head can be only be updated by ForkchoiceUpdate Jun 20, 2022
@fab-10 fab-10 marked this pull request as ready for review June 20, 2022 15:30
@fab-10 fab-10 force-pushed the newPayload_wo_changing_head branch 2 times, most recently from 6785b1f to a939dd2 Compare June 20, 2022 17:01
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

Question about HEAD_ADVANCED event types, but this looks great.

@@ -236,6 +237,35 @@ protected Hash calculateRootHash(
return Hash.wrap(rootHash);
}

@Override
public void remember(final BlockHeader blockHeader) {
Copy link
Contributor

Choose a reason for hiding this comment

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

cool name

public void remember(final BlockHeader blockHeader) {
checkArgument(blockHeader != null, "Block header must not be null");

final BonsaiWorldStateUpdater localUpdater = updater.copy();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be of interest to @gezero

Copy link
Contributor

Choose a reason for hiding this comment

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

The copy method will disappear, I will see this as fail of build when I will try to merge.

@@ -34,4 +34,15 @@ public interface MutableWorldState extends WorldState, MutableWorldView {
* `null` should be passed in.
*/
void persist(BlockHeader blockHeader);

/**
* Remember accumulated changes to underlying storage for future use, without making this the
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

updateCacheForNewCanonicalHead(block, td);
final BlockAddedEvent blockAddedEvent;
if (storeOnly) {
blockAddedEvent = handleStoreOnly(blockWithReceipts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean that if/when we "rewind" to this future block in forkchoiceUpdated that we are not going to fire a an event of type HEAD_ADVANCED anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! Indeed this need to be reviewed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@garyschulte please check now, this commit
Introduce forwardToBlock

It moves head to the block sent by a previous newPayload, and triggers
the advanced head event that will also update the Bonsai world state.

@fab-10 fab-10 force-pushed the newPayload_wo_changing_head branch from a939dd2 to b02de74 Compare June 22, 2022 12:03
…Block

When executing a newPayload or build a new block, we do not need to move
the chain head and update the world state, since this will be done by a
following forkchoice update call, but we still need to validate the block
and doing so we can also prepare everything for the future call, so we do
not need to re-execute everything, but only update the pointers, so that the
response to the forkchoice update call is quick.
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
… nothing

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
It moves head to the block sent by a previous newPayload, and triggers
the advanced head event that will also update the Bonsai world state.

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
@fab-10 fab-10 force-pushed the newPayload_wo_changing_head branch from b02de74 to 964d2d9 Compare June 23, 2022 07:59
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@fab-10
Copy link
Contributor Author

fab-10 commented Jun 24, 2022

Thanks @garyschulte for the review, I am closing this, since it is now part of this one #4013, that contains also other changes to avoid persisting state when preparing new payloads.

@fab-10 fab-10 closed this Jun 24, 2022
@fab-10 fab-10 deleted the newPayload_wo_changing_head branch November 8, 2022 13:29
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.

eth_getBlockByNumber should support latest as fcu head
3 participants