Ensure Non-Validators and Validators Bootstrap Before Indexing and Verifying Blocks

[samliok]

Addresses #530 but for non-validator and validators.

Nodes will first need to complete bootstrapping before being able to index/verify blocks.

[yacovm]

Looks like this test demonstrates we have a liveness problem in case we miss the first broadcast we send via broadcastLatestEpoch:

// TestNonValidatorBootstrapsWhenStartupBroadcastIsLost asserts a non-validator still bootstraps when
// nobody receives the replication request it broadcasts on start. The non-validator joins an empty
// network, so that broadcast reaches no one, and the validator only comes online afterwards. Until it
// bootstraps, the non-validator drops every block and finalization the validator sends it, so the only
// way for it to recover is to ask again.
func TestNonValidatorBootstrapsWhenStartupBroadcastIsLost(t *testing.T) {
	validator := newNodeMapping(1)
	pChain := newTestPChain([]metadata.NodeBLSMapping{validator})
	network := newNetwork(t, pChain)

	// The non-validator starts alone, so the replication request it broadcasts on start is lost.
	nonValidator := newNodeMapping(2)
	node := network.addNode(nonValidator.NodeID[:])
	isValidator, bootstrapped := node.role()
	require.False(t, isValidator)
	require.False(t, bootstrapped)

	// The validator then comes online and commits the first block on its own,
	// broadcasting its finalization to the non-validator, which drops it.
	network.addNode(validator.NodeID[:]).sync()

	// The validator is there to answer, so the non-validator must eventually ask again and bootstrap.
	require.Eventually(t, func() bool {
		_, bootstrapped := node.role()
		return bootstrapped
	}, 20*time.Second, 100*time.Millisecond, "the non-validator never bootstrapped after its startup broadcast was lost")

	node.sync()
}

[yacovm]

In the issue #530 I wrote:

What we should do instead is when we bootstrap the node, we replicate only the sealing blocks backwards, until we reach the latest committed sealing block, and then we keep the chain of sealing blocks and use them and never replicate them by verifying quorum certificates.

The below test demonstrates that that's not what we do:

// TestNonValidatorRejectsSealingBlockOffTheHashChain asserts a bootstrapping non-validator only accepts
// sealing blocks that lie on the backward hash chain from the sealing block it bootstrapped on, however
// well they are signed. See https://github.com/ava-labs/Simplex/issues/530.
//
// The node has committed epoch 1's defining block. The current validator set tells it the latest sealing
// block is at seq 3, whose PrevSealingBlockHash names an honest sealing block at seq 2. A peer then serves
// a forged sealing block at seq 2 that carries a valid quorum certificate of epoch 1's validators but is
// not the block seq 3 points to. Epoch 1's keys may have leaked long ago, so the forged block must be
// rejected. Today the node validates it by its quorum certificate alone and indexes it.
func TestNonValidatorRejectsSealingBlockOffTheHashChain(t *testing.T) {
	validator := newNodeMapping(1)
	validators := metadata.NodeBLSMappings{validator}
	signers := []common.NodeID{validator.NodeID[:]}
	sigAggregator := &testutil.TestSignatureAggregator{N: len(validators)}

	pChain := newTestPChain(validators)
	storage, epochBlock := newChainStorage(t, validators)

	// A node outside the validator set whose ledger ends at epoch 1's defining block.
	nonValidator := newNodeMapping(2)
	node := newNetwork(t, pChain).addNodeWithConfig(nonValidator.NodeID[:], nodeConfig{storage: storage})
	isValidator, bootstrapped := node.role()
	require.False(t, isValidator)
	require.False(t, bootstrapped)

	// sealingBlock builds a sealing block at seq in the given epoch, keeping the same validator set.
	sealingBlock := func(seq, epoch uint64, prev, prevSealingBlockHash [32]byte, payload string) *ParsedBlock {
		timestamp := epochBlockTime.Add(time.Duration(seq) * time.Millisecond)
		return &ParsedBlock{StateMachineBlock: metadata.StateMachineBlock{
			InnerBlock: &testInnerBlock{Height_: seq, TS: timestamp, Payload: []byte(payload)},
			Metadata: metadata.StateMachineMetadata{
				Timestamp:               uint64(timestamp.UnixMilli()),
				SimplexProtocolMetadata: common.ProtocolMetadata{Epoch: epoch, Round: seq, Seq: seq, Prev: common.Digest(prev)},
				SimplexEpochInfo: metadata.SimplexEpochInfo{
					EpochNumber:          epoch,
					PrevSealingBlockHash: prevSealingBlockHash,
					BlockValidationDescriptor: &metadata.BlockValidationDescriptor{
						AggregatedMembership: metadata.AggregatedMembership{Members: validators},
					},
				},
			},
		}}
	}

	// The honest chain: seq 1 (in storage) <- honest seq 2 <- seq 3. The forged seq 2 is signed by the
	// same epoch 1 validators but is not what seq 3 points to.
	honestSealing2 := sealingBlock(2, 1, epochBlock.Digest(), epochBlock.Digest(), "honest sealing block 2")
	forgedSealing2 := sealingBlock(2, 1, epochBlock.Digest(), epochBlock.Digest(), "forged sealing block 2")
	require.NotEqual(t, honestSealing2.Digest(), forgedSealing2.Digest())
	sealing3 := sealingBlock(3, 2, honestSealing2.Digest(), honestSealing2.Digest(), "sealing block 3")

	honestFinalization2, _ := testutil.NewFinalizationRecord(t, sigAggregator, honestSealing2, signers)
	forgedFinalization2, _ := testutil.NewFinalizationRecord(t, sigAggregator, forgedSealing2, signers)
	finalization3, _ := testutil.NewFinalizationRecord(t, sigAggregator, sealing3, signers)

	// The current validator set reports seq 3 as the latest sealing block, which bootstraps the node.
	require.NoError(t, node.inst.HandleMessage(&common.Message{
		ReplicationResponse: &common.ReplicationResponse{
			LatestSeq: &common.QuorumRound{Block: sealing3, Finalization: &finalization3},
		},
	}, validator.NodeID[:]))
	_, bootstrapped = node.role()
	require.True(t, bootstrapped)

	// A peer serves the forged seq 2. Its quorum certificate checks out against epoch 1, but its digest
	// is not the PrevSealingBlockHash of seq 3, so it is off the hash chain and must not be indexed.
	require.NoError(t, node.inst.HandleMessage(&common.Message{
		ReplicationResponse: &common.ReplicationResponse{
			Data: []common.QuorumRound{{Block: forgedSealing2, Finalization: &forgedFinalization2}},
		},
	}, validator.NodeID[:]))
	require.Never(t, func() bool {
		_, _, err := node.storage.Retrieve(2)
		return err == nil
	}, 2*time.Second, 50*time.Millisecond, "indexed a sealing block that is not on the hash chain from the sealing block we bootstrapped on")

	// The honest seq 2 is what seq 3 points to, so it is accepted.
	require.NoError(t, node.inst.HandleMessage(&common.Message{
		ReplicationResponse: &common.ReplicationResponse{
			Data: []common.QuorumRound{{Block: honestSealing2, Finalization: &honestFinalization2}},
		},
	}, validator.NodeID[:]))
	committed := node.storage.WaitForBlockCommit(2)
	require.Equal(t, honestSealing2.Bytes(), committed.Bytes())
}

[yacovm]

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.

[samliok]

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

[samliok]

as to your test, i made a few non-validator tests to ensure we do the backwards hash validation first

TestNonValidator_BootstrapIgnoresSealingBlockOffChain && TestNonValidator_BootstrapWalksHashChain

[yacovm]

These are the comments I have so far, I'm not nearly done with the review.

[yacovm]

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.

[samliok]

updated from bootstrapped terminology to EpochsReplicated 57c282e

[yacovm]

but we should still replicate all blocks in the last epoch that we know about before we declare that we have finished bootstrapping.

[yacovm]

I think this comment is still relevant

[samliok]

updated from bootstrapped terminology to EpochsReplicated 57c282e

[yacovm]

unrelated to this PR, but... what updates the comm's validator set once we move through epochs after we bootstrap?

[samliok]

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

[yacovm]

Made another pass

[yacovm]

nv.sealingBlockTimeouts.AddTask(0) - I don't understand what we're trying to do here.

[samliok]

i updated it to

// initialBootstrapTask is the sealingBlockTimeouts task that continuously asks for sealing blocks until a
// threshold of responses validates an epoch. We use Seq 1, since requests with Seq 0 are dropped.
const initialBootstrapTask uint64 = 1

This timeout task only gets removed when we validate an epoch, otherwise it will keep sending replication requests. This avoids the problem where we send a request, but that request gets dropped and never delivered.

I also moved adding the task to Start.

[yacovm]

It looks like we maybe validate and then anyway store the QR?

but maybeValidateNextEpoch can return early in many cases. Is that intentional?

I guess the purpose of validateSealingBlock is to kickstart validation of the previous sealing block? If so, is this the right name for the method?

[samliok]

yea we only call validateSealingBlock when the sealing block can be validated. so we should store the quorum round.

also maybeValidateNextEpoch is called in case the block is sealing block, not necessarily from replication.

What would u suggest the name be?

[yacovm]

Not really relevant to this PR, but - if validators changes between two invocations of this function, then we have a problem.

Consider two invocations collectedSealingBlockInfo() in t1 and collectedSealingBlockInfo() in t2, and the first one sampled validators = [v1, v2, v3, v4] and the second once sampled v2, v3, v4, v5]'.

The first one got a vote from v1 and the second one got a vote from v2 but v1 is not in the second.

We reached the threshold f+1 but with an illegal count.

[samliok]

#607

[yacovm]

I think this comment is still relevant