walletrpc: return the master key birthday from ListAccounts - #10993
walletrpc: return the master key birthday from ListAccounts#10993Roasbeef wants to merge 4 commits into
Conversation
In this commit, we add a Birthday method to the WalletController interface, returning the birthday of the wallet's master key as btcwallet's address manager has it. That's the earliest time any of the wallet's keys could have been used, and therefore where a rescan of the chain has to start. The wallet has always known this, there just wasn't a way to ask it from the RPC layer. The next commit needs it so ListAccounts can hand the birthday to whoever imports those accounts into a watch-only wallet.
cf65ab0 to
4c89f2b
Compare
|
/gateway review |
![lightninglabs-gateway[bot]](https://avatars.githubusercontent.com/in/3635431?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gateway review — 4 findings
🔴 0 Blocker · 🟠 0 Major · 🟡 4 Minor · 🔵 0 Nit
Summary
This PR plumbs the wallet master-key birthday through walletrpc.ListAccounts (new master_key_birthday_timestamp field) so a watch-only import via lncli createwatchonly can start its rescan at the real creation height instead of lnd's 2017-08-24 default. The change is small, well-scoped, and well-tested: the new field round-trips through the existing accounts JSON that createwatchonly already consumes, Birthday() is added to WalletController with the two mocks and BtcWallet updated, and TestListAccountsBirthday covers both the set and zero-birthday cases. The zero-time handling (Unix() > 0 guard collapsing to the 0 = "unknown" sentinel) is correct and deliberately matches what InitWallet already expects.
No blocking issues surfaced. The remaining points are minor: a source-level interface addition to confirm against all implementers, a behavior change where the birthday prompt now auto-accepts a file-sourced value, an invariant worth naming on the Unix() > 0 guard, and a small privacy note on the newly-disclosed field.
Bot commands
/gateway re-review— re-run after pushing changes/gateway dismiss <id>— silence a finding (maintainers)/gateway explain <id>— elaborate on a finding (anyone)
| // recovery progress made so far. | ||
| GetRecoveryInfo() (bool, float64, error) | ||
|
|
||
| // Birthday returns the birthday of the wallet's master key, meaning the |
There was a problem hiding this comment.
Verified rather than changed. WalletController has exactly four implementers in tree, all satisfied: BtcWallet (this PR), the two mocks in lnwallet/mock.go and lntest/mock/walletcontroller.go (this PR), and rpcwallet.RPCKeyRing, which embeds *btcwallet.BtcWallet and so inherits it.
$ grep -rn "WalletController = " --include="*.go" .
lntest/mock/walletcontroller.go:42:var _ lnwallet.WalletController = (*WalletController)(nil)
lnwallet/mock.go:48:var _ WalletController = (*mockWalletController)(nil)
lnwallet/rpcwallet/rpcwallet.go:73:var _ lnwallet.WalletController = (*RPCKeyRing)(nil)
lnwallet/btcwallet/{btcwallet,blockchain}.go: var _ lnwallet.WalletController = (*BtcWallet)(nil)
go build ./... and go vet ./... are clean under the full RPC tag set plus dev integration, which covers the itest doubles too.
Skipping the release-note suggestion: these release notes are written for node operators, and every implementer of this interface is in this repo, so a Go-interface note would be noise for the audience that reads them.
| // master key its accounts were derived from, which is more accurate than | ||
| // anything the operator is likely to type in, so we offer it as the | ||
| // default. An older export leaves this at zero and the prompt behaves as | ||
| // it always did. |
There was a problem hiding this comment.
🟡 F2 (Minor) — createwatchonly now auto-accepts a file-sourced birthday for the rescan start
createWatchOnly now feeds jsonAccts.MasterKeyBirthdayTimestamp from the operator-supplied accounts file into askBirthdayTimestamp as the default, and a bare Enter accepts it; that value flows into WatchOnly.MasterKeyBirthdayTimestamp and sets the watch-only rescan start height. This is a change in posture — previously the empty answer resolved to 0 (lnd's default 2017-08-24), now it resolves to whatever the file carries. In the normal flow this is strictly better (accurate and faster), and the fund-loss exposure is limited: a genuine master-key birthday cannot predate the keys it derives, so a correct value can never skip a block holding this wallet's UTXOs; the accounts file is already the fully-trusted root defining the wallet (it holds the xpubs), so tampering with the birthday is strictly weaker than tampering already possible; and a wrong (stale/buggy) value only costs a recoverable manual rescan. Still, because the prompt now pre-fills a fund-recovery-relevant parameter and treats Enter as consent, consider printing the resolved calendar date next to the default so the operator can sanity-check it before accepting.
There was a problem hiding this comment.
Took the suggestion. askBirthdayTimestamp now spells a non-zero default out as a date next to the raw value, so the thing an empty answer accepts is legible:
Input an optional wallet birthday unix timestamp of first block to start scanning from (default 1767225600, which is 2026-01-01T00:00:00Z):
The zero default (the create path, importing an extended master key, where no birthday exists to offer) still prints a bare 0 rather than 1970.
Agreed on the rest of the analysis: a real master-key birthday cannot predate the keys derived from it, so an accurate value can never skip a block holding this wallet's outputs, and the accounts file is already the trusted root that defines the wallet.
| // A wallet that somehow has no birthday at all reports zero rather than | ||
| // the nonsense a uint64 cast of a pre-epoch time would produce, since | ||
| // zero is what the InitWallet request already reads as "unknown". | ||
| var birthdayTimestamp uint64 |
There was a problem hiding this comment.
🟡 F3 (Minor) — The Unix() > 0 guard is load-bearing; do not refactor to IsZero()
The if birthday := w.cfg.Wallet.Birthday(); birthday.Unix() > 0 guard does double duty: it filters the zero-value sentinel (time.Time{}.Unix() is a large negative) and it guarantees positivity before uint64(birthday.Unix()). A well-meaning refactor to !birthday.IsZero() would let a non-zero pre-1970 birthday through as a negative int64, which the cast turns into a ~1.8e19 nonsense timestamp — the exact failure the code is avoiding. The current form is correct (aezeed genesis 2017-08-24 is well positive), so this is a note to keep the invariant explicit, not a defect. One small inaccuracy: the comment says the guard only catches "a wallet that somehow has no birthday at all," but an exact-epoch birthday (Unix() == 0) is also mapped to 0 — practically unreachable, but the comment could say so.
There was a problem hiding this comment.
Good catch on the refactor hazard, took that half. The comment now names why the comparison has to stay a positivity check:
// The comparison has to stay a positivity check and not become an
// IsZero one: a non-zero time before 1970 has a negative Unix value,
// which is exactly what the cast below would turn into a timestamp
// somewhere past the year 292 billion.Leaving the exact-epoch nuance out. A wallet whose birthday is precisely 1970-01-01T00:00:00Z maps to 0 and is therefore read as "unknown", which is the correct outcome anyway, and spelling out a case that cannot arise on a Bitcoin wallet costs more attention than it returns.
|
|
||
| return &ListAccountsResponse{ | ||
| Accounts: rpcAccounts, | ||
| MasterKeyBirthdayTimestamp: birthdayTimestamp, |
There was a problem hiding this comment.
Dismissing this one. The birthday adds nothing an onchain:read holder cannot already derive, and often less precisely: the same scope covers ListUnspent and ListAddresses (walletkit_server.go:142,146), and lnrpc.GetTransactions sits behind onchain:read too. Anyone holding that credential can already enumerate the wallet's UTXOs, its addresses and its full on-chain transaction history, which pins the wallet's activity window to actual block heights. A creation timestamp is a strictly weaker signal than the set of blocks its outputs are actually in.
Gating the field is also self-defeating: the whole point is that the accounts export, which is fetched with a macaroon, carries the birthday to the importing side. Gating it behind a stronger scope would break the flow this PR exists to fix.
That leaves documenting "onchain:read now exposes the wallet birthday" in the field comment, and I would rather not: it cannot be turned off, so it does not inform any decision an operator can act on, and it invites the reading that the birthday was ever confidential from a credential that already reveals the chain history it summarizes.
|
🤖 gateway audit metadata for this PR — auto-generated, please don't edit. |
In this commit, we add a master_key_birthday_timestamp field to ListAccountsResponse and populate it from the wallet. The accounts in that response are extended public keys, and an xpub says nothing about when it was created. That's a problem for the one thing the response exists for: exporting a signer's accounts so a watch-only wallet can import them. The importing side has no birthday to work with, so lnd substitutes the aezeed "Bitcoin Days Genesis" epoch of 2017-08-24 and rescans from there. On mainnet that's hundreds of thousands of blocks and several hours, even for a node with no history whatsoever. See lightninglabs/lndinit#97 for what that looks like in practice. The field is named to match WatchOnly.master_key_birthday_timestamp on the InitWallet request, which is where the value ends up, and it round-trips through the accounts JSON file that lncli's createwatchonly and lndinit both read. A wallet that somehow has no birthday at all reports zero rather than the nonsense a uint64 cast of a pre-epoch time would produce, since zero is already what InitWallet reads as "unknown".
In this commit, we have createwatchonly offer the birthday it found in the accounts file as the default for its birthday prompt, so an accounts file exported by a recent enough lnd carries the right value through on its own. The prompt stays, since the operator may know better than the file does, and an older accounts file without the field leaves the default at zero, which is exactly how the prompt behaved before.
In this commit, we note the new ListAccounts field and the createwatchonly default under RPC and lncli updates.
4c89f2b to
a9b0948
Compare
| // recovery progress made so far. | ||
| GetRecoveryInfo() (bool, float64, error) | ||
|
|
||
| // Birthday returns the birthday of the wallet's master key, meaning the |
There was a problem hiding this comment.
Verified rather than changed. WalletController has exactly four implementers in tree, all satisfied: BtcWallet (this PR), the two mocks in lnwallet/mock.go and lntest/mock/walletcontroller.go (this PR), and rpcwallet.RPCKeyRing, which embeds *btcwallet.BtcWallet and so inherits it.
$ grep -rn "WalletController = " --include="*.go" .
lntest/mock/walletcontroller.go:42:var _ lnwallet.WalletController = (*WalletController)(nil)
lnwallet/mock.go:48:var _ WalletController = (*mockWalletController)(nil)
lnwallet/rpcwallet/rpcwallet.go:73:var _ lnwallet.WalletController = (*RPCKeyRing)(nil)
lnwallet/btcwallet/{btcwallet,blockchain}.go: var _ lnwallet.WalletController = (*BtcWallet)(nil)
go build ./... and go vet ./... are clean under the full RPC tag set plus dev integration, which covers the itest doubles too.
Skipping the release-note suggestion: these release notes are written for node operators, and every implementer of this interface is in this repo, so a Go-interface note would be noise for the audience that reads them.
|
|
||
| return &ListAccountsResponse{ | ||
| Accounts: rpcAccounts, | ||
| MasterKeyBirthdayTimestamp: birthdayTimestamp, |
There was a problem hiding this comment.
Dismissing this one. The birthday adds nothing an onchain:read holder cannot already derive, and often less precisely: the same scope covers ListUnspent and ListAddresses (walletkit_server.go:142,146), and lnrpc.GetTransactions sits behind onchain:read too. Anyone holding that credential can already enumerate the wallet's UTXOs, its addresses and its full on-chain transaction history, which pins the wallet's activity window to actual block heights. A creation timestamp is a strictly weaker signal than the set of blocks its outputs are actually in.
Gating the field is also self-defeating: the whole point is that the accounts export, which is fetched with a macaroon, carries the birthday to the importing side. Gating it behind a stronger scope would break the flow this PR exists to fix.
That leaves documenting "onchain:read now exposes the wallet birthday" in the field comment, and I would rather not: it cannot be turned off, so it does not inform any decision an operator can act on, and it invites the reading that the birthday was ever confidential from a credential that already reveals the chain history it summarizes.
|
🚫 Dismissed F1 (minor) by @Roasbeef — no reason given Open findings on this PR: 🟡 F2 (minor) · 🟡 F3 (minor) · 🟡 F4 (minor) |
|
🚫 Dismissed F4 (minor) by @Roasbeef — no reason given Open findings on this PR: 🟡 F2 (minor) · 🟡 F3 (minor) |
1 participant
In this PR, we teach
walletrpc.ListAccountsto also return the birthday of thewallet's master key, so that exporting a signer's accounts and importing them into
a watch-only wallet no longer loses the one piece of information that decides where
the rescan starts.
The accounts in that response are extended public keys, and an xpub says nothing
about when it was created. That's awkward for the main thing the response is used
for,
lncli wallet accounts liston a signer feedinglncli createwatchonly(orlndinit init-wallet --init-rpc.watch-only) on the gateway. The importing side hasno birthday to work with, so we substitute the aezeed "Bitcoin Days Genesis" epoch
of
2017-08-24and rescan from there. On mainnet today that's ~478k blocks andseveral hours parked at
Wallet is not synced to height <tip> yet, for a node thatmay have been created this morning and has nothing at all to find:
That's from lightninglabs/lndinit#97, filed against a fresh remote-signing gateway
on mainnet.
lndinithas its own half of the fix inlightninglabs/lndinit#98, adding a flag so the operator can supply the birthday by
hand. This PR is the half that makes the flag unnecessary in the common case: the
value already exists on the signer, it just never made it into the export.
The pieces
lnwallet.WalletControllergrows aBirthday()method, backed by btcwallet'saddress manager, which has held this all along. There was simply no way to ask for
it from the RPC layer.
ListAccountsResponsegrowsmaster_key_birthday_timestamp, named to matchWatchOnly.master_key_birthday_timestampon theInitWalletrequest since that'swhere the value ends up. Because the accounts JSON file that
createwatchonlyconsumes is a marshaled
ListAccountsResponse, the birthday round-trips throughthe file with no format changes on either end. A wallet that somehow has no
birthday reports zero rather than the nonsense a
uint64cast of a pre-epoch timewould give, zero being what
InitWalletalready reads as "unknown".createwatchonlythen offers that value as the default for its birthday prompt.The prompt stays, since an operator may know better than the file does, and an
older accounts file without the field leaves the default at zero, which is exactly
how the prompt behaved before.
Testing
TestListAccountsBirthdaycovers both the normal case and the no-birthday wallet.Beyond that, I ran the whole thing on regtest against a real signer/watch-only
split: a signer holding an aezeed generated today, its accounts exported with
wallet accounts list, and a gateway with--remotesigner.enableimporting them.A stock regtest chain stamps every block with roughly the same time, so every
birthday resolves to the same block and there's nothing to see. Mining 200 blocks
under
setmocktimewith timestamps spread from 2017-01 to 2026-06 (~17 days perblock) puts the aezeed epoch at height 13 and a recent birthday at height 190,
which makes the difference visible: the rescan goes from 187 blocks to 10, and the
resulting wallet is fully functional afterwards (identity pubkey matching the
signer,
newaddressgoing out to the remote signer, on-chain funds detected).See each commit message for the detailed reasoning w.r.t the individual pieces.