validator: close the node axis, make the canonical order build-invariant

Red review of the oracle found the bytes unbreakable and the set-plane rule around them incomplete. This closes it. HIGH — one node, one key. Register enforced one-key-one-node but not the converse: a slice could name one node twice under two keys, each with a genuine proof, so possession did not catch it and one operator held several signer indices and several shares of the weight under one identity. FlattenValidatorSet's map input made that unrepresentable; a slice does not. A byNode set beside byKey refuses it (ErrDuplicateNode), in the same pass, so the set is a set on both axes — the rule weighted_merkle already states and registration.go had half of. HIGH — one canonical order, on every build. PublicKeyBytes, the sort key, was the UNCOMPRESSED encoding, which the crypto library returns as 96 bytes under blst and 48 under CIRCL — so the same set sorted two ways, and two nodes on different builds indexed the signer bits differently. It is the COMPRESSED encoding now, in both Register and the local FlattenValidatorSet, so the order is the same on every build. Under the canonical (purego) image this still matches upstream; under cgo it is now deterministic where upstream is not. The crypto root cause is upstream and live on the warp path — escalated separately, not fixed here. Also: key uniqueness now keys on the compressed bytes decoded from the point, not the caller's input buffer read three times (a caller could mutate it between the verify and the map insert); a zero-weight validator is refused (a signer that raises the count but not the weight, the phantom BuildWeightedValidatorSet already refuses); cert-store eviction drops the file the evicted slot names, not whatever the map points at, and unmaps a decision only if the map still names the removed file; and a canonicalCommitter test block exercises the cert-store re-key in the regime where the inner decision differs from the outer envelope — the case every prior test degraded past. Node width is 20 bytes, frozen in vectors/pop.json. Nothing here runs on mainnet: node pins a consensus version without these files, and Register has no non-test callers. A validator set that shares a key across node ids is now refused — the intended behaviour, and the seam with the P-chain step still to come. Co-authored-by: Hanzo Dev <[email protected]>
2026-09-24 00:26:58 +00:00 · 2026-09-01 08:10:35 -07:00
parent af808b25ad
commit c9b9b94e99
4 changed files with 165 additions and 19 deletions
@@ -192,11 +192,16 @@ func (c *fileCerts) evictLocked() {
 	for len(c.order) > maxServedCerts {
 		ev := c.order[0]
 		c.order = c.order[1:]
-		name, ok := c.byDecision[ev.id]
-		if !ok {
-			continue
-		}
-		delete(c.byDecision, ev.id)
+		// Drop the file THIS slot names, not whatever the map points at. If a
+		// decision ever has two names on disk — a failed remove, an operator
+		// copy — the map names one and the evicted slot the other, and the slot's
+		// is the stale one to drop; deleting the map's would take the live cert.
+		name := certName(ev.height, ev.id)
 		_ = os.Remove(filepath.Join(c.dir, name))
+		// Unmap the decision only if the map still points at the file just
+		// removed, so a decision that survives under a different name stays served.
+		if mapped, ok := c.byDecision[ev.id]; ok && mapped == name {
+			delete(c.byDecision, ev.id)
+		}
 	}
 }
@@ -188,3 +188,55 @@ func TestCertStoreNameRoundTrips(t *testing.T) {
 		}
 	}
 }
+
+// canonicalTestBlock is a block whose INNER execution commitment differs from its
+// OUTER envelope id — a proposervm-wrapped block, the regime the cert-store re-key
+// exists for. Without one, canonicalIDOf degrades to the outer id in every test
+// and the re-key is never exercised where it differs.
+type canonicalTestBlock struct {
+	*testBlock
+	canonical ids.ID
+}
+
+func (b *canonicalTestBlock) CanonicalID() ids.ID        { return b.canonical }
+func (b *canonicalTestBlock) ParentCanonicalID() ids.ID  { return ids.Empty }
+func (b *canonicalTestBlock) ExecutionStateRoot() ids.ID { return ids.Empty }
+func (b *canonicalTestBlock) PayloadRoot() ids.ID        { return ids.Empty }
+
+// TestCertStoreKeysOnTheDecisionNotTheEnvelope: when a block exposes a distinct
+// inner canonical id, the cert is filed and found under THAT id, and the outer
+// envelope id finds nothing. This is the mainnet-644 class the re-key closes; a
+// store keyed on the envelope would answer the wrong question here.
+func TestCertStoreKeysOnTheDecisionNotTheEnvelope(t *testing.T) {
+	outer := blockAt(7)
+	inner := blockAt(200)
+	blk := &canonicalTestBlock{
+		testBlock: &testBlock{id: outer, height: 42},
+		canonical: inner,
+	}
+
+	// The engine keys on this, not on blk.ID().
+	decision := canonicalIDOf(blk)
+	if decision != inner {
+		t.Fatalf("canonicalIDOf took the envelope, not the inner commitment: %s", decision)
+	}
+	// And a bare block with no inner commitment degrades to its own id, exactly.
+	if got := canonicalIDOf(blk.testBlock); got != outer {
+		t.Fatalf("a bare block should key on its outer id, got %s", got)
+	}
+
+	dir := t.TempDir()
+	store, err := OpenCerts(dir)
+	if err != nil {
+		t.Fatalf("OpenCerts: %v", err)
+	}
+	if err := store.Put(decision, blk.Height(), []byte("cert-for-inner")); err != nil {
+		t.Fatalf("Put: %v", err)
+	}
+	if _, ok := store.Get(decision); !ok {
+		t.Fatalf("a cert filed under the decision is not found under the decision")
+	}
+	if _, ok := store.Get(outer); ok {
+		t.Fatalf("a cert filed under the decision must not be found under the envelope")
+	}
+}
@@ -43,6 +43,15 @@ var (
 	// One key, one node: counting distinct voters has to count distinct signers.
 	ErrDuplicateKey = errors.New("validators: public key is registered to more than one node")

+	// ErrDuplicateNode is returned when one node id is registered twice. One
+	// node, one key: a slice can name a node twice where a map could not, and
+	// each entry is a separate signer index and a separate share of the weight.
+	ErrDuplicateNode = errors.New("validators: node is registered more than once")
+
+	// ErrZeroWeight is returned when a validator is admitted with no stake — a
+	// signer that counts toward the number of signers but not toward the weight.
+	ErrZeroWeight = errors.New("validators: validator has zero weight")
+
 	// ErrNoKey is returned when a registration carries no public key. A validator
 	// with no key cannot sign, so it cannot be admitted through the proof path.
 	ErrNoKey = errors.New("validators: registration carries no public key")
@@ -86,12 +95,20 @@ func Register(rs []Registration) (CanonicalValidatorSet, error) {
 	var (
 		out         = make([]*CanonicalValidator, 0, len(sorted))
 		byKey       = make(map[string]ids.NodeID, len(sorted))
+		byNode      = make(map[ids.NodeID]struct{}, len(sorted))
 		totalWeight uint64
 	)
 	for _, r := range sorted {
 		if len(r.Key) == 0 {
 			return CanonicalValidatorSet{}, fmt.Errorf("%w: %s", ErrNoKey, r.NodeID)
 		}
+		// A keyed validator with no stake is a phantom signer: it raises the
+		// signer count without raising the weight, which is the same disagreement
+		// between "how many signed" and "how much signed" that BuildWeightedValidatorSet
+		// refuses for the same reason.
+		if r.Weight == 0 {
+			return CanonicalValidatorSet{}, fmt.Errorf("%w: %s", ErrZeroWeight, r.NodeID)
+		}
 		// ENCODING, then POSSESSION — both inside pop.Verify, in that order.
 		if err := pop.Verify(r.NodeID, r.Key, r.Proof); err != nil {
 			return CanonicalValidatorSet{}, fmt.Errorf("%w: %s: %w", ErrPossession, r.NodeID, err)
@@ -100,12 +117,24 @@ func Register(rs []Registration) (CanonicalValidatorSet, error) {
 		if err != nil {
 			return CanonicalValidatorSet{}, fmt.Errorf("%s: %w", r.NodeID, err)
 		}
-		// UNIQUENESS. Keyed on the canonical compressed encoding, which pop.Verify
-		// has already established is the only spelling of this point.
-		if first, dup := byKey[string(r.Key)]; dup {
+		// The set has to be a set on BOTH axes, and neither implies the other.
+		// UNIQUENESS OF KEY, keyed on the canonical compressed encoding decoded
+		// from the point — never the caller's input bytes, which pop.Verify read
+		// but which a caller could mutate between reads.
+		canonical := string(bls.PublicKeyToCompressedBytes(pk))
+		if first, dup := byKey[canonical]; dup {
 			return CanonicalValidatorSet{}, fmt.Errorf("%w: %s and %s", ErrDuplicateKey, first, r.NodeID)
 		}
-		byKey[string(r.Key)] = r.NodeID
+		byKey[canonical] = r.NodeID
+		// UNIQUENESS OF NODE. A slice can name one node twice under two keys,
+		// each with a genuine proof; possession does not catch it, and one
+		// operator in N canonical slots is N signer indices and N times its
+		// weight. `FlattenValidatorSet`'s map input made this unrepresentable;
+		// a slice does not, so it is refused here.
+		if _, dup := byNode[r.NodeID]; dup {
+			return CanonicalValidatorSet{}, fmt.Errorf("%w: %s", ErrDuplicateNode, r.NodeID)
+		}
+		byNode[r.NodeID] = struct{}{}

 		// WEIGHT, last.
 		sum, err := math.Add64(totalWeight, r.Weight)
@@ -114,8 +143,12 @@ func Register(rs []Registration) (CanonicalValidatorSet, error) {
 		}
 		totalWeight = sum
 		out = append(out, &CanonicalValidator{
-			PublicKey:      pk,
-			PublicKeyBytes: bls.PublicKeyToUncompressedBytes(pk),
+			PublicKey: pk,
+			// Compressed, always 48 bytes — the sort key `Compare` reads, so the
+			// canonical ORDER does not depend on which crypto build produced it.
+			// Upstream's uncompressed bytes are 96 under cgo and 48 under purego,
+			// which orders the same set two ways; the oracle must have one order.
+			PublicKeyBytes: []byte(canonical),
 			Weight:         r.Weight,
 			NodeIDs:        []ids.NodeID{r.NodeID},
 		})
@@ -174,7 +207,9 @@ func FlattenValidatorSet(vdrSet map[ids.NodeID]*GetValidatorOutput) (CanonicalVa
 		if err != nil {
 			continue
 		}
-		pkBytes := bls.PublicKeyToUncompressedBytes(pk)
+		// Compressed, so uniqueness and the canonical order are the same on every
+		// crypto build — see Register.
+		pkBytes := bls.PublicKeyToCompressedBytes(pk)
 		if first, dup := byKey[string(pkBytes)]; dup {
 			return CanonicalValidatorSet{}, fmt.Errorf("%w: %s and %s", ErrDuplicateKey, first, nodeID)
 		}
@@ -114,6 +114,40 @@ func TestOneKeyOneNode(t *testing.T) {
 	}
 }

+// TestOneNodeOneKey is the other axis: one node id, two keys, each with a genuine
+// proof. Possession does not catch it — both proofs are sound — so only a node
+// uniqueness rule refuses it, and it must, or one operator holds several signer
+// indices and several shares of the weight under one identity.
+func TestOneNodeOneKey(t *testing.T) {
+	node := nodeID(0x40)
+	skA, skB := secret(t, 40), secret(t, 41)
+	keyA := bls.PublicKeyToCompressedBytes(skA.PublicKey())
+	keyB := bls.PublicKeyToCompressedBytes(skB.PublicKey())
+	proofA, err := pop.Sign(skA, node)
+	if err != nil {
+		t.Fatalf("pop.Sign A: %v", err)
+	}
+	proofB, err := pop.Sign(skB, node)
+	if err != nil {
+		t.Fatalf("pop.Sign B: %v", err)
+	}
+	// Both proofs are real: possession holds for each pair on its own.
+	if err := pop.Verify(node, keyA, proofA); err != nil {
+		t.Fatalf("first proof is not sound: %v", err)
+	}
+	if err := pop.Verify(node, keyB, proofB); err != nil {
+		t.Fatalf("second proof is not sound: %v", err)
+	}
+
+	set, err := Register([]Registration{
+		{NodeID: node, Key: keyA, Proof: proofA, Weight: 100},
+		{NodeID: node, Key: keyB, Proof: proofB, Weight: 100},
+	})
+	if !errors.Is(err, ErrDuplicateNode) {
+		t.Fatalf("one node admitted under two keys: err=%v set=%d validators", err, len(set.Validators))
+	}
+}
+
 // TestRegisterRefusesTheUnproven walks the possession gate.
 func TestRegisterRefusesTheUnproven(t *testing.T) {
 	good := registration(t, 6, 0x30, 1)
@@ -261,16 +295,36 @@ func TestFlattenMatchesUpstreamWhenKeysAreDistinct(t *testing.T) {
 	if len(mine.Validators) != len(theirs.Validators) {
 		t.Fatalf("validator count diverged: %d vs %d", len(mine.Validators), len(theirs.Validators))
 	}
-	for i := range mine.Validators {
-		a, b := mine.Validators[i], theirs.Validators[i]
-		if !bytes.Equal(a.PublicKeyBytes, b.PublicKeyBytes) {
-			t.Fatalf("canonical order diverged at %d", i)
+
+	// Same CONTENT, compared as a set keyed by node id, not index by index:
+	// upstream sorts by uncompressed bytes, which are 96 under cgo and 48 under
+	// purego, so its ORDER is build-dependent. Ours sorts by the compressed bytes
+	// and is the same on every build; that the two orders can differ under cgo is
+	// the divergence this branch removes, not a parity failure.
+	theirByNode := map[ids.NodeID]*CanonicalValidator{}
+	for _, v := range theirs.Validators {
+		theirByNode[v.NodeIDs[0]] = v
+	}
+	for _, a := range mine.Validators {
+		if len(a.NodeIDs) != 1 {
+			t.Fatalf("a validator carries %d node ids, want 1", len(a.NodeIDs))
+		}
+		b, ok := theirByNode[a.NodeIDs[0]]
+		if !ok {
+			t.Fatalf("node %s is in mine but not upstream", a.NodeIDs[0])
 		}
 		if a.Weight != b.Weight {
-			t.Fatalf("weight diverged at %d: %d vs %d", i, a.Weight, b.Weight)
+			t.Fatalf("weight diverged for %s: %d vs %d", a.NodeIDs[0], a.Weight, b.Weight)
 		}
-		if len(a.NodeIDs) != 1 || a.NodeIDs[0] != b.NodeIDs[0] {
-			t.Fatalf("node ids diverged at %d: %v vs %v", i, a.NodeIDs, b.NodeIDs)
+		if !bytes.Equal(a.PublicKeyBytes, bls.PublicKeyToCompressedBytes(b.PublicKey)) {
+			t.Fatalf("key diverged for %s", a.NodeIDs[0])
+		}
+	}
+
+	// And ours is deterministically ordered by the compressed key, on any build.
+	for i := 1; i < len(mine.Validators); i++ {
+		if bytes.Compare(mine.Validators[i-1].PublicKeyBytes, mine.Validators[i].PublicKeyBytes) >= 0 {
+			t.Fatalf("mine is not sorted ascending by compressed key at %d", i)
 		}
 	}
 }