From 9f143cdb37a9f2474e64d89f6cbf8fdef512f558 Mon Sep 17 00:00:00 2001 From: Yacov Manevich Date: Fri, 24 Jul 2026 01:18:13 +0200 Subject: [PATCH 1/2] Fix data races in replication and MSM - When failing verifying a block, e.replicationState.ResendFinalizationRequest is invoked and it's not thread safe. - computeNewApprovals of MSM referenced the approval store but without memory coherency. Signed-off-by: Yacov Manevich --- msm/msm.go | 10 ++++------ simplex/epoch.go | 2 ++ 2 files changed, 6 insertions(+), 6 deletions(-) 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 } From bd909bc24910fa6d5ea7bdba9b925fadee85a552 Mon Sep 17 00:00:00 2001 From: Yacov Manevich Date: Sun, 26 Jul 2026 01:28:01 +0200 Subject: [PATCH 2/2] Skip signature verification in PutApprovals when same PK PutApprovals copies approvals from the current store to a given store. It snapshots the approvals and then calls HandleApproval on each approval, but it also performs a signature verification which is redundant, because the signature was already checked the first time it was inserted into the current approval store. This commit simply refactors the code to enable PutApprovals to skip the signature check if it has the same PK. Signed-off-by: Yacov Manevich --- msm/approvals.go | 25 ++++++++++++-- msm/approvals_test.go | 78 +++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 99 insertions(+), 4 deletions(-) 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))