fix: cancel signing only when collected signatures meet the threshold

core/v3/signature_build_test.go

@@ -0,0 +1,90 @@
+package v3_test
+
+import (
+	"bytes"
+	"context"
+	"math/big"
+	"testing"
+
+	"github.com/0xsequence/ethkit/go-ethereum/common"
+	"github.com/0xsequence/go-sequence/core"
+	v3 "github.com/0xsequence/go-sequence/core/v3"
+	"github.com/stretchr/testify/require"
+)
+
+// A subdigest leaf reports max signersWeight for any payload, so the config threshold
+// looks met as soon as the first signature is collected. Early cancellation on that
+// estimate nondeterministically drops the other signer's signature even though recovery
+// of a payload not matching the subdigest still needs it. Every build must embed both
+// signatures and produce identical bytes.
+func TestBuildRegularSignatureCollectsAllSignersDespiteSubdigestLeaf(t *testing.T) {
+	signerA := common.HexToAddress("0x1111111111111111111111111111111111111111")
+	signerB := common.HexToAddress("0x2222222222222222222222222222222222222222")
+
+	dummySignature := func(fill byte) []byte {
+		sig := make([]byte, 65)
+		for i := range 64 {
+			sig[i] = fill
+		}
+		sig[64] = 27
+		return sig
+	}
+	signatureA := dummySignature(0xaa)
+	signatureB := dummySignature(0xbb)
+
+	config := &v3.WalletConfig{
+		Threshold_: 2,
+		Tree: v3.WalletConfigTreeNodes(
+			v3.WalletConfigTreeSubdigestLeaf{Subdigest: common.BigToHash(big.NewInt(1))},
+			&v3.WalletConfigTreeAddressLeaf{Weight: 1, Address: signerA},
+			&v3.WalletConfigTreeAddressLeaf{Weight: 1, Address: signerB},
+		),
+	}
+
+	signatures := map[common.Address][]byte{
+		signerA: signatureA,
+		signerB: signatureB,
+	}
+	signingFunc := func(ctx context.Context, signer core.Signer, _ []core.SignerSignature) (core.SignerSignatureType, []byte, error) {
+		if sig, ok := signatures[signer.Address]; ok {
+			return core.SignerSignatureTypeEthSign, sig, nil
+		}
+		return 0, nil, nil
+	}
+
+	var first []byte
+	for range 100 {
+		sig, err := config.BuildRegularSignature(context.Background(), signingFunc, true)
+		require.NoError(t, err)
+		data, err := sig.Data()
+		require.NoError(t, err)
+		require.True(t, bytes.Contains(data, signatureA[:64]), "signer A's signature must be embedded on every build")
+		require.True(t, bytes.Contains(data, signatureB[:64]), "signer B's signature must be embedded on every build")
+		if first == nil {
+			first = data
+		}
+		require.Equal(t, first, data, "signature encoding must be deterministic")
+	}
+}
+
+// A subdigest leaf reports max signersWeight even for an empty signer set, so
+// signing-power validation must still fail when no signature is collected at all.
+func TestBuildSignatureValidationRejectsEmptySignerSetDespiteSubdigestLeaf(t *testing.T) {
+	config := &v3.WalletConfig{
+		Threshold_: 2,
+		Tree: v3.WalletConfigTreeNodes(
+			v3.WalletConfigTreeSubdigestLeaf{Subdigest: common.BigToHash(big.NewInt(1))},
+			&v3.WalletConfigTreeAddressLeaf{Weight: 1, Address: common.HexToAddress("0x1111111111111111111111111111111111111111")},
+		),
+	}
+
+	signingFunc := func(ctx context.Context, signer core.Signer, _ []core.SignerSignature) (core.SignerSignatureType, []byte, error) {
+		return 0, nil, nil
+	}
+
+	_, err := config.BuildRegularSignature(context.Background(), signingFunc, true)
+	require.ErrorContains(t, err, "not enough signers")
+
+	_, err = config.BuildNoChainIDSignature(context.Background(), signingFunc, true)
+	require.ErrorContains(t, err, "not enough signers")
+}

core/v3/v3.go

@@ -2001,6 +2001,7 @@ func (c *WalletConfig) BuildSubdigestSignature(noChainID bool) (core.Signature[*
 func (c *WalletConfig) BuildRegularSignature(ctx context.Context, sign core.SigningFunction, validateSigningPower bool, checkpointerData ...[]byte) (core.Signature[*WalletConfig], error) {
 	var isValid bool
 	configSigners := c.Signers()
+	threshold := new(big.Int).SetUint64(uint64(c.Threshold_))
 
 	signCtx, signCancel := context.WithCancel(ctx)
 	defer signCancel()
@@ -2020,9 +2021,15 @@ func (c *WalletConfig) BuildRegularSignature(ctx context.Context, sign core.Sign
 			signerSignatures[signerSignature.Signer] = signerSignature
 			signedSigners[signerSignature.Signer] = configSigners[signerSignature.Signer]
 
-			weight := c.Tree.signersWeight(signedSigners)
-			if weight.Cmp(new(big.Int).SetUint64(uint64(c.Threshold_))) >= 0 {
+			// Cancel outstanding signers only once collected signatures alone meet the
+			// threshold. signersWeight counts payload-blind subdigest leaves as satisfied,
+			// and cancelling on that estimate nondeterministically drops signatures that
+			// recovery of a non-matching payload still needs.
+			if c.Tree.collectedSignersWeight(signedSigners).Cmp(threshold) >= 0 {
 				signCancel()
+			}
+
+			if c.Tree.signersWeight(signedSigners).Cmp(threshold) >= 0 {
 				isValid = true
 			}
 		}
@@ -2050,6 +2057,7 @@ func (c *WalletConfig) BuildRegularSignature(ctx context.Context, sign core.Sign
 func (c *WalletConfig) BuildNoChainIDSignature(ctx context.Context, sign core.SigningFunction, validateSigningPower bool, checkpointerData ...[]byte) (core.Signature[*WalletConfig], error) {
 	var isValid bool
 	configSigners := c.Signers()
+	threshold := new(big.Int).SetUint64(uint64(c.Threshold_))
 
 	signCtx, signCancel := context.WithCancel(ctx)
 	defer signCancel()
@@ -2069,9 +2077,15 @@ func (c *WalletConfig) BuildNoChainIDSignature(ctx context.Context, sign core.Si
 			signerSignatures[signerSignature.Signer] = signerSignature
 			signedSigners[signerSignature.Signer] = configSigners[signerSignature.Signer]
 
-			weight := c.Tree.signersWeight(signedSigners)
-			if weight.Cmp(new(big.Int).SetUint64(uint64(c.Threshold_))) >= 0 {
+			// Cancel outstanding signers only once collected signatures alone meet the
+			// threshold. signersWeight counts payload-blind subdigest leaves as satisfied,
+			// and cancelling on that estimate nondeterministically drops signatures that
+			// recovery of a non-matching payload still needs.
+			if c.Tree.collectedSignersWeight(signedSigners).Cmp(threshold) >= 0 {
 				signCancel()
+			}
+
+			if c.Tree.signersWeight(signedSigners).Cmp(threshold) >= 0 {
 				isValid = true
 			}
 		}
@@ -2126,6 +2140,11 @@ type WalletConfigTree interface {
 	isComplete() bool
 	maxWeight() *big.Int
 	signersWeight(signers map[core.Signer]uint16) *big.Int
+	// collectedSignersWeight is signersWeight with payload-blind leaves (subdigest
+	// leaves, which claim max weight for any payload) counting zero, so it only
+	// reflects weight from actually collected signatures. Safe to use for early
+	// cancellation decisions; signersWeight is not.
+	collectedSignersWeight(signers map[core.Signer]uint16) *big.Int
 	readSignersIntoMap(signers map[core.Signer]uint16)
 	buildSignatureTree(signerSignatures map[core.Signer]core.SignerSignature) signatureTree
 }
@@ -2291,6 +2310,10 @@ func (n *WalletConfigTreeNode) signersWeight(signers map[core.Signer]uint16) *bi
 	return new(big.Int).Add(n.Left.signersWeight(signers), n.Right.signersWeight(signers))
 }
 
+func (n *WalletConfigTreeNode) collectedSignersWeight(signers map[core.Signer]uint16) *big.Int {
+	return new(big.Int).Add(n.Left.collectedSignersWeight(signers), n.Right.collectedSignersWeight(signers))
+}
+
 func (n *WalletConfigTreeNode) readSignersIntoMap(signers map[core.Signer]uint16) {
 	n.Left.readSignersIntoMap(signers)
 	n.Right.readSignersIntoMap(signers)
@@ -2369,6 +2392,10 @@ func (l *WalletConfigTreeAddressLeaf) signersWeight(signers map[core.Signer]uint
 	}
 }
 
+func (l *WalletConfigTreeAddressLeaf) collectedSignersWeight(signers map[core.Signer]uint16) *big.Int {
+	return l.signersWeight(signers)
+}
+
 func (l *WalletConfigTreeAddressLeaf) readSignersIntoMap(signers map[core.Signer]uint16) {
 	signers[core.Signer{Address: l.Address}] = uint16(l.Weight)
 }
@@ -2466,6 +2493,10 @@ func (l WalletConfigTreeNodeLeaf) signersWeight(signers map[core.Signer]uint16)
 	return new(big.Int)
 }
 
+func (l WalletConfigTreeNodeLeaf) collectedSignersWeight(signers map[core.Signer]uint16) *big.Int {
+	return new(big.Int)
+}
+
 func (l WalletConfigTreeNodeLeaf) readSignersIntoMap(signers map[core.Signer]uint16) {
 }
 
@@ -2553,6 +2584,13 @@ func (l *WalletConfigTreeNestedLeaf) signersWeight(signers map[core.Signer]uint1
 	return new(big.Int)
 }
 
+func (l *WalletConfigTreeNestedLeaf) collectedSignersWeight(signers map[core.Signer]uint16) *big.Int {
+	if l.Tree.collectedSignersWeight(signers).Cmp(new(big.Int).SetUint64(uint64(l.Threshold))) >= 0 {
+		return new(big.Int).SetUint64(uint64(l.Weight))
+	}
+	return new(big.Int)
+}
+
 func (l *WalletConfigTreeNestedLeaf) readSignersIntoMap(signers map[core.Signer]uint16) {
 	l.Tree.readSignersIntoMap(signers)
 }
@@ -2619,6 +2657,10 @@ func (l WalletConfigTreeSubdigestLeaf) signersWeight(signers map[core.Signer]uin
 	return new(big.Int).Set(maxUint256)
 }
 
+func (l WalletConfigTreeSubdigestLeaf) collectedSignersWeight(signers map[core.Signer]uint16) *big.Int {
+	return new(big.Int)
+}
+
 func (l WalletConfigTreeSubdigestLeaf) readSignersIntoMap(signers map[core.Signer]uint16) {
 }
 
@@ -2709,6 +2751,10 @@ func (l *WalletConfigTreeSapientSignerLeaf) signersWeight(signers map[core.Signe
 	}
 }
 
+func (l *WalletConfigTreeSapientSignerLeaf) collectedSignersWeight(signers map[core.Signer]uint16) *big.Int {
+	return l.signersWeight(signers)
+}
+
 func (l *WalletConfigTreeSapientSignerLeaf) readSignersIntoMap(signers map[core.Signer]uint16) {
 	signers[core.SapientSigner(l.Address, l.ImageHash_.Hash)] = uint16(l.Weight)
 }
@@ -2785,6 +2831,10 @@ func (l WalletConfigTreeAnyAddressSubdigestLeaf) signersWeight(signers map[core.
 	return new(big.Int).Set(maxUint256)
 }
 
+func (l WalletConfigTreeAnyAddressSubdigestLeaf) collectedSignersWeight(signers map[core.Signer]uint16) *big.Int {
+	return new(big.Int)
+}
+
 func (l WalletConfigTreeAnyAddressSubdigestLeaf) readSignersIntoMap(signers map[core.Signer]uint16) {
 }