Conversation
6be0d9c to
23d9c67
Compare
9c76fa7 to
9501164
Compare
9501164 to
a1bb893
Compare
9d1886a to
b4bfaba
Compare
b4bfaba to
4b6b2f5
Compare
a1a3fb9 to
b507b64
Compare
|
Looks like this test demonstrates we have a liveness problem in case we miss the first broadcast we send via |
|
In the issue #530 I wrote:
The below test demonstrates that that's not what we do: |
| return nil | ||
| } | ||
|
|
||
| n.Logger.Info("Bootstrapped, received a threshold of sealing block info for an epoch", zap.Stringer("Info", qr.Block.SealingBlockInfo())) |
There was a problem hiding this comment.
We should only consider ourselves as bootstrapped when we have replicated and committed all blocks from the last block in the ledger to the last known tip.
There was a problem hiding this comment.
i think we can consider ourselves bootstrapped once we have validated the hash chain of sealing blocks, then we can start as normal syncing all the blocks in between
There was a problem hiding this comment.
as to your test, i made a few non-validator tests to ensure we do the backwards hash validation first
TestNonValidator_BootstrapIgnoresSealingBlockOffChain && TestNonValidator_BootstrapWalksHashChain
b04bddb to
c2d3997
Compare
yacovm
left a comment
There was a problem hiding this comment.
These are the comments I have so far, I'm not nearly done with the review.
| // and it is in the validator set | ||
| TransitionToValidator func(epoch uint64, validators common.Nodes) | ||
|
|
||
| // Bootstrapped is set once every epoch from our tip up to the one a threshold of the latest |
There was a problem hiding this comment.
Why would we call that bootstrapped? Bootstrapped should just mean that we finished bootstrapping, exactly like we do in snowman. Which is that we have replicated all blocks we know are missing from the latest discovered tip down to the tip before bootstrapping.
There was a problem hiding this comment.
updated from bootstrapped terminology to EpochsReplicated 57c282e
| return nil | ||
| } | ||
|
|
||
| // No sealing block is missing, so every epoch from our tip to the highest is validated. |
There was a problem hiding this comment.
but we should still replicate all blocks in the last epoch that we know about before we declare that we have finished bootstrapping.
There was a problem hiding this comment.
I think this comment is still relevant
There was a problem hiding this comment.
updated from bootstrapped terminology to EpochsReplicated 57c282e
| } | ||
|
|
||
| comm := newCommunication(i.Config.Sender, i.Config.Broadcaster, mappings.Nodes()) | ||
| comm := newCommunication(i.Config.Sender, i.Config.Broadcaster, latestValidatorSet.Nodes()) |
There was a problem hiding this comment.
unrelated to this PR, but... what updates the comm's validator set once we move through epochs after we bootstrap?
There was a problem hiding this comment.
yea we need to change the non-validator comm to not hardcode its Validators() method. everytime it calls Validators the pchain should be queried or something
1751db3 to
3e51cb0
Compare
| for _, seq := range seqs { | ||
| n.Logger.Debug("Re-requesting a sealing block", zap.Uint64("Seq", seq)) | ||
| n.Comm.Broadcast(&common.Message{ | ||
| ReplicationRequest: &common.ReplicationRequest{Seqs: []uint64{seq}}, |
There was a problem hiding this comment.
shouldn't we add here the LatestFinalizedSeq or call broadcastLatestEpoch?
Seems like the initial broadcastLatestEpoch we do in Start() only works once now, no?
There was a problem hiding this comment.
ah yea good point
|
|
||
| n.sealingBlockTimeouts.RemoveTask(nextEpoch) | ||
|
|
||
| // The first simplex block opens its own epoch, so there is no earlier sealing block. |
There was a problem hiding this comment.
how is this comment relevant to the code below it?
There was a problem hiding this comment.
Is it about prevSealingSeq == nextEpoch ?
| } | ||
| } | ||
|
|
||
| // validateSealingBlock validates the epoch a sealing block opens and stores its quorum round. |
There was a problem hiding this comment.
I am not sure validateX is the right way to call this method.
Judging by the name I would expect it should check the sealing block and return if it's valid or not.
In practice the method stores it or even sends request to fetch the previous one.
Can we give it a more descriptive name?
There was a problem hiding this comment.
Perhaps we can call this processSealingBlock and call maybeValidateNextEpoch - maybeReplicateNextEpoch ?
And can we add a comment saying that we expect the given block to have already been authenticated here and in maybeValidateNextEpoch?
|
|
||
| func (n *NonValidator) maybeValidateNextEpoch(block common.Block) { | ||
| nextEpoch := block.BlockHeader().Seq | ||
| // maybeValidateNextEpoch validates the epoch block opens when block is a sealing block. While |
There was a problem hiding this comment.
maybeValidateNextEpoch validates the epoch block opens when block is a sealing block.
I can't grok this sentence. The epoch block opens what?
| if block.SealingBlockInfo() != nil { | ||
| highestEpoch, highestValidatorSet := n.epochs.highestEpoch() | ||
|
|
||
| // We should only transition to become a validator, if the sealing block is creating the highest |
There was a problem hiding this comment.
did you fold the below lines into maybeTransitionToValidator ?
| if !n.isIndexed(bh.Seq) { | ||
| n.validateSealingBlock(qr, from) | ||
| } | ||
| case n.highestEpochCollector.collectedSealingBlockInfo(sealingInfo, bh, from): |
There was a problem hiding this comment.
maybe we can rename collectedSealingBlockInfo to something that reflects better its implementation or what it actually stands for?
Perhaps maybeObserveThresholdResponses() ?
| } | ||
|
|
||
| func (i *Instance) maybeReplicateEpochs() error { | ||
| i.Config.Logger.Debug("Checking if epoch replication is required") |
There was a problem hiding this comment.
Shouldn't it be something like: "Checking if latest persisted validator set is up to date" ?
| // We have indexed the latest validator set, therefore we can skip epoch replication and start as a validator. | ||
| // Note: this may not be the latest epoch, but a future PR will eventually notice we are behind and transition properly. | ||
| if latestIndexedEpochValidators.Equal(latestValidatorSet.Nodes()) && latestValidatorSet.Nodes().Contains(i.Config.ID) { | ||
| i.Config.Logger.Debug("Node skipping epoch replication because its latest epoch is up to date with the Platform Chain") |
There was a problem hiding this comment.
We should prefer writing log messages that are understandable to node operators and not to us.
So instead of "epoch replication" let's say "validator set replication" here and below.
I think it conveys more what we're trying to accomplish.
| network.setOnline(v3.NodeID[:]) | ||
| node.sync() | ||
|
|
||
| // The only way for the epoch transition to finish is if ourNode becomes a validator |
There was a problem hiding this comment.
shouldn't we check the node is also a validator now?
| } | ||
|
|
||
| // TestNonValidator_EpochReplicationWalksHashChain asserts a non-validator several epochs behind requests | ||
| // sealing blocks one hop back at a time once a threshold reports the highest one, and finishes |
There was a problem hiding this comment.
why "once" and not "starting from" ?
| } | ||
|
|
||
| // TestNonValidator_EpochReplicationIgnoresSealingBlockOffChain asserts that while following the hash chain | ||
| // a sealing block further down it is dropped until the epoch pointing back to it has been validated. |
There was a problem hiding this comment.
a sealing block further down it is dropped until the epoch pointing back to it has been validated
I am not sure I understand what this means. Why is a sealing block dropped? How can something be dropped until something?
| require.NoError(t, err) | ||
| defer nv.Stop() | ||
|
|
||
| inOurEpoch := tc.appendBlock() |
There was a problem hiding this comment.
The code below is confusing regarding which epoch corresponds to which block.
Can we add some kind of require.Equal(expectedEpoch, block.Epoch) after to make this more clear?
| @@ -1047,6 +1058,291 @@ func TestNonValidatorRejectsQuorumRoundFromNonValidator(t *testing.T) { | |||
| } | |||
There was a problem hiding this comment.
Aren't we missing a non_validator_test test case that shows that when a new non-validator is added, it replicates the sealing blocks first and afterwards the blocks between them?
Addresses #530 but for non-validator and validators.
Nodes will first need to complete bootstrapping before being able to index/verify blocks.