Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions itest/lnd_wallet.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ import (
// walletTestCases defines a set of tests aiming at asserting functionalities
// provided by the wallerpc.
var walletTestCases = []*lntest.TestCase{
{
Name: "xcreate account",
TestFunc: testXCreateAccount,
},
{
Name: "xcreate account rejections",
TestFunc: testXCreateAccountRejections,
},
{
Name: "listunspent P2WPKH",
TestFunc: func(ht *lntest.HarnessTest) {
Expand Down
190 changes: 190 additions & 0 deletions itest/lnd_wallet_xcreate_account.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
package itest

import (
"github.com/btcsuite/btcd/btcutil/v2"
"github.com/btcsuite/btcd/wire/v2"
"github.com/lightningnetwork/lnd/lnrpc"
"github.com/lightningnetwork/lnd/lnrpc/walletrpc"
"github.com/lightningnetwork/lnd/lntest"
"github.com/lightningnetwork/lnd/lnwallet"
"github.com/stretchr/testify/require"
)

const (
// createAccountName is the account these tests create and spend from.
createAccountName = "custom"

// defaultCreateAccountFeeRate is the sat/vB rate the miner uses when
// funding the account under test.
defaultCreateAccountFeeRate = btcutil.Amount(10)

// maxCreateAccountSpendFee bounds what the account's own spend may
// cost. The transaction is one input and two outputs at 5 sat/vB, so
// a few thousand sats is a generous ceiling; the point is only to
// distinguish "paid a fee" from "the money went somewhere else".
maxCreateAccountSpendFee = btcutil.Amount(10_000)
)

// testXCreateAccount asserts the end-to-end behaviour of an account created

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the documented manual reconstruction is currently the only recovery mitigation, could we cover that procedure end to end as well? I am not suggesting that this PR must implement automatic seed-only discovery—the X/acknowledgement gates already make that limitation explicit—but the claim that the funds remain manually recoverable is important enough to verify before users rely on it.

The useful regression test would create a preceding account so the target has a non-trivial index, fund an external address, then spend through FundPsbt so value remains on an internal/change address. Record the target xpub plus both branch counts; restore the same seed into a fresh wallet DB; recreate accounts until the same scope/index is reached and assert the xpub matches; replay NextAddr(change=false) and NextAddr(change=true) using their respective counts; rescan with --reset-wallet-transactions; and finally assert that both balances are found and can be spent. Leaving real value on the internal branch is the part that would catch the recovery issue above.

// from the wallet's own master key: it is not watch-only, its funds are
// reported against it rather than the default account, and — the property
// that distinguishes it from an imported account — the wallet can sign for
// it.
func testXCreateAccount(ht *lntest.HarnessTest) {
alice := ht.NewNode("Alice", nil)

account := alice.RPC.XCreateAccount(&walletrpc.XCreateAccountRequest{
Name: createAccountName,
AddressType: walletrpc.AddressType_TAPROOT_PUBKEY,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we could cover different address types too.

}).GetAccount()

require.Equal(ht, createAccountName, account.GetName())
require.Equal(
ht, walletrpc.AddressType_TAPROOT_PUBKEY,
account.GetAddressType(),
)

// The whole point of this RPC: unlike an imported account, the wallet
// holds the keys, so it can spend what the account receives.
require.False(ht, account.GetWatchOnly(), "account must be spendable")

// It shows up in ListAccounts under the scope it was created in.
listed := alice.RPC.ListAccounts(&walletrpc.ListAccountsRequest{
Name: createAccountName,
AddressType: walletrpc.AddressType_TAPROOT_PUBKEY,
}).GetAccounts()
require.Len(ht, listed, 1)
require.Equal(ht, account.GetExtendedPublicKey(),
listed[0].GetExtendedPublicKey())

// Fund an address belonging to the new account. The address type has
// to match the one the account was created with: lnd resolves a custom
// account name inside the key scope the requested type implies.
addr := alice.RPC.NewAddress(&lnrpc.NewAddressRequest{
Type: lnrpc.AddressType_TAPROOT_PUBKEY,
Account: createAccountName,
}).GetAddress()

const fundAmt = btcutil.Amount(500_000)
ht.SendOutputsWithoutChange(
[]*wire.TxOut{{
Value: int64(fundAmt),
PkScript: ht.PayToAddrScript(ht.DecodeAddress(addr)),
}}, defaultCreateAccountFeeRate,
)
ht.MineBlocksAndAssertNumTxes(1, 1)

// The balance lands in the new account, and nowhere else. Both halves
// matter: the account must see its own coins, and the default account
// must not see them.
ht.AssertWalletAccountBalance(
alice, createAccountName, int64(fundAmt), 0,
)
ht.AssertWalletAccountBalance(
alice, lnwallet.DefaultAccountName, 0, 0,
)

// Now prove the wallet can actually spend it. An imported (watch-only)
// account gets this far too — funding a PSBT only needs public data —
// but finalizing is where it fails, because the wallet has no private
// key for it and silently signs nothing.
dest := alice.RPC.NewAddress(&lnrpc.NewAddressRequest{
Type: lnrpc.AddressType_TAPROOT_PUBKEY,
Account: createAccountName,
}).GetAddress()

funded := alice.RPC.FundPsbt(&walletrpc.FundPsbtRequest{
Template: &walletrpc.FundPsbtRequest_Raw{
Raw: &walletrpc.TxTemplate{
Outputs: map[string]uint64{
dest: uint64(fundAmt / 2),
},
},
},
Fees: &walletrpc.FundPsbtRequest_SatPerVbyte{
SatPerVbyte: 5,
},
Account: createAccountName,
})

finalized := alice.RPC.FinalizePsbt(&walletrpc.FinalizePsbtRequest{
FundedPsbt: funded.GetFundedPsbt(),
Account: createAccountName,
})
require.NotEmpty(ht, finalized.GetRawFinalTx(),
"wallet produced no signed transaction for its own account")

alice.RPC.PublishTransaction(&walletrpc.Transaction{
TxHex: finalized.GetRawFinalTx(),
})
ht.MineBlocksAndAssertNumTxes(1, 1)

// The spend confirmed, and both halves of where the money went matter.
// The account still holds its funds minus fees, which is what shows
// the inputs were spent from it and the change came back to it rather
// than leaking elsewhere; and the default account is still empty,
// which shows "elsewhere" was not it.
accounts := alice.RPC.WalletBalance().GetAccountBalance()
after := btcutil.Amount(
accounts[createAccountName].GetConfirmedBalance(),
)
require.Less(ht, after, fundAmt, "the spend should have paid a fee")
require.Greater(ht, after, fundAmt-maxCreateAccountSpendFee,
"the account should still hold its funds minus fees")

ht.AssertWalletAccountBalance(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "the account still holds its funds minus fees", but only the default account is asserted here — the custom account's post-spend balance never gets checked.

That's the strongest post-condition in the test: it's what proves the spend came out of the account and the change went back into it rather than leaking to the default account. Worth an AssertWalletAccountBalance on createAccountName too (or trimming the claim from the comment).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added rather than trimmed, since you're right that it's the strongest post-condition. The test now reads the account's own confirmed balance back from WalletBalance's per-account map and asserts it is below the funded amount (a fee was paid) but above funded-minus-a-fee-ceiling — which is what shows the inputs came from the account and the change returned to it, not to default.

alice, lnwallet.DefaultAccountName, 0, 0,
)
}

// testXCreateAccountRejections asserts the requests lnd refuses, each of which
// would otherwise leave the caller with an account that does not behave the
// way it asked for.
func testXCreateAccountRejections(ht *lntest.HarnessTest) {
alice := ht.NewNode("Alice", nil)

alice.RPC.XCreateAccount(&walletrpc.XCreateAccountRequest{
Name: createAccountName,
AddressType: walletrpc.AddressType_TAPROOT_PUBKEY,
})

// The same name a second time, even under a different address type.
// Coin selection resolves a custom account name to whichever key scope
// matches first, so a duplicate would make later funding ambiguous.
err := alice.RPC.XCreateAccountAssertErr(
&walletrpc.XCreateAccountRequest{
Name: createAccountName,
AddressType: walletrpc.AddressType_WITNESS_PUBKEY_HASH,
},
)
require.ErrorContains(ht, err, "already exists")

// The wallet's own reserved account names.
err = alice.RPC.XCreateAccountAssertErr(
&walletrpc.XCreateAccountRequest{
Name: lnwallet.DefaultAccountName,
AddressType: walletrpc.AddressType_TAPROOT_PUBKEY,
},
)
require.ErrorContains(ht, err, "reserved")

// An empty name.
err = alice.RPC.XCreateAccountAssertErr(
&walletrpc.XCreateAccountRequest{
AddressType: walletrpc.AddressType_TAPROOT_PUBKEY,
},
)
require.ErrorContains(ht, err, "account name is required")

// The strict nested-witness scheme, which a wallet-derived account
// cannot provide: it stores no address schema, so it would silently
// behave as the hybrid scheme instead.
err = alice.RPC.XCreateAccountAssertErr(
&walletrpc.XCreateAccountRequest{
Name: "nested",
AddressType: walletrpc.
AddressType_NESTED_WITNESS_PUBKEY_HASH,
},
)
require.ErrorContains(ht, err, "cannot be created")
}
Loading