wallet: add sign methods to vault interface by GustavoStingelin · Pull Request #1279 · btcsuite/btcwallet

GustavoStingelin · 2026-07-05T19:48:24Z

Adds a digest-level signing surface to the wallet keyvault boundary so future
signing paths can keep private key operations behind the vault.

This PR introduces a shared wallet/signing package with:

sealed key locator variants:
- full HD derivation address via waddrmgr.AddrScope
- account-row child reference
- script pubkey reference
sealed primitive signing request variants:
- ECDSA
- compact recoverable ECDSA
- Schnorr
- tweaked Schnorr
sealed signature result variants with Serialize() helpers

The keyvault interface is extended with:

DerivePubKey
Sign

Sign accepts a sealed signing.Request containing a key locator, a
caller-computed 32-byte digest, and the primitive signing mode. Requests do not
carry transactions, PSBTs, scripts, witnesses, sighash types, or Taproot tree
data. Digest construction, transaction assembly, signature serialization format,
and appended sighash-byte handling remain wallet-layer responsibilities.

Tweaked Schnorr signing uses an already-computed additive secp256k1 scalar plus
an even-Y normalization flag. It does not accept Taproot script roots, merkle
roots, or BIP341 tweak input bytes.

WalletVault currently returns a generic key-operation unimplemented sentinel
for derivation and signing until the backing implementation is added.

The vault mock was moved into its own mock/vault package to avoid an import
cycle with waddrmgr.

GustavoStingelin · 2026-07-05T22:18:57Z

@Abdulkbk ping for review

yyforyongyu · 2026-07-06T01:44:33Z

No issues found by claude-opus-4-8[1m] ✍️

yyforyongyu · 2026-07-06T01:42:32Z

 Keyvault accesses persistence through `db.Store`. It does not talk directly to
 SQL backends.

+The vault API also includes digest level signing methods. `DerivePubKey`,


yyforyongyu · 2026-07-06T02:07:41Z

+}
+
+// SchnorrKeyTweak describes an additive secp256k1 tweak for Schnorr signing.
+type SchnorrKeyTweak struct {


Can we make the conversion semantics explicit here? The existing signer code models taproot tweaks as raw []byte, while this type adds a fixed 32-byte scalar and NormalizeEvenY. That may be the right vault-level shape, but the signer-to-vault adapter needs to define exactly how current taproot tweak bytes map into this descriptor so delegation cannot silently change the tweak being signed with. (gpt-5.5)

Changed to the modular scalar to be explicit.

Adding a bit more detail here: the current shape makes the vault boundary take
the final additive scalar directly.

SchnorrKeyTweak.Scalar is a btcec.ModNScalar, so callers must pass the
already-computed secp256k1 tweak scalar. It is intentionally not a Taproot script
root, merkle root, or BIP341 tweak input byte string.

So any signer-to-vault adapter remains responsible for converting existing raw
Taproot tweak inputs into that final scalar before constructing the
TweakedSchnorrRequest. The vault only applies the scalar tweak and signs the
caller-computed digest.

yyforyongyu · 2026-07-06T02:07:41Z

+	// corresponding public key without exposing private key material.
+	DerivePubKey(ctx context.Context, ref KeyLocator) (*btcec.PublicKey, error)
+
+	// SignECDSA signs a caller-computed 32-byte digest for the provided key


Can the PR document the intended layering between this primitive signing surface and the existing intent-based signer API? Keeping SignDigestIntent in the signer and unpacking it into SignECDSA, SignCompactECDSA, SignSchnorr, or SignTweakedSchnorr makes sense, but the API docs should make clear that the signer owns intent validation and dispatch while the vault intentionally exposes only primitive signing methods. (gpt-5.5)

Created a new wallet/signing package so the vault and public signer can share the same API.

Since the vault needs to support the same signing operations exposed by the public signer, this helps avoid duplicated types and extra conversions between equivalent request and response structs.

Also clarified the layering in the ADR and Vault.Sign docs.

The wallet/signer layer owns the higher-level intent work: choosing the signing
intent, constructing the digest, selecting sighash semantics, assembling
transactions/PSBTs/witnesses, deciding serialization format, and appending any
sighash byte.

The vault boundary stays primitive: it receives a sealed signing.Request for a
located key and caller-computed 32-byte digest, performs the private-key
operation, and returns a primitive signing.Signature.

I am confused about the design - if the signer.go and the keyvault are using the same struct, like sign request param to pass info, there must be sth wrong with the abstraction as how can both the public API and the private internal vault require the same set of info? I think it's better to open another PR to show how it's actually used in the wallet/signer.go first.

yyforyongyu · 2026-07-06T02:07:41Z

+	ScriptPubKey *ScriptPubKeyRef
+}
+
+// FullHDKeyRef identifies a complete wallet-root derivation path.


This should probably reuse the existing derivation types instead of flattening them. The flattened form loses the InternalAccount/Account split and MasterKeyFingerprint; InternalAccount is what DeriveFromKeyPath* uses to load the account row, so this API cannot round-trip the current wallet signing path. Consider modeling this as waddrmgr.KeyScope plus waddrmgr.DerivationPath or a local wrapper that embeds both so the vault locator preserves the full derivation identity. (gpt-5.5)

It intentionally does not use InternalAccount or MasterKeyFingerprint, because we do not need them here. Those fields are tied to the legacy kvdb flow.

I changed it to use the new waddrmgr.AddrScope, which includes the full path.

Abdulkbk

LGTM 🚢

Abdulkbk · 2026-07-06T10:54:06Z

+
+// KeyLocator identifies a key using one of the vault's supported reference
+// forms.
+type KeyLocator struct {


will it make sense to specify what implementations must do when zero or multiple are set and maybe adding a Validate helper to enforce how this will be used?

It was supposed to use only one reference at a time. I converted it to sealed structures behind an interface, so now this is guaranteed by construction, and callers cannot accidentally provide multiple references.

Abdulkbk · 2026-07-06T11:14:52Z

+	//
+	// The caller provides a wallet key locator, and the vault returns the
+	// corresponding public key without exposing private key material.
+	DerivePubKey(ctx context.Context, ref KeyLocator) (*btcec.PublicKey, error)


nit: can specify wether the vault needs be locked/unlocked for the newly added medthods.

Abdulkbk · 2026-07-06T11:17:29Z

+func (v *WalletVault) DerivePubKey(context.Context,
+	KeyLocator) (*btcec.PublicKey, error) {
+
+	return nil, ErrVaultSigningUnimplemented


not important atm, DerivePubKey isn't signing, so ErrVaultSigningUnimplemented doesn't apply.

yeah, that's just a temporary error until the next PR. Renamed.

coveralls · 2026-07-09T01:12:19Z

Coverage Report for CI Build 29040784850

Warning

No base build found for commit 1e0ec3d on sql-wallet.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 55.559%

Details

Patch coverage: 42 uncovered changes across 5 files (9 of 51 lines covered, 17.65%).

Uncovered Changes

File	Changed	Covered	%
wallet/internal/bwtest/mock/vault/vault.go	24	0	0.0%
wallet/internal/keyvault/vault.go	8	0	0.0%
wallet/signing/request.go	4	0	0.0%
wallet/signing/key_locator.go	3	0	0.0%
wallet/signing/signature.go	12	9	75.0%

Coverage Regressions

Requires a base build to compare against. How to fix this →

Coverage Stats


Relevant Lines:	58205
Covered Lines:	32338
Line Coverage:	55.56%
Coverage Strength:	7225.84 hits per line

💛 - Coveralls

GustavoStingelin · 2026-07-09T01:28:51Z

Added a new wallet/signing package with primitives that can be used by the vault now and later reused by the public signer.

This should let us remove some duplicated code later and keep the signing request and response types in one place.

yyforyongyu · 2026-07-09T06:37:14Z

+func TestECDSASignatureSerializeNil(t *testing.T) {
+	t.Parallel()
+
+	// Given


we should fix these?

u mean removing or renaming to "Arrange, Act, Assert"?

removed, btw

yyforyongyu

Cool. I think the current wallet/signing looks premature without another PR to show how it can be applied to wallet/signer.go first. It's weird that both the public Signer and the internal keyvault can share the same signing request, tho not necessarily wrong - either the public layer is a pass-through, so a slim, shallow layer, or the internal layer is leaking. So it will be better to show how this new pkg will be applied first, otherwise I think wallet/signing should be removed and put everything under the internal/keyvault.

There is still the option that we can revert to the previous version where we have individual methods like SignECDSA, SignSchnorr, etc., but without a concrete implementation first it's hard to tell.

yyforyongyu · 2026-07-09T06:48:54Z

+`ScriptPubKeyLocator` for raw or imported rows without exposing private key
+material or database routing.
+
+Wallet code builds a sealed `wallet/signing.Request` containing both the key


is there another consumer of signing? I don't see the necessity of making it a public module, nor making it an independent module. We can just put all the code under wallet/internal/keyvault as the vault is the only consumer?

yyforyongyu · 2026-07-09T07:11:02Z

+	// corresponding public key without exposing private key material.
+	DerivePubKey(ctx context.Context, ref KeyLocator) (*btcec.PublicKey, error)
+
+	// SignECDSA signs a caller-computed 32-byte digest for the provided key


I am confused about the design - if the signer.go and the keyvault are using the same struct, like sign request param to pass info, there must be sth wrong with the abstraction as how can both the public API and the private internal vault require the same set of info? I think it's better to open another PR to show how it's actually used in the wallet/signer.go first.

GustavoStingelin · 2026-07-09T14:58:05Z

@yyforyongyu I think the root issue is that the current
wallet.SignDigest shape does not line up well with the future keyvault path.
It currently assumes the wallet signer can derive/load the private key itself,
but once signing is delegated to the vault, the public signer should not need
that knowledge.

I actually like the shape where the public signer becomes a thin adapter:

// SignDigest signs a message digest based on the provided request.
func (w *Wallet) SignDigest(ctx context.Context,
	request signing.Request) (signing.Signature, error) {

	if err := w.state.canSign(); err != nil {
		return nil, err
	}

	return w.keyVault.Sign(ctx, request)
}

To me, this makes the layering pretty clear: the wallet signer performs
wallet-state authorization, while the vault owns key location and primitive
signing. From that perspective, a shared signing.Request does not necessarily
mean the internal layer is leaking; it can also be the boundary object passed
from the public signer into the vault.
That said, I also understand the concern that this may be premature in this PR,
because the current patch does not yet fully migrate wallet/signer.go to that
shape. Without that follow-through, wallet/signing can look like a public
package introduced mainly for the vault.

So I think there are two reasonable directions:

Keep the shared signing.Request model, where wallet.SignDigest
delegates to keyVault.Sign.
Keep this PR narrower by using explicit vault methods like SignECDSA,
SignCompactECDSA, SignSchnorr, etc., and defer the request/public
signer cleanup to a follow-up PR.

Personally, I like the thin SignDigest -> keyVault.Sign shape, but I also
agree the explicit-method version is less speculative for this PR and easier to
review locally. Since neither of us seems strongly attached to the single-method
vault API, I’m leaning toward switching this PR back to the explicit vault
methods and leaving the public signer for a follow-up.

Mainly, I want to avoid continuing to go back and forth on the API shape in this
PR. Which direction sounds better to you?

saubyk added this to lnd v0.22 Jul 5, 2026

saubyk assigned GustavoStingelin Jul 5, 2026

github-project-automation Bot moved this to Backlog in lnd v0.22 Jul 5, 2026

GustavoStingelin force-pushed the sqldb/add-keyvault-sig-interface branch from 74b08b3 to 588cfd1 Compare July 5, 2026 21:06

GustavoStingelin commented Jul 5, 2026

View reviewed changes

Comment thread wallet/internal/keyvault/keyvault.go Outdated

GustavoStingelin requested a review from yyforyongyu July 5, 2026 22:18

GustavoStingelin added this to the Introduce SQL store milestone Jul 5, 2026

yyforyongyu reviewed Jul 6, 2026

View reviewed changes

Abdulkbk approved these changes Jul 6, 2026

View reviewed changes

wallet: move vault mock under mock namespace

888b64c

GustavoStingelin force-pushed the sqldb/add-keyvault-sig-interface branch from 588cfd1 to c7543c8 Compare July 9, 2026 00:51

GustavoStingelin changed the base branch from sqldb/impl-keyvault-lifecycle to sql-wallet July 9, 2026 00:53

GustavoStingelin force-pushed the sqldb/add-keyvault-sig-interface branch from ee4b8fb to c7543c8 Compare July 9, 2026 01:05

GustavoStingelin requested review from Abdulkbk and yyforyongyu July 9, 2026 01:40

yyforyongyu reviewed Jul 9, 2026

View reviewed changes

Comment thread wallet/signing/key_locator.go

yyforyongyu reviewed Jul 9, 2026

View reviewed changes

Comment thread wallet/signing/request.go Outdated

yyforyongyu reviewed Jul 9, 2026

View reviewed changes

Comment thread wallet/signing/signature.go Outdated

yyforyongyu reviewed Jul 9, 2026

View reviewed changes

yyforyongyu requested changes Jul 9, 2026

View reviewed changes

yyforyongyu removed the request for review from Abdulkbk July 9, 2026 07:42

GustavoStingelin requested a review from yyforyongyu July 9, 2026 17:12

wallet: add signing request types

b54b11f

GustavoStingelin force-pushed the sqldb/add-keyvault-sig-interface branch from c7543c8 to 4adf634 Compare July 9, 2026 17:52

GustavoStingelin added 2 commits July 9, 2026 15:28

wallet: add signing signature types

4cb84c7

wallet: add keyvault digest signing interface

96241f0

GustavoStingelin force-pushed the sqldb/add-keyvault-sig-interface branch from 4adf634 to 96241f0 Compare July 9, 2026 18:28

Uh oh!

Conversation

GustavoStingelin commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

GustavoStingelin commented Jul 5, 2026

Uh oh!

yyforyongyu commented Jul 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abdulkbk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report for CI Build 29040784850

Coverage: 55.559%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

GustavoStingelin commented Jul 9, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GustavoStingelin commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

GustavoStingelin commented Jul 5, 2026 •

edited

Loading

coveralls commented Jul 9, 2026 •

edited

Loading