diff --git a/pkg/arkd-wallet/core/application/wallet/service.go b/pkg/arkd-wallet/core/application/wallet/service.go index 5bd66271b..35616aa8a 100644 --- a/pkg/arkd-wallet/core/application/wallet/service.go +++ b/pkg/arkd-wallet/core/application/wallet/service.go @@ -677,7 +677,13 @@ func (w *wallet) SignTransaction( if len(input.TaprootLeafScript) > 0 { var signingKey *btcec.PrivateKey if signMode == application.SignModeSigner { - signingKey = w.signerKeyForLeaf(input.TaprootLeafScript[0].Script) + leafKey, err := w.signerKeyForLeaf( + input.TaprootLeafScript[0].Script, len(inputIndexes) > 0, + ) + if err != nil { + return "", err + } + signingKey = leafKey } else { signingKey = w.keyMgr.forfeitPrvkey } @@ -794,38 +800,68 @@ func (w *wallet) SignTransaction( return ptx.B64Encode() } -// signerKeyForLeaf returns the deprecated signer key referenced by the leaf, or the current SignerKey. -func (w *wallet) signerKeyForLeaf(leafScript []byte) *btcec.PrivateKey { - if len(w.DeprecatedSignerKeys) == 0 { - return w.SignerKey +// signerKeyForLeaf returns the wallet key that signs the given tapscript leaf: +// the current SignerKey or a deprecated one, whichever the leaf's multisig closure +// references. When the leaf references none of the wallet's keys, a required input +// (caller passed explicit indexes) is a hard error rather than a silent wrong-key +// signature, while a best-effort one falls back to the current key. Non-multisig +// leaves also fall back to the current key. +func (w *wallet) signerKeyForLeaf(leafScript []byte, required bool) (*btcec.PrivateKey, error) { + leafKeys, ok := multisigClosureKeys(leafScript) + if !ok { + return w.SignerKey, nil } + if keyInLeaf(w.SignerKey, leafKeys) { + return w.SignerKey, nil + } + for _, k := range w.DeprecatedSignerKeys { + if keyInLeaf(k.Key, leafKeys) { + return k.Key, nil + } + } + + if required { + return nil, fmt.Errorf( + "no signer key for tapscript leaf: it references none of the wallet's keys " + + "(current or deprecated); a rotated signer key may not have been retained " + + "as a deprecated key", + ) + } + return w.SignerKey, nil +} + +// multisigClosureKeys returns the public keys of a multisig-bearing closure and +// whether the leaf could be decoded as one. +func multisigClosureKeys(leafScript []byte) ([]*btcec.PublicKey, bool) { closure, err := script.DecodeClosure(leafScript) if err != nil { - return w.SignerKey + return nil, false } - - leafKeys := make([]*btcec.PublicKey, 0) switch c := closure.(type) { case *script.MultisigClosure: - leafKeys = c.PubKeys + return c.PubKeys, true case *script.CLTVMultisigClosure: - leafKeys = c.PubKeys + return c.PubKeys, true case *script.ConditionMultisigClosure: - leafKeys = c.PubKeys + return c.PubKeys, true default: - return w.SignerKey + return nil, false } +} - for _, k := range w.DeprecatedSignerKeys { - want := schnorr.SerializePubKey(k.Key.PubKey()) - for _, pubkey := range leafKeys { - if bytes.Equal(schnorr.SerializePubKey(pubkey), want) { - return k.Key - } +// keyInLeaf reports whether key is one of the leaf's multisig public keys. +func keyInLeaf(key *btcec.PrivateKey, leafKeys []*btcec.PublicKey) bool { + if key == nil { + return false + } + want := schnorr.SerializePubKey(key.PubKey()) + for _, pubkey := range leafKeys { + if bytes.Equal(schnorr.SerializePubKey(pubkey), want) { + return true } } - return w.SignerKey + return false } // WithdrawAll withdraws all available balance including connectors account funds diff --git a/pkg/arkd-wallet/core/application/wallet/signer_keys_test.go b/pkg/arkd-wallet/core/application/wallet/signer_keys_test.go index 3424c7852..77aefa989 100644 --- a/pkg/arkd-wallet/core/application/wallet/signer_keys_test.go +++ b/pkg/arkd-wallet/core/application/wallet/signer_keys_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + arklib "github.com/arkade-os/arkd/pkg/ark-lib" "github.com/arkade-os/arkd/pkg/ark-lib/script" "github.com/arkade-os/arkd/pkg/arkd-wallet/core/application" "github.com/btcsuite/btcd/btcec/v2" @@ -133,6 +134,81 @@ func leafScript(t *testing.T, owner, signer *btcec.PublicKey) []byte { return s } +// TestSignerKeyForLeaf covers the key selection used when signing tapscript leaves: +// the wallet signs with whichever of its keys (current or deprecated) the leaf's +// multisig closure references; a required leaf that references none of the wallet's +// keys is a hard error instead of a silent wrong-key signature. +func TestSignerKeyForLeaf(t *testing.T) { + mustKey := func() *btcec.PrivateKey { + k, err := btcec.NewPrivateKey() + require.NoError(t, err) + return k + } + current, deprecated, user, stranger := mustKey(), mustKey(), mustKey(), mustKey() + + multisigLeaf := func(keys ...*btcec.PublicKey) []byte { + s, err := (&script.MultisigClosure{ + PubKeys: keys, Type: script.MultisigTypeChecksig, + }).Script() + require.NoError(t, err) + return s + } + pub := func(k *btcec.PrivateKey) []byte { return schnorr.SerializePubKey(k.PubKey()) } + + w := &wallet{WalletOptions: WalletOptions{ + SignerKey: current, + DeprecatedSignerKeys: []DeprecatedSignerKey{{Key: deprecated}}, + }} + + t.Run("current key in multisig leaf", func(t *testing.T) { + key, err := w.signerKeyForLeaf(multisigLeaf(user.PubKey(), current.PubKey()), true) + require.NoError(t, err) + require.Equal(t, pub(current), pub(key)) + }) + + // The fix must select the deprecated key for every multisig closure type the + // server co-signs (plain, CLTV, and condition multisig), which previously fell + // through to the current key (the regression surface). + t.Run("deprecated key matched across all closure types", func(t *testing.T) { + ms := script.MultisigClosure{ + PubKeys: []*btcec.PublicKey{user.PubKey(), deprecated.PubKey()}, + Type: script.MultisigTypeChecksig, + } + cond := []byte{txscript.OP_TRUE} + closures := map[string]script.Closure{ + "multisig": &ms, + "cltv": &script.CLTVMultisigClosure{MultisigClosure: ms, Locktime: arklib.AbsoluteLocktime(1000)}, + "condition": &script.ConditionMultisigClosure{MultisigClosure: ms, Condition: cond}, + } + for name, c := range closures { + t.Run(name, func(t *testing.T) { + leaf, err := c.Script() + require.NoError(t, err) + key, err := w.signerKeyForLeaf(leaf, true) + require.NoError(t, err) + require.Equal(t, pub(deprecated), pub(key)) + }) + } + }) + + t.Run("required leaf with no held key errors", func(t *testing.T) { + _, err := w.signerKeyForLeaf(multisigLeaf(user.PubKey(), stranger.PubKey()), true) + require.ErrorContains(t, err, "no signer key for tapscript leaf") + }) + + t.Run("best-effort leaf with no held key falls back to current", func(t *testing.T) { + key, err := w.signerKeyForLeaf(multisigLeaf(user.PubKey(), stranger.PubKey()), false) + require.NoError(t, err) + require.Equal(t, pub(current), pub(key)) + }) + + t.Run("non-multisig leaf falls back to current", func(t *testing.T) { + key, err := w.signerKeyForLeaf([]byte{0x01, 0x02, 0x03}, true) + require.NoError(t, err) + require.Equal(t, pub(current), pub(key)) + }) +} + func TestSignTransactionRejectsPartialSigHashTypes(t *testing.T) { owner, err := btcec.NewPrivateKey() require.NoError(t, err)