Don't restart non-validators that are behind - #475
Conversation
| onEpochChange: func(epoch uint64, validators common.Nodes) error { | ||
| height := i.Config.PlatformChain.GetCurrentHeight() | ||
| vdrs, err := i.Config.PlatformChain.GetValidatorSet(height) | ||
| // set the communication to the highest validator set, since this node is a non-validator and may be behind. |
There was a problem hiding this comment.
is this basically a poor man's caching of the highest validator set from the P-chain?
There was a problem hiding this comment.
it uses the same mechanism as before, i just extracted it to a helper.
But yea, we should feed the non-validator the highest pchain validators. I think alternatively we could just pass in the platform chain interface into the non-validator. This could help if the non-validator was offline and the validator set diverged.
There was a problem hiding this comment.
Good point 👍 if the non-validator was offline and the validator set has drifted away, it may still be able to replicate the P-chain and get the latest validator set.
| switch epochChange.nodeRole { | ||
| case nonValidator: | ||
| switch { | ||
| case i.e != nil && i.nv != nil: |
There was a problem hiding this comment.
we removed the role but can we at least put some more meaningful code than i.e != nil and i.nv != nil?
Maybe:
amValidator := i.e != nil
amNonValidator := i.nv != nil
?
Signed-off-by: Sam Liokumovich <65994425+samliok@users.noreply.github.com>
There was a problem hiding this comment.
I am not sure this file belongs to the PR
| default: | ||
| // The slot holds a stale epoch change: take it and keep the newer of the two. | ||
| select { | ||
| case pending := <-i.epochChanges: |
| require.ErrorContains(t, inst.Start(t.Context()), "instance already started") | ||
| } | ||
|
|
||
| // currentNonValidator returns the non-validator the instance is running, or nil if it |
There was a problem hiding this comment.
this is a weird thing to do. Can't we achieve the same thing in a different way?
| case <-i.stopCh: | ||
| return | ||
| default: | ||
| // The slot holds a stale epoch change: take it and keep the newer of the two. |
There was a problem hiding this comment.
I don't understand why we can't just wait until the goroutine has picked up the epoch change from the channel?
This way, we will never move on to higher epochs.
There was a problem hiding this comment.
so we have a suppeerrr annoying deadlock here if we don't do something along these lines.
Essentially the index from both nonvalidator and epoch run under their own lock. So for example, when we are indexing a sealing block as a non-validator the non-validator lock is being held. Then we try to send on this channel, but it will block until the consumer can process it.
But say we have been indexing sealing blocks very fast into storage(i.e in the tests). There may be a scenario where the consumer goroutine(listenForEpochChanges), is already processing an epoch change. So we have sent an epoch change, and the channel is now full.
Now imagine immediately after we index another sealing block. The channel is now full, so our non-validator blocks.
The goroutine that is processing an epoch change may call back into the non-validator and request the non-validator lock(ex. HighestValidatedEpoch).
This means we are deadlocked because the index goroutine can only let go of the lock if the epoch change channel is empty, but in order for the epoch change channel to get empty it needs to acquire the lock.
| } | ||
|
|
||
| // The epoch change is acted on by another goroutine, so the node has to be given the | ||
| // chance to restart before concluding that it did not. |
There was a problem hiding this comment.
before concluding it did not what?
|
|
||
| // sealingBlockAt builds the sealing block of epoch `epoch` at sequence `seq` | ||
| func sealingBlockAt(seq, epoch uint64, validators metadata.NodeBLSMappings) *ParsedBlock { | ||
| md := common.ProtocolMetadata{Epoch: epoch, Round: seq, Seq: seq} |
There was a problem hiding this comment.
what about the prev hash pointer? i guess this works because we replicate it and we don't check the prev pointer because we have a QC?
2 participants
nodeRolestruct