diff --git a/msm/approvals.go b/msm/approvals.go index d2a53674..de0349dd 100644 --- a/msm/approvals.go +++ b/msm/approvals.go @@ -4,6 +4,7 @@ package metadata import ( + "bytes" "fmt" "sync" @@ -12,6 +13,8 @@ import ( "go.uber.org/zap" ) +type approvalCheck func(approval *common.ValidatorSetApproval, pk []byte) error + type approvalKey struct { pChainHeight uint64 auxInfoDigest [32]byte @@ -71,6 +74,10 @@ func (as *ApprovalStore) Approvals() ValidatorSetApprovals { } func (as *ApprovalStore) HandleApproval(approval *common.ValidatorSetApproval, timestamp uint64) error { + return as.handleApproval(approval, timestamp, as.checkApprovalSignature) +} + +func (as *ApprovalStore) handleApproval(approval *common.ValidatorSetApproval, timestamp uint64, checkApproval approvalCheck) error { // First thing we check is if the node that sent this approval is a validator. pk, exists := as.nodeIDToPK[avalanchego.NodeID(approval.NodeID)] if !exists { @@ -82,7 +89,7 @@ func (as *ApprovalStore) HandleApproval(approval *common.ValidatorSetApproval, t // Second thing we check is if the signature of the approval is valid. // We need it to be valid in order for nodes to be able to aggregate it later on along with other approvals. // This is checked before taking the lock, as it only reads immutable state. - if err := as.checkApprovalSignature(approval, pk); err != nil { + if err := checkApproval(approval, pk); err != nil { as.logger.Debug("Received an approval with an invalid signature", zap.String("nodeID", fmt.Sprintf("%x", approval.NodeID)), zap.Uint64("pChainHeight", approval.PChainHeight)) return nil @@ -194,6 +201,20 @@ func (as *ApprovalStore) PutApprovals(approvalStore *ApprovalStore) { as.lock.RUnlock() for _, a := range snapshot { - approvalStore.HandleApproval(&a.ValidatorSetApproval, a.Timestamp) + oldPK := as.nodeIDToPK[a.NodeID] + newPK := approvalStore.nodeIDToPK[a.NodeID] + if oldPK == nil || newPK == nil || !bytes.Equal(oldPK, newPK) { + // Either this node is/was not a validator any longer, + // or the node's public key has changed between the two stores. + // In either case, we cannot verify the signature of this approval, + // so we just invoke HandleApproval to simulate receiving it as a fresh approval. + approvalStore.HandleApproval(&a.ValidatorSetApproval, a.Timestamp) + continue + } + approvalStore.handleApproval(&a.ValidatorSetApproval, a.Timestamp, func(*common.ValidatorSetApproval, []byte) error { + // We don't need to check the signature when copying approvals from one store to another, + // because we already verified the signature when we first received it. + return nil + }) } } diff --git a/msm/approvals_test.go b/msm/approvals_test.go index ba25697d..abc0d190 100644 --- a/msm/approvals_test.go +++ b/msm/approvals_test.go @@ -238,11 +238,22 @@ func TestApprovalStorePutApprovals(t *testing.T) { return common.ValidatorSetApproval{}, false } + errStaleSignature := errors.New("signature does not verify under this key") + for _, tc := range []struct { name string // srcValidators/dstValidators size the two stores via makeValidators; NodeIDs are makeNodeID(i+1). srcValidators int dstValidators int + // srcVdrs/dstVdrs, when non-nil, override the validator set (and therefore the per-node public keys) + // used to build the store, taking precedence over srcValidators/dstValidators. This lets a case give + // the same NodeID a different BLSKey in the two stores to model a key rotation across an epoch change. + srcVdrs NodeBLSMappings + dstVdrs NodeBLSMappings + // dstSigErr, when set, makes the destination store's signature verifier reject every signature. It + // models an approval whose signature no longer verifies under the destination's key; a case combines + // it with a rotated key to prove PutApprovals re-verifies (and drops) instead of copying blindly. + dstSigErr error // srcApprovals are loaded into the source store; dstApprovals are pre-loaded into the destination // store before PutApprovals is called. srcApprovals []approvalAndTimestamp @@ -369,10 +380,73 @@ func TestApprovalStorePutApprovals(t *testing.T) { require.Equal(t, dstSent[0].ValidatorSetApproval, got[0], "the destination approval is retained on a timestamp tie") }, }, + { + // Fast path: the carried-over node keeps the same public key across the two stores, so its + // already-verified signature is copied without re-verification. We prove verification is skipped + // by making the destination verifier reject everything (dstSigErr): the approval must still land. + name: "unchanged public key skips signature re-verification on carry-over", + srcVdrs: NodeBLSMappings{{NodeID: makeNodeID(1), BLSKey: []byte{0xa1}, Weight: 1}}, + dstVdrs: NodeBLSMappings{{NodeID: makeNodeID(1), BLSKey: []byte{0xa1}, Weight: 1}}, // identical key + dstSigErr: errStaleSignature, + srcApprovals: []approvalAndTimestamp{ + {common.ValidatorSetApproval{NodeID: makeNodeID(1), PChainHeight: 7, Signature: signApproval(7, [32]byte{})}, 100}, + }, + verify: func(t *testing.T, dst, _ *ApprovalStore, srcSent, _ []approvalAndTimestamp) { + got := dst.Approvals() + require.Len(t, got, 1, "the approval carries over without re-verifying the signature when the key is unchanged") + require.Equal(t, 1, dst.storedCount) + require.Equal(t, srcSent[0].ValidatorSetApproval, got[0]) + }, + }, + { + // Key-rotation branch: the same NodeID has a different key in + // the destination store. PutApprovals must NOT copy blindly; it re-verifies via HandleApproval, + // and since the signature no longer verifies under the new key (dstSigErr) the stale approval is + // dropped. Against the previous "always skip" implementation this approval would leak into dst. + name: "rotated public key re-verifies and drops the now-invalid approval", + srcVdrs: NodeBLSMappings{{NodeID: makeNodeID(1), BLSKey: []byte{0xa1}, Weight: 1}}, + dstVdrs: NodeBLSMappings{{NodeID: makeNodeID(1), BLSKey: []byte{0xb2}, Weight: 1}}, // key rotated + dstSigErr: errStaleSignature, + srcApprovals: []approvalAndTimestamp{ + {common.ValidatorSetApproval{NodeID: makeNodeID(1), PChainHeight: 7, Signature: signApproval(7, [32]byte{})}, 100}, + }, + verify: func(t *testing.T, dst, _ *ApprovalStore, _, _ []approvalAndTimestamp) { + require.Empty(t, dst.Approvals(), "a stale-key approval is re-verified against the new key and dropped") + require.Equal(t, 0, dst.storedCount) + }, + }, + { + // Key-rotation branch, happy path: the key changed, but the signature still verifies under the new + // key (dstSigErr unset), so the re-verified approval is carried over. This shows the branch does not + // blanket-drop on rotation — it re-verifies and keeps what is still valid. + name: "rotated public key re-verifies and keeps the still-valid approval", + srcVdrs: NodeBLSMappings{{NodeID: makeNodeID(1), BLSKey: []byte{0xa1}, Weight: 1}}, + dstVdrs: NodeBLSMappings{{NodeID: makeNodeID(1), BLSKey: []byte{0xb2}, Weight: 1}}, // key rotated + srcApprovals: []approvalAndTimestamp{ + {common.ValidatorSetApproval{NodeID: makeNodeID(1), PChainHeight: 7, Signature: signApproval(7, [32]byte{})}, 100}, + }, + verify: func(t *testing.T, dst, _ *ApprovalStore, srcSent, _ []approvalAndTimestamp) { + got := dst.Approvals() + require.Len(t, got, 1, "on key rotation the approval is re-verified and, if still valid, carried over") + require.Equal(t, 1, dst.storedCount) + require.Equal(t, srcSent[0].ValidatorSetApproval, got[0]) + }, + }, } { t.Run(tc.name, func(t *testing.T) { - src := NewApprovalStore(&signatureVerifier{}, makeValidators(tc.srcValidators), testutil.MakeLogger(t)) - dst := NewApprovalStore(&signatureVerifier{}, makeValidators(tc.dstValidators), testutil.MakeLogger(t)) + srcVdrs := tc.srcVdrs + if srcVdrs == nil { + srcVdrs = makeValidators(tc.srcValidators) + } + dstVdrs := tc.dstVdrs + if dstVdrs == nil { + dstVdrs = makeValidators(tc.dstValidators) + } + + // The source verifier always accepts, so the fixtures load. Only the destination + // verifier is parameterized, to model a signature that no longer verifies under a rotated key. + src := NewApprovalStore(&signatureVerifier{}, srcVdrs, testutil.MakeLogger(t)) + dst := NewApprovalStore(&signatureVerifier{err: tc.dstSigErr}, dstVdrs, testutil.MakeLogger(t)) for _, a := range tc.srcApprovals { require.NoError(t, src.HandleApproval(&a.ValidatorSetApproval, a.Timestamp)) diff --git a/msm/msm.go b/msm/msm.go index 13764abc..acbd346e 100644 --- a/msm/msm.go +++ b/msm/msm.go @@ -257,7 +257,7 @@ func (sm *StateMachine) HandleApproval(approval *common.ValidatorSetApproval, ti return approvalStore.HandleApproval(approval, timestamp) } -func (sm *StateMachine) maybeInitializeApprovalStore(validatorSet NodeBLSMappings) error { +func (sm *StateMachine) maybeInitializeApprovalStore(validatorSet NodeBLSMappings) *ApprovalStore { sm.lock.Lock() defer sm.lock.Unlock() @@ -270,10 +270,8 @@ func (sm *StateMachine) maybeInitializeApprovalStore(validatorSet NodeBLSMapping if oldApprovalStore != nil { oldApprovalStore.PutApprovals(sm.approvalStore) } - return nil } - - return nil + return sm.approvalStore } // BuildBlock constructs the next block on top of the given parent block, and passes in the provided simplex metadata and blacklist. @@ -1099,8 +1097,8 @@ func (sm *StateMachine) computeNewApprovals(parentBlock *StateMachineBlock, vali // We retrieve approvals that validators have sent us for the next epoch. // These approvals are signed by validators of the next epoch. - sm.maybeInitializeApprovalStore(validators) - approvalsFromPeers := sm.approvalStore.Approvals() + approvalStore := sm.maybeInitializeApprovalStore(validators) + approvalsFromPeers := approvalStore.Approvals() sm.Logger.Debug("Retrieved approvals from peers", zap.Int("numApprovals", len(approvalsFromPeers))) // Optimistically sign the epoch transition even if we have already did so in a previous round. diff --git a/simplex/epoch.go b/simplex/epoch.go index ab58f7f4..5ef7f9d1 100644 --- a/simplex/epoch.go +++ b/simplex/epoch.go @@ -2156,6 +2156,8 @@ func (e *Epoch) createFinalizedBlockVerificationTask(block common.Block, finaliz if err != nil { e.Logger.Debug("Failed verifying block", zap.Error(err)) // if we fail to verify the block, we re-add to request timeout + e.lock.Lock() + defer e.lock.Unlock() e.replicationState.ResendFinalizationRequest(md.Seq, finalization.QC.Signers()) return md.Digest }