Implement approval store

[yacovm]

Also self-sign during aggregation

• Introduces ApprovalStore (msm/approvals.go) — an in-memory store of validator approvals for epoch transitions, keyed by (NodeID, PChainHeight). It verifies BLS signatures on
  ingest, deduplicates by timestamp (newer wins, older is dropped), and prunes per-node entries to at most len(validators) by evicting the oldest timestamp.
• Adds Timestamp to ValidatorSetApproval (msm/encoding.go) so the store can order/evict approvals deterministically.
• computeNewApprovals (msm/msm.go) now optimistically self-signs the next epoch's P-chain reference height each round and appends its own ValidatorSetApproval to the peer set since the store deduplicates it later.

[samliok]

hmm is it not a problem that we verify the signature is valid over just the pChainHeight here, but in the MSM code we verify it with the context + asn1 encoding? shouldn't there be one canonical way of verifying/signing that is shared for both of them?

[yacovm]

Yeah, because when I implemented this, I still haven't done that domain separation in the MSM.

Will fix 👍

[yacovm]

6718a90#diff-5e383ab850834861309c7e77b4123dbe73b6b4d3e2b96f96832869f5847b48fdR117-R125

[samliok]

is it still not a problem though that we don't check the approval signature against the timestamp? we also don't pass in who sent the approval so someone could send an approval with a timestamp = MaxUint64 and say it was from another node. This would remove all other approvals from that node in maybePruneOldApprovals.

[yacovm]

I think you have answered your own question.

This only works if the dissemination of the approvals is point to point, over authenticated channels, and not via gossip.

[yacovm]

Also recall that:

type NextEpochApprovals struct {
	NodeIDs   []byte `canoto:"bytes,1"`
	Signature []byte `canoto:"bytes,2"`

	canotoData canotoData_NextEpochApprovals
}

This doesn't include timestamps, and we naturally cannot expect all signatures to match a single timestamp.

The timestamp is just there to give us liveness when a node crashes and restarts or picks a different P-chain height to sign over. We need some way of disposing of "old" in-memory values.

We could have the timestamp be even computed locally inside of HandleApproval and not sent over the wire at all.

[samliok]

We could have the timestamp be even computed locally inside of HandleApproval and not sent over the wire at all

might be a nice change, why send it if we don't need to.

[yacovm]

da16638#diff-5e383ab850834861309c7e77b4123dbe73b6b4d3e2b96f96832869f5847b48fdR17-R19

[samliok]

should we just do something like instead? i think the for loop is redundant(see comment)

func (as *ApprovalStore) maybePruneOldApprovals(approval *ValidatorSetApproval) {
	approvals := as.approvalsByNodes[approval.NodeID]

	// We add at most one approval per call, so the node can exceed the cap by
	// at most one entry. When it does, evict the one with the oldest timestamp.
	if len(approvals) <= len(as.validators) {
		return
	}

	var oldest *ValidatorSetApproval
	for _, a := range approvals {
		if oldest == nil || a.Timestamp < oldest.Timestamp {
			oldest = a
		}
	}

	as.logger.Debug("Deleting old approval from node",
		zap.String("nodeID", fmt.Sprintf("%x", oldest.NodeID)),
		zap.Uint64("oldestApprovalPChainHeight", oldest.PChainHeight),
		zap.Uint64("oldestApprovalTimestamp", oldest.Timestamp))
	delete(approvals, oldest.PChainHeight)
	as.storedCount--
}

[yacovm]

yeah we could do that, that was probably too much of defensive programming

[yacovm]

da16638#diff-5e383ab850834861309c7e77b4123dbe73b6b4d3e2b96f96832869f5847b48fdR104-R121