Simplex orchestration layer preliminary implementation#427
Conversation
yacovm
force-pushed
the
orchestration
branch
9 times, most recently
from
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
yacovm
force-pushed
the
orchestration
branch
2 times, most recently
from
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
},
| // 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
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.