Simplex orchestration layer preliminary implementation#427
Conversation
ed41178 to
55576be
Compare
yacovm
changed the title
samliok
left a comment
samliok
left a comment
There was a problem hiding this comment.
leaving what i have, was not able to review as much as i hoped today
| // P-chain height referenced by the last block in the chain and until the provided P-chain height. | ||
| hasValidatorSetChanged func(pChainHeight uint64) (bool, NodeBLSMappings, error) | ||
| getPChainHeight func() uint64 | ||
| getPChainHeight func(ctx context.Context) (uint64, error) |
There was a problem hiding this comment.
are these changes important to the orchestrator? if not can we separate them?
There was a problem hiding this comment.
| } | ||
|
|
||
| // mockBlockBuilder is a test BlockBuilder whose BuildBlock returns a preconfigured block/error. | ||
| type mockBlockBuilder struct { |
There was a problem hiding this comment.
i think the use of the word mock will be triggering to some members of the avalanchego team
1bfcbc4 to
5eeffd9
Compare
This commit contains an implementation of an orchestration layer that orchestrates
epoch transition for validators and non-validators as they move through epochs.
The orchestration layer introduces an Instance that drives the epoch lifecycle,
switching between validator consensus and non-validator block tracking as the
node's role changes across epochs.
Key pieces:
- Instance: manages the per-epoch lifecycle, ticks the active epoch or
non-validator, and handles the handoff at epoch boundaries.
- epochChangeSupression: drops outgoing messages and cancels VM operations
while an epoch change is in progress, preventing side effects mid-transition.
- EpochAwareStorage: ignores blocks from previous epochs and signals when a
new epoch is detected.
- Config: wiring for the P-chain, storage, crypto, and networking dependencies.
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
| ) | ||
|
|
||
| const ( | ||
| DefaultMaxSequenceWindow = 100 |
There was a problem hiding this comment.
i think this should be a similar value to DefaultMaxRoundWindow, if not the same. Mainly because validators will reject replication that have more seqs than MaxRoundWindow.
BTW, I think we should change how we process replication requests, to not drop messages that request too many sequences but rather fill up the response with at most MaxRoundWindow. Also, we should ensure this message does not exceed the maximum number of bytes AvalancheGo clients can accept.
There was a problem hiding this comment.
Yeah good catch, I removed and changed it here.
| // GetBlock retrieves the block and finalization at [seq] with the given digest. | ||
| // If the digest is nil, the block with the given sequence number is returned. | ||
| // If [seq] the block cannot be found, returns ErrBlockNotFound. | ||
| GetBlock(seq uint64) (metadata.StateMachineBlock, *common.Finalization, error) |
There was a problem hiding this comment.
can we use the same terminology as simplex.Storage? maybe we can embed simplex.Storage in this API?
type Storage interface {
simplex.Storage
// CreateWAL creates a new Write-Ahead Log (WAL).
CreateWAL() (wal.DeletableWAL, error)There was a problem hiding this comment.
why do couple the WAL with the storage anyways?
There was a problem hiding this comment.
can we use the same terminology as simplex.Storage? maybe we can embed simplex.Storage in this API?
We can't, Retrieve conflicts because the parameters are different.
./instance.go:468:12: cannot use i.cs (variable of type *CachedStorage) as Storage value in struct literal: *CachedStorage does not implement Storage (wrong type for method Retrieve)
have Retrieve(uint64, [32]byte) (common.VerifiedBlock, *common.Finalization, error)
want Retrieve(uint64) (common.VerifiedBlock, common.Finalization, error)
why do couple the WAL with the storage anyways?
Yeah, so i was trying to minimize the number of dependencies. I was too frugal to dedicate an entire interface for a single method. I can do it though
There was a problem hiding this comment.
yea i think it makes sense to separate it
There was a problem hiding this comment.
| defer i.lock.Unlock() | ||
|
|
||
| // Stop the non-validator before doing anything else, so that we don't process any more messages while we are changing epochs. | ||
| i.stopNonValidator() |
There was a problem hiding this comment.
i dont get why we need to stop and restart a non-validator for every epoch change. This actually seems like a problem during a non-validators bootstrapping phase.
Non-validators backwards hash-chain validate epochs. This process involves storing requested future sealing blocks in memory and then indexing in order. However, when we index a sealing block, we would restart the non-validator and therefore clear its cached memory of sealing blocks meaning it needs to restart the requesting phase.
The non-validator is epoch agnostic so I think if we transition from non-validator -> non-validator all we need to do is update the communication interface to return the latest validator set.
There was a problem hiding this comment.
even further, if we go from non-validator -> validator, and we are aware of a future epoch we should still be in the non-validating phase?
There was a problem hiding this comment.
Yeah, all very good callouts 👍
I was aware that the current approach isn't ideal but was in a hurry to meet the end of month deadline.
Take a look at the latest code version in the meantime. I will add a test a bit later that will check that we don't needlessly restart non-validators.
There was a problem hiding this comment.
There was a problem hiding this comment.
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
| // GetBlock retrieves the block and finalization at [seq] with the given digest. | ||
| // If the digest is nil, the block with the given sequence number is returned. | ||
| // If [seq] the block cannot be found, returns ErrBlockNotFound. | ||
| GetBlock(seq uint64) (metadata.StateMachineBlock, *common.Finalization, error) |
There was a problem hiding this comment.
yea i think it makes sense to separate it
| } | ||
| } | ||
|
|
||
| func (m *Message) Seq() uint64 { |
There was a problem hiding this comment.
wait why is this added back?
There was a problem hiding this comment.
I rebased and out of habit added it back
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
…y after bootstrapping Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
| ID common.NodeID | ||
| } | ||
|
|
||
| type MessageHandler interface { |
There was a problem hiding this comment.
| close(i.stopCh) | ||
| } | ||
|
|
||
| if i.e != nil { |
There was a problem hiding this comment.
unnecessary since this is checked in stopValidator. Or is this defensive?
There was a problem hiding this comment.
| lastNonSimplexHeight := i.Config.LastNonSimplexInnerBlock.Height() | ||
| genesisValidatorSet := i.Config.PlatformChain.GenesisValidatorSet() | ||
| nodes, epochNum, err := constructEpochAndValidatorSet(lastNonSimplexHeight, genesisValidatorSet, numBlocks, ParsedBlock{StateMachineBlock: lastBlock}, i.Config.Storage) |
There was a problem hiding this comment.
nit: this is duplicated in the Start function as well, might make sense to create another shared helper?
// currentEpochState reads the last persisted block and derives the epoch
// number and validator set the chain is currently in.
func (i *Instance) currentEpochState() (metadata.StateMachineBlock, common.Nodes, uint64, error) {
lastBlock, numBlocks, err := i.lastBlock()
if err != nil {
return metadata.StateMachineBlock{}, nil, 0, err
}
lastNonSimplexHeight := i.Config.LastNonSimplexInnerBlock.Height()
genesisValidatorSet := i.Config.PlatformChain.GenesisValidatorSet()
nodes, epochNum, err := constructEpochAndValidatorSet(lastNonSimplexHeight, genesisValidatorSet, numBlocks, ParsedBlock{StateMachineBlock: lastBlock}, i.Config.Storage)
if err != nil {
return metadata.StateMachineBlock{}, nil, 0, fmt.Errorf("error determining latest epoch and validator set: %w", err)
}
return lastBlock, nodes, epochNum, nil
}There was a problem hiding this comment.
I am not sure it's worth it, because we essentially add a new function that returns a bunch of parameters and then we lose their context/purpose, and we don't actually save a lot in the number of code lines saves as it's only duplicated once.
| return i.startValidatorOrNonValidator(epochChange.validators, epochChange.epochNum) | ||
| } | ||
|
|
||
| func constructEpochAndValidatorSet(lastNonSimplexInnerBlockHeight uint64, genesisValidatorSet metadata.NodeBLSMappings, numBlocks uint64, lastBlock ParsedBlock, storage Storage) (common.Nodes, uint64, error) { |
There was a problem hiding this comment.
since this is an isolated function, should we have claude write some tests that we review for it?
alternatively since this is only used once should we just update the caller? I told claude to do this and it seemed to have made some good cleanups
// currentEpochState reads the last persisted block and derives the epoch
// number and validator set the chain is currently in.
func (i *Instance) currentEpochState() (metadata.StateMachineBlock, common.Nodes, uint64, error) {
lastBlock, numBlocks, err := i.lastBlock()
if err != nil {
return metadata.StateMachineBlock{}, nil, 0, err
}
parsed := ParsedBlock{StateMachineBlock: lastBlock}
sealingInfo := parsed.SealingBlockInfo()
switch {
// If all we have in the ledger is non-Simplex blocks, load the validator set from genesis.
case i.Config.LastNonSimplexInnerBlock.Height()+1 == numBlocks:
genesisValidatorSet := i.Config.PlatformChain.GenesisValidatorSet()
return lastBlock, validatorSetToNodes(genesisValidatorSet), i.Config.LastNonSimplexInnerBlock.Height() + 1, nil
// If the last block persisted is a sealing block, then we are in the next epoch,
// which is identified by the sealing block's sequence and carries its validator set.
case sealingInfo != nil:
return lastBlock, sealingInfo.ValidatorSet, parsed.BlockHeader().Seq, nil
// Else, we have at least one Simplex block in the ledger, and it's not a sealing block.
// Its Epoch field is the sequence of the sealing block that started this epoch.
default:
sealingBlockSeq := parsed.BlockHeader().Epoch
sealingBlock, _, err := i.Config.Storage.GetBlock(sealingBlockSeq)
if err != nil {
return metadata.StateMachineBlock{}, nil, 0, fmt.Errorf("error retrieving sealing block from storage: %w", err)
}
info := (&ParsedBlock{StateMachineBlock: sealingBlock}).SealingBlockInfo()
if info == nil {
return metadata.StateMachineBlock{}, nil, 0, fmt.Errorf("expected sealing block at seq %d, but got a non-sealing block", sealingBlockSeq)
}
return lastBlock, info.ValidatorSet, sealingBlockSeq, nil
}
}
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
Signed-off-by: Yacov Manevich <yacov.manevich@avalabs.org>
|
|
||
| epochAwareStorage := &EpochAwareStorage{ | ||
| epoch: epochNum, | ||
| Storage: i.Config.Storage, |
There was a problem hiding this comment.
is there a reason we are not using the cached storage?
There was a problem hiding this comment.
we only need it for MSM
samliok
left a comment
samliok
left a comment
There was a problem hiding this comment.
i think my previous approval was premature, so i'm just noting some more comments while i'm working on related code.
| cs *CachedStorage | ||
| wal *wal.GarbageCollectedWAL | ||
| msm *metadata.StateMachine | ||
| e *simplex.Epoch | ||
| nv *nonvalidator.NonValidator | ||
| epochOrNV timeAdvancer | ||
| epochChanges chan epochChange | ||
| stopCh chan struct{} |
There was a problem hiding this comment.
can we add some helpful comments to these fields?
| select { | ||
| case now := <-ticker.C: | ||
| i.lock.Lock() | ||
| timeAdvancer := i.epochOrNV |
There was a problem hiding this comment.
can we remove i.epochOrNV? it think it would be fine to just check if i.e != nil { } or i.nonValidator != nil
There was a problem hiding this comment.
we can remove it... sure. Do you want to make a PR and I'll approve?
| return err | ||
| } | ||
|
|
||
| nonValidator, err := nonvalidator.NewNonValidator(config) |
There was a problem hiding this comment.
wait, doesn't this restart the validator? so in the case of non-validator -> non-validator we still run into the problem of resetting the memory & needing to rebroadcast
There was a problem hiding this comment.
No, if we have a nonvalidator-->nonvalidator we do not do an epoch change: https://github.com/ava-labs/Simplex/pull/427/changes/BASE..12fcee7776a03e91664accc93cd440fa7d46b610#diff-f4581317ccb65231765f0fa1aec4aa2bb1c08b09fe9dc1faf095d9039483e4fdR164
There was a problem hiding this comment.
onEpochChange: func(epoch uint64, validators common.Nodes) error {
height := i.Config.PlatformChain.GetCurrentHeight()
vdrs, err := i.Config.PlatformChain.GetValidatorSet(height)
if err != nil {
i.Config.Logger.Error("error getting validator set", zap.Error(err))
return fmt.Errorf("error getting validator set from platform chain: %w", err)
}
comm.SetValidators(validators)
if i.iAmValidator(vdrs.Nodes()) {
i.notifyEpochChange(epoch, validators, nonValidator)
} else {
i.Config.Logger.Debug("I am still a non-validator at the tip of the P-chain, skipping role change",
zap.Uint64("height", height))
}
return nil
},
There was a problem hiding this comment.
i dont think this can work because the tip of the pchain might not correspond to the latest validator set.
I.e. our epoch may be transitioning to a new epoch, which means the latest pchain tip has a different validator set than the validators transitioning the epoch.
There was a problem hiding this comment.
all I'm saying is, if we remain a non-validator we do not do an epoch change.
| // This is the first auxiliary info we're generating for this epoch, | ||
| // so we need to initialize it. | ||
| auxInfo = &AuxiliaryInfo{ | ||
| VersionID: versionID, | ||
| Info: auxInf, | ||
| } |
There was a problem hiding this comment.
What happens if other nodes timed out and we call generate twice? maybe the validator set is small so if 2 nodes time out, we will come back to our node and generate again
| return block.(*ParsedBlock).StateMachineBlock, finalization, nil | ||
| } | ||
|
|
||
| func (cs *CachedStorage) Retrieve(seq uint64, digest common.Digest) (common.VerifiedBlock, *common.Finalization, error) { |
There was a problem hiding this comment.
Retrieve may return a block if its only been verified. Is this a problem in the MSM code on the GetBlock callsites? I initially thought GetBlock would return a notarized or finalized block, in this case it can return neither.
| // Optimistically sign the epoch transition even if we have already did so in a previous round. | ||
| // We'll just deduplicate this approval later on. | ||
|
|
||
| sig, err := sm.createSelfApproval(prevBlockNextPChainReferenceHeight, auxInfoDigest) |
There was a problem hiding this comment.
is it possible for the AuxInfoDigest to change once we are in the collecting approvals phase? i.e we are in the approvals phase if IsSufficient returns true, but can't another block add more aux info and IsSufficient would still be true, however the last digest would change.
| copy(payloadToSignBuff[8:], auxInfoDigest[:]) | ||
|
|
||
| signedMsg := common.SignedMessage{Payload: payloadToSignBuff, Context: signatureContext} | ||
| return asn1.Marshal(signedMsg) |
There was a problem hiding this comment.
do we ever marshal using canato? doesn't seem like it, so why do we include canator data?
| i.Config.Logger.Error("error getting validator set", zap.Error(err)) | ||
| return fmt.Errorf("error getting validator set from platform chain: %w", err) | ||
| } | ||
| comm.SetValidators(validators) |
There was a problem hiding this comment.
i think this is actually a pretty big bug. The non-validator expects comm.Validators to be the most up-to-date validator set, rather than the validator set of the transitioning epoch.
What if we have a completely new set of validators at the tip and set the validators to a disjoint set here? Any replication requests would fail, and we would be stuck.
There was a problem hiding this comment.
Also we can't just use the latest pchain tip, we need to know the pchain height that is referenced by the last epoch...
| return nil | ||
| } | ||
|
|
||
| func (i *Instance) transitionEpochNonValidator(epochChange epochChange) error { |
There was a problem hiding this comment.
i think i mentioned this before, or maybe you did but we need to ensure the epoch we are changing into is the most current. If they are in this epochs validator set, yet are behind we should restart them as a non-validator.
Just noting this so we don't forget
2 participants
This commit contains an implementation of an orchestration layer that orchestrates
epoch transition for validators and non-validators as they move through epochs.
The orchestration layer introduces an Instance that drives the epoch lifecycle,
switching between validator consensus and non-validator block tracking as the
node's role changes across epochs.
Key pieces:
non-validator, and handles the handoff at epoch boundaries.
while an epoch change is in progress, preventing side effects mid-transition.
new epoch is detected.