Consensus pushes finality data to Execution. Execution sets finalized block in blockchain #2936
Conversation
diegoximenes
changed the title
diegoximenes
force-pushed
the
push_safe_finalized
branch
2 times, most recently
from
f57d7be to
cbc3407
Compare
diegoximenes
requested review from
eljobe,
ganeshvanahalli and
Tristan-Wilson
|
As a suggestion from Tsahi, I will include the behavior implemented in #2927 in this PR |
diegoximenes
force-pushed
the
push_safe_finalized
branch
from
2adbc2c to
c5f1cf8
Compare
diegoximenes
changed the title
diegoximenes
changed the title
| type ConsensusExecutionSyncer struct { | ||
| stopwaiter.StopWaiter | ||
|
|
||
| config func() *ConsensusExecutionSyncerConfig |
There was a problem hiding this comment.
config functions are generally only needed for hot-configurable configs.
Here we don't have any, so it's fine to just hold a pointer to a config instead of config-func
| log.Warn("Finality not supported so not setting finalized block number") | ||
| } else if err == nil { | ||
| err = s.exec.SetFinalized(finalizedBlockNumber) | ||
| func (s *SyncMonitor) SetFinalityData(ctx context.Context, finalityData *arbutil.FinalityData) error { |
There was a problem hiding this comment.
separate PR:
I'm thinking about two contradicting options:
- don't pass valid as part of finality data, only pass finalized & safe, let consensus side implement logic to limit finalized & safe by valid
- do pass final, safe & valid, use valid to also replace "MarkValid" from the blockRecorder.
Not sure yet which makes more sense, specifically for scenario with multiple execution engines
There was a problem hiding this comment.
I decided to pass valid from consensus to execution because FinalizedBlockWaitForBlockValidator and SafeBlockWaitForBlockValidator configs are defined in the execution side.
So to implement option 1 we will need to do a breaking change in that regard, i.e., move these configs to the consensus side.
There was a problem hiding this comment.
makes sense. We may change it in the future, but it's a good deciding factor for now.
| type FinalityData struct { | ||
| FinalizedMsgCount MessageIndex | ||
| SafeMsgCount MessageIndex | ||
| ValidatedMsgCount *MessageIndex |
There was a problem hiding this comment.
separate PR: we should also have block-hashes for all of these, to make sure weird race conditions don't make us finalize a reorged-out block
There was a problem hiding this comment.
This possible inconsistency is more likely to happen with safe blocks than finalized.
It seems that geth doesn't update safe and finalized blocks in case of reorgs BTW.
ConsensusExecutionSyncer will periodically push finality data to execution, so this inconsistency will eventually disappear.
But it makes sense to add those hashes that you mentioned, considering that SetSafe and SetFinalizes will be called holding ExecutionEngine.createBlocksMutex.
I will create a linear task to handle those issues.
diegoximenes
dismissed stale reviews from tsahee and ganeshvanahalli
via
16a53fc
…id inconsistencies
diegoximenes
force-pushed
the
push_safe_finalized
branch
from
16a53fc to
b6ef1a4
Compare
| @@ -263,6 +263,7 @@ type NodeBuilder struct { | |||
| withProdConfirmPeriodBlocks bool | |||
| wasmCacheTag uint32 | |||
| delayBufferThreshold uint64 | |||
| useFreezer bool | |||
There was a problem hiding this comment.
separate PR: is there actually a good reason to ever set that to false? If not, can remove the config.
Resolves NIT-2627 and NIT-2585.
Today execution polls finality data from consensus.
With this PR consensus pushes finality data to execution, which avoids a circular dependency between consensus and simple execution (execution without sequencer behavior, for example).
Also, with this PR Execution starts setting the finalized block into the blockchain.
This enables go-ethereum to move blocks from the fast database to the ancients only if they are finalized, or if they are older than
HEAD-params.FullImmutabilityThreshold.params.FullImmutabilityThresholdis set to 90000 by go-ethereum today.Pulls OffchainLabs/go-ethereum#411