handle reorgs for proposer preferences#16651
Conversation
Squashed cherry-pick of PR #16651 onto glamsterdam-devnet-2.
Squashed cherry-pick of PR #16651 onto glamsterdam-devnet-2.
| // NextSlotState may return a state at slot < boundarySlot when the | ||
| // dependent block was at an earlier slot (empty boundary slot); only use it | ||
| // if it lands exactly on the boundary, otherwise fall through to load+advance. | ||
| if cached := transition.NextSlotState(dependentRoot[:], boundarySlot); cached != nil && cached.Slot() == boundarySlot { |
There was a problem hiding this comment.
for reviewer please take a look at this to see if it makes sense, this is for getting proposer preference messages at the epoch boundary when the head state could still be behind so we need to use the next slot state cache
| // state.block_roots[start_slot(epoch(slot)-1) - 1]. Returns | ||
| // ErrProposerDependentRootUnderflow when the proposal epoch is < 2; the spec's | ||
| // fallback to the genesis block root is the caller's responsibility. | ||
| func ProposerDependentRoot(st state.ReadOnlyBeaconState, slot primitives.Slot) ([32]byte, error) { |
There was a problem hiding this comment.
why not make this a state getter like st.ProposerDependentRoot? you are accessing state.BlockRootAtIndex at the end anyway
There was a problem hiding this comment.
thanks for the suggestion, made it a state getter in 9e3126f
| return cache.TrackedValidator{}, false | ||
| } | ||
| var feeRecipient primitives.ExecutionAddress | ||
| copy(feeRecipient[:], pref.FeeRecipient) |
There was a problem hiding this comment.
do we really need to copy here?
| // trackedProposer now anchors preferences on dependent_root derived from the | ||
| // passed state (state.block_roots lookup). At slot 0 the lookup underflows so | ||
| // proposerPreference falls back to the no-cache path; cached-preference | ||
| // behavior is exercised end-to-end by the gossip and bid validation tests | ||
| // under beacon-chain/sync. |
There was a problem hiding this comment.
are those comments helpful? they seem like mostly used by AI
There was a problem hiding this comment.
no its not sorry that was a miss on personal review , fixed in 9e3126f
| dependentRoot [32]byte, | ||
| slot primitives.Slot, | ||
| validatorIndex primitives.ValidatorIndex, | ||
| feeRecipient []byte, | ||
| gasLimit uint64, |
There was a problem hiding this comment.
that's enough arguments should we just pass in the preference
| ) | ||
| } | ||
|
|
||
| valIdx := msg.Message.ValidatorIndex |
There was a problem hiding this comment.
move this next to L94?
| var dependentRoot [fieldparams.RootLength]byte | ||
| copy(dependentRoot[:], msg.Message.DependentRoot) |
| epoch := slots.ToEpoch(slot) | ||
| if epoch < 2 { | ||
| root, err := s.cfg.beaconDB.GenesisBlockRoot(ctx) | ||
| if err != nil { | ||
| return [32]byte{}, errors.Wrap(err, "genesis block root") | ||
| } | ||
| return root, nil | ||
| } |
There was a problem hiding this comment.
can we add this to DependentRootForEpoch rather than doing it for all the callers?
|
|
||
| dependentRoot := bytesutil.ToBytes32(signedPreferences.Message.DependentRoot) | ||
| // [IGNORE] block with root preferences.dependent_root has been seen. | ||
| if !s.cfg.chain.InForkchoice(dependentRoot) && !s.cfg.beaconDB.HasBlock(ctx, dependentRoot) { |
There was a problem hiding this comment.
why put this here rather than part of verifier package.. and we can reuse similar method there
| // NextSlotState may return a state at slot < boundarySlot when the | ||
| // dependent block was at an earlier slot (empty boundary slot); only use it | ||
| // if it lands exactly on the boundary, otherwise fall through to load+advance. | ||
| if cached := transition.NextSlotState(dependentRoot[:], boundarySlot); cached != nil && cached.Slot() == boundarySlot { |
There was a problem hiding this comment.
why use the next slot state cache here, it will surely be a misse right because we are getting an old boundary slot?
There was a problem hiding this comment.
i tried to isolate it more, this is for the case of getting the a proposer preference for the next epoch when our head is not up to date for slot 0 , also a part of 9e3126f
What type of PR is this?
Bug fix
What does this PR do? Why is it needed?
The proposer preference cache was keyed by slot only (
map[Slot]ProposerPreference) with first-write-wins semantics. During a reorg that changes the proposer shuffling (different RANDAO at epoch boundary), a new proposer's preferences for the same slot would be rejected because the slot already had an entry. This caused:Makes the proposer preferences cache reorg-safe by re-keying from
slotto(slot, validatorIndex). After a reorg that changes the proposer shuffling, the correct proposer's preferences are resolved dynamically from the head state's proposer lookahead.Reproducing reorg testing with Kurtosis
To validate this change under real reorgs, add a temporary broadcast delay flag that is NOT part of this PR. Apply the following patch locally, build new images, and run with the kurtosis config below.
1. Patch
config/features/flags.goAdd a flag variable before
DisableDutiesV2:Add
reorgTestBroadcastDelay,to theBeaconChainFlagsslice.2. Patch
config/features/config.goAdd a field to the
Flagsstruct:Add to
ConfigureBeaconChain()beforeInit(cfg):3. Patch
beacon-chain/rpc/prysm/v1alpha1/validator/proposer.goAdd
"github.com/OffchainLabs/prysm/v7/config/features"to imports.Replace
broadcastReceiveBlockwith:4. Kurtosis config (
gloas-config-4node-reorg.yml)5. Run and monitor
With 4-second slots and a 5-second delay, the delayed nodes' blocks arrive after the next slot begins, reliably triggering reorgs (depth 2-3).
Logs to look for
Chain reorg occurredblockchainNew proposer preference for slot that already has a different validator (possible reorg)cacheProcessed signed proposer preferencesrpc/validatorWhich issues(s) does this PR fix?
Fixes ##16616
related specs
ethereum/consensus-specs#5196
ethereum/consensus-specs#5190
ethereum/consensus-specs#5191
Other notes for review
Acknowledgements