wallet: impl keyvault lifecycle by GustavoStingelin · Pull Request #1273 · btcsuite/btcwallet

GustavoStingelin · 2026-06-18T04:51:45Z

Change Description

This PR implements the keyvault.Vault interface in the DBVault struct backed by db.Store.
Still uses the old salsa20poly1305 implemented by the snacl package.

Steps to Test

make unit pkg=wallet/internal/keyvault/...
make unit-race pkg=wallet/internal/keyvault/...

coveralls · 2026-06-18T04:57:58Z

Coverage Report for CI Build 28334091658

Warning

No base build found for commit 90eaacc 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: 54.731%

Details

Patch coverage: 68 uncovered changes across 5 files (255 of 323 lines covered, 78.95%).

Uncovered Changes

File	Changed	Covered	%
wallet/internal/keyvault/vault_refreshpassphrase.go	123	82	66.67%
wallet/internal/keyvault/vault_unlock.go	111	94	84.68%
wallet/internal/bwtest/mock/vault.go	5	0	0.0%
wallet/internal/keyvault/vault_encrypt.go	31	28	90.32%
wallet/internal/keyvault/vault.go	17	15	88.24%
Total (8 files)	323	255	78.95%

Coverage Regressions

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

Coverage Stats


Relevant Lines:	54581
Covered Lines:	29873
Line Coverage:	54.73%
Coverage Strength:	17755.71 hits per line

💛 - Coveralls

GustavoStingelin · 2026-06-19T03:43:18Z

+	case waddrmgr.CKTPublic:
+		return nil, fmt.Errorf("public crypto key: %w",
+			errUnsupportedCryptoKeyType)


We will need a compatibility layer to decrypt this type of data from the legacy kvdb backend

for kvdb we will just return an error if it's not supported atm.

yyforyongyu

Cool very clean commits!

I think the name DBVault implies this is a static db-related struct, while I think we wanna build a vault as a service? The intended architecture is,

wallet level: the user facing front, provides all public APIs
keyvault: the middle layer, handles locking/unlocking/secret rotation
db: the raw layer, provides CRUD

The current keyvault impl seems to be missing that - it's not a pure vault for DB, as it handles timers, and it has an implicit lifecycle there, based on the mutex flow. I think what we want here is a full sub-service that takes over mainLoop (which atm is just a locking service), and handles auto locking - otherwise, we should leave the timer to be handled by the main loop, and this vault should provide only encrypt/decrypt/rotate functionalities.

GustavoStingelin · 2026-06-25T19:00:58Z

@yyforyongyu I responded in the threads.

What name do you think works better for keyvault.DBVault? Maybe keyvault.WalletVault or keyvault.StoreVault? We already have the keyvault.Vault interface, so I think we should avoid making the concrete implementation name too generic.

Also, my next PR will expand the interface to include signing functions. The goal is for the vault to know as little as possible about transactions, while still keeping private keys inside the vault. The design is not done yet, but it will be close to this:

	DerivePubKey(ctx context.Context, ref KeyLocator) (*btcec.PublicKey, error)

	SignECDSA(ctx context.Context, ref KeyLocator,
		digest [32]byte) (*ecdsa.Signature, error)

	SignSchnorr(ctx context.Context, ref KeyLocator,
		digest [32]byte) (*schnorr.Signature, error)

	SignTweakedSchnorr(ctx context.Context, ref KeyLocator,
		digest [32]byte, tweak SchnorrKeyTweak) (*schnorr.Signature, error)

So I think we should also keep that in mind when naming it. The vault will not be only a decrypt and encrypt layer.

What are your thoughts?

yyforyongyu · 2026-06-25T19:42:14Z

So I think we should also keep that in mind when naming it. The vault will not be only a decrypt and encrypt layer.

Yeah right, i mean all the secret stuff - if we wanna keep it simple, we can remove the auto locker and let the lifecycle of locking/unlocking be handled by wallet. Or we can expand this to be a component, sort like other components, Controller, Syncer etc, tho we haven't peeled them off yet.

What name do you think works better for keyvault.DBVault? Maybe keyvault.WalletVault or keyvault.StoreVault

Naming indeed can be challenging - I think it depends on what we wanna make keyvault do here - if it's pure encrypt/decrypt/signing, DBVault can work(or StoreVault to keep the concepts aligned) as it's now just a thin layer above the DB. If we wanna expand it, can rename it to keyvault.Service? Also don't have good options i guess.

As for the direction - it's up to you to decide, think there are some implementation details from your local branch😺

GustavoStingelin · 2026-06-25T20:29:55Z

I will create a new commit using the channel approach to try it out, but it looks like it will force us to serialize the requests. With the mutex approach, we can still allow parallelism by using sync.RWMutex.

yyforyongyu · 2026-06-26T16:11:02Z

I will create a new commit using the channel approach to try it out, but it looks like it will force us to serialize the requests. With the mutex approach, we can still allow parallelism by using sync.RWMutex.

Ok let's keep it simple - keep DBVault thin and small, leave the auto locking and lifecycle management to be handled by Controller, and use the mutex to guard simple operations. This means timeout should not be part of keyvault.Vault.Unlock. The timeout belongs to Wallet.UnlockRequest/controller policy, not the passive vault.

GustavoStingelin · 2026-06-26T16:44:17Z

Ok let's keep it simple - keep DBVault thin and small, leave the auto locking and lifecycle management to be handled by Controller, and use the mutex to guard simple operations. This means timeout should not be part of keyvault.Vault.Unlock. The timeout belongs to Wallet.UnlockRequest/controller policy, not the passive vault.

cool, agreed, pushed this one just as a future reference GustavoStingelin@b9670fb#diff-35d82ef33e113def7ecb59fc4ee94faf61459ce701d755589b9673f9794fa0cc

GustavoStingelin · 2026-06-28T20:03:35Z

@yyforyongyu ready for review

yyforyongyu · 2026-06-29T01:19:27Z

+// Unlock loads wallet secrets from the store and decrypts them into runtime
+// state using the provided private passphrase. If the vault is already
+// unlocked, Unlock is a no-op and does not validate the passphrase.
+func (v *WalletVault) Unlock(ctx context.Context, passphrase []byte) error {


This unlock path is never wired into the wallet lifecycle: handleUnlockReq still only calls DBUnlock, and handleLockReq only locks addrStore. The concrete vault installed on Wallet therefore keeps unlockedState == nil, so existing wallet paths such as account creation that call w.keyVault.Decrypt(CKTPrivate, ...) will fail with ErrVaultLocked even after a successful wallet unlock. Please either unlock/lock the vault from the controller lifecycle with a Store that can load secrets, or keep the concrete vault out of Wallet until that wiring exists. (gpt-5.5)

Can ignore given that this is an intermediate PR.

yyforyongyu · 2026-06-29T01:22:23Z

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

yyforyongyu

Cool very clean now! Left a few comments about the rotation, filenames, typos, and simplification of the zeroing logic.

yyforyongyu · 2026-06-29T02:23:26Z

+	// RefreshPrivatePassphrase rotates persisted wallet secrets to the provided
+	// new private passphrase. The vault must already be unlocked when this
+	// method is called.
+	RefreshPrivatePassphrase(ctx context.Context, passphrase []byte) error


Q: so this is now basically ChangePassphrase?

Yes, but I can easy change to ChangePassphrase if u think it is clearer

yyforyongyu · 2026-06-29T02:31:15Z

+	case waddrmgr.CKTPublic:
+		return nil, fmt.Errorf("public crypto key: %w",
+			errUnsupportedCryptoKeyType)


for kvdb we will just return an error if it's not supported atm.

yyforyongyu · 2026-06-29T02:31:21Z

+package keyvault
+
+// IsLocked reports whether the vault currently has unlocked runtime state.
+func (v *WalletVault) IsLocked() bool {


I notice that the filenames are mimicking the db store filenames, is this intentional? We previously did this on the db store files because a single file can be too large, so we applied the rules only on the store filenames. It's also nice there because each db ops is very independent. Here I would say it's better to move them into the single vault.go as they are closely related.

The split was intentional. I wanted to avoid creating a very large file and make it easier to separate private functions by responsibility. That said, I can join everything into a single file if you still prefer.

yyforyongyu · 2026-06-29T02:33:49Z

+}
+
+// TestDBVaultLockWaitsForInFlightUnlock verifies that explicit Lock calls are
+// ordered after an Unlock that has already entered the vault lifecycle.


I am not following this test - are we testing the mutex behavior here? seems unnecessary?

Yes, since we have removed autolock, this test no longer makes much sense.

yyforyongyu · 2026-06-29T02:36:51Z

+			"rotate secrets: %w", v.walletID, err)
+	}
+
+	// validate that the rotated secrets derive the same runtime keys before


nit: looks like all the inline comments are not using title case - weird that the agent could also make typos.

I wrote it manually 🤣, will fix the case.

yyforyongyu · 2026-06-29T02:41:26Z

+
+// RefreshPrivatePassphrase rotates persisted wallet secrets to the new private
+// passphrase and keeps the existing unlocked runtime state unchanged.
+func (v *WalletVault) RefreshPrivatePassphrase(ctx context.Context,


If WalletVault is meant to stay dead simple/passive, RefreshPrivatePassphrase should probably not be on the Vault interface. It makes the vault own a higher-level workflow:

load persisted secrets

derive a new master private key from the new passphrase

re-encrypt wallet secrets

validate them

write them back to the store

That feels like controller/store orchestration, not a thin vault primitive.

A cleaner split would be:

Controller handles ChangePassphraseRequest.

Controller validates policy/state and old passphrase.

Store transaction updates persisted wallet secret blobs.

Vault is only told to Lock or maybe re-Unlock after the change if needed.

I agree. Additionally, because this method holds v.mtx.Lock(), the same lock used by Encrypt and Decrypt, a slow RefreshPrivatePassphrase will block every Encrypt and Decrypt.

The same can be said for Unlock (i.e., it holds the same lock and performs DB ops), but Unlock is typically called once per session.

not sure, I will elaborate.

yyforyongyu · 2026-06-29T02:42:36Z

+// Unlock loads wallet secrets from the store and decrypts them into runtime
+// state using the provided private passphrase. If the vault is already
+// unlocked, Unlock is a no-op and does not validate the passphrase.
+func (v *WalletVault) Unlock(ctx context.Context, passphrase []byte) error {


Can ignore given that this is an intermediate PR.

yyforyongyu · 2026-06-29T02:46:28Z

+	defer v.mtx.Unlock()
+
+	if v.unlockedState != nil {
+		return nil


Think this case should be an error?

It was documented in struct def as:

If Unlock is called while the vault is already unlocked, it must be a no-op and must not validate the provided passphrase.

yyforyongyu · 2026-06-29T02:53:38Z

+	// including the master HD private key that can spend coins.
+	state := &unlockedState{}
+
+	clearState := true


This deserves some inline comments to explain why we wanna clear the state, like defensive programming and GC can take time etc.

yyforyongyu · 2026-06-29T02:55:02Z

+
+	clearState := true
+	defer func() {
+		if clearState {


looking at this again, think there is a cleaner and simpler version, as we don't need to trace the clearState to understand the flow,

cryptoKeyPrivate, err := decryptCryptoKey( &masterPrivateKey, secrets.EncryptedCryptoPrivKey, ) if err != nil { return nil, fmt.Errorf("crypto key private: %w", err) } defer cryptoKeyPrivate.Zero() cryptoKeyScript, err := decryptCryptoKey( &masterPrivateKey, secrets.EncryptedCryptoScriptKey, ) if err != nil { return nil, fmt.Errorf("crypto key script: %w", err) } defer cryptoKeyScript.Zero() // ... decrypt/parse hdRootKey ... state := &unlockedState{ cryptoKeyPrivate: cryptoKeyPrivate, cryptoKeyScript: cryptoKeyScript, hdRootKey: hdRootKey, } // Transfer ownership to state; don't zero locals after this point.

cool, much simpler this one!

Abdulkbk

Nice work and easy to review!

Left some feedback.

Abdulkbk · 2026-06-29T10:35:33Z

 // NewDBVault creates a key-vault bridge scoped to one wallet row.
-func NewDBVault(store db.Store, walletID uint32) *DBVault {
-	return &DBVault{
+func NewDBVault(store db.Store, walletID uint32) *WalletVault {


should we also update the constructor's name and callsites to NewWalletVault?

Abdulkbk · 2026-06-29T10:40:32Z

+	defer v.mtx.Unlock()
+
+	if v.unlockedState != nil {
+		return nil


It was documented in struct def as:

If Unlock is called while the vault is already unlocked, it must be a no-op and must not validate the provided passphrase.

Abdulkbk · 2026-06-29T11:16:09Z

+			v.walletID, err)
+	}
+
+	// After a successful unlockedState construction, clear any existing runtime


nit: update comment to reflect that v.clearRuntimeAndLock() is defensive here (or drop) since we already passed through a nil check above (and we held the mtx lock):

if v.unlockedState != nil { return nil }

v.clearRuntimeAndLock() is no-op.

func (v *WalletVault) clearRuntimeAndLock() { if v.unlockedState != nil { v.unlockedState.zero() v.unlockedState = nil } }

Abdulkbk · 2026-06-29T11:34:23Z

+
+// RefreshPrivatePassphrase rotates persisted wallet secrets to the new private
+// passphrase and keeps the existing unlocked runtime state unchanged.
+func (v *WalletVault) RefreshPrivatePassphrase(ctx context.Context,


I agree. Additionally, because this method holds v.mtx.Lock(), the same lock used by Encrypt and Decrypt, a slow RefreshPrivatePassphrase will block every Encrypt and Decrypt.

The same can be said for Unlock (i.e., it holds the same lock and performs DB ops), but Unlock is typically called once per session.

Abdulkbk · 2026-06-29T11:45:08Z

+	var encryptedHDRootKey []byte
+	if v.unlockedState.hdRootKey != nil {
+		encryptedHDRootKey, err = v.unlockedState.cryptoKeyPrivate.Encrypt(
+			[]byte(v.unlockedState.hdRootKey.String()),


Might as well add a todo here because this seems to leak priv material too. Like you did for decryptWalletSecrets:

// TODO(gus): wrap with secret.Do from golang 1.26+....

saubyk assigned GustavoStingelin Jun 18, 2026

saubyk added this to lnd v0.22 Jun 18, 2026

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

GustavoStingelin force-pushed the sqldb/impl-keyvault-lifecycle branch 3 times, most recently from 662eceb to 2c9cd5c Compare June 19, 2026 03:27

GustavoStingelin commented Jun 19, 2026

View reviewed changes

Comment thread wallet/internal/keyvault/vault_unlock.go

GustavoStingelin commented Jun 19, 2026

View reviewed changes

GustavoStingelin added this to the Introduce SQL store milestone Jun 19, 2026

GustavoStingelin requested a review from yyforyongyu June 19, 2026 03:43

GustavoStingelin marked this pull request as ready for review June 19, 2026 03:44

yyforyongyu reviewed Jun 25, 2026

View reviewed changes

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

yyforyongyu reviewed Jun 25, 2026

View reviewed changes

Comment thread wallet/internal/keyvault/vault_encrypt.go

yyforyongyu reviewed Jun 25, 2026

View reviewed changes

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

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

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

yyforyongyu requested changes Jun 25, 2026

View reviewed changes

GustavoStingelin requested a review from yyforyongyu June 25, 2026 19:08

yyforyongyu force-pushed the sql-wallet branch from 64b514c to 8139f20 Compare June 26, 2026 16:23

yyforyongyu force-pushed the sql-wallet branch from 8139f20 to 90eaacc Compare June 26, 2026 22:36

GustavoStingelin force-pushed the sqldb/impl-keyvault-lifecycle branch 3 times, most recently from ccf4548 to c44b2f5 Compare June 28, 2026 16:58

GustavoStingelin added 3 commits June 28, 2026 16:31

wallet: remove keyvault auto lock

7cfbad8

wallet: add keyvault unlocked state

f4a45c6

wallet: impl keyvault IsLocked

cfb0d00

GustavoStingelin added 7 commits June 28, 2026 16:31

wallet: impl keyvault Lock

55ee6ad

wallet: impl keyvault Unlock

c3cf029

wallet: pass context to keyvault refresh passphrase

4f9cea8

wallet: impl keyvault RefreshPrivatePassphrase

491e36e

wallet: impl keyvault Encrypt

43a8d35

wallet: impl keyvault Decrypt

8dd4c4e

wallet: add keyvault encryption vector tests

1312a0f

GustavoStingelin force-pushed the sqldb/impl-keyvault-lifecycle branch from c44b2f5 to 1312a0f Compare June 28, 2026 19:32

wallet: rename DBVault to WalletVault

cec670a

GustavoStingelin force-pushed the sqldb/impl-keyvault-lifecycle branch from 585bda5 to cec670a Compare June 28, 2026 19:54

yyforyongyu reviewed Jun 29, 2026

View reviewed changes

yyforyongyu requested changes Jun 29, 2026

View reviewed changes

Abdulkbk reviewed Jun 29, 2026

View reviewed changes

Uh oh!

Conversation

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

Uh oh!

Change Description

Steps to Test

Uh oh!

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

Uh oh!

Coverage Report for CI Build 28334091658

Coverage: 54.731%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

GustavoStingelin commented Jun 25, 2026

Uh oh!

yyforyongyu commented Jun 25, 2026

Uh oh!

GustavoStingelin commented Jun 25, 2026

Uh oh!

yyforyongyu commented Jun 26, 2026

Uh oh!

GustavoStingelin commented Jun 26, 2026

Uh oh!

GustavoStingelin commented Jun 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yyforyongyu commented Jun 29, 2026

Uh oh!

yyforyongyu 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!

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

GustavoStingelin commented Jun 18, 2026 •

edited

Loading

coveralls commented Jun 18, 2026 •

edited

Loading