-
Notifications
You must be signed in to change notification settings - Fork 2.3k
walletrpc: return the master key birthday from ListAccounts #10993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
3cba026
3d3648b
f366509
a9b0948
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2689,7 +2689,28 @@ func (w *WalletKit) ListAccounts(ctx context.Context, | |
| rpcAccounts = append(rpcAccounts, rpcAccount) | ||
| } | ||
|
|
||
| return &ListAccountsResponse{Accounts: rpcAccounts}, nil | ||
| // The accounts above are extended public keys, which say nothing about | ||
| // when they were created. We return the wallet's birthday along with | ||
| // them so that a watch-only wallet importing this response doesn't have | ||
| // to rescan the chain from lnd's default birthday. | ||
| // | ||
| // 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". | ||
| // | ||
| // 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. | ||
| var birthdayTimestamp uint64 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| if birthday := w.cfg.Wallet.Birthday(); birthday.Unix() > 0 { | ||
| birthdayTimestamp = uint64(birthday.Unix()) | ||
| } | ||
|
|
||
| return &ListAccountsResponse{ | ||
| Accounts: rpcAccounts, | ||
| MasterKeyBirthdayTimestamp: birthdayTimestamp, | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dismissing this one. The birthday adds nothing an 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /gateway dismiss |
||
| }, nil | ||
| } | ||
|
|
||
| // RequiredReserve returns the minimum amount of satoshis that should be | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -550,6 +550,12 @@ type WalletController interface { | |
| // recovery progress made so far. | ||
| GetRecoveryInfo() (bool, float64, error) | ||
|
|
||
| // Birthday returns the birthday of the wallet's master key, meaning the | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified rather than changed.
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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /gateway dismiss |
||
| // earliest time at which any of its keys could have been used. Blocks | ||
| // before that point cannot contain any of the wallet's outputs, so this | ||
| // is where a rescan of the chain starts. | ||
| Birthday() time.Time | ||
|
|
||
| // Start initializes the wallet, making any necessary connections, | ||
| // starting up required goroutines etc. | ||
| Start() error | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡
F2(Minor) — createwatchonly now auto-accepts a file-sourced birthday for the rescan startcreateWatchOnlynow feedsjsonAccts.MasterKeyBirthdayTimestampfrom the operator-supplied accounts file intoaskBirthdayTimestampas the default, and a bare Enter accepts it; that value flows intoWatchOnly.MasterKeyBirthdayTimestampand sets the watch-only rescan start height. This is a change in posture — previously the empty answer resolved to0(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took the suggestion.
askBirthdayTimestampnow spells a non-zero default out as a date next to the raw value, so the thing an empty answer accepts is legible:The zero default (the
createpath, importing an extended master key, where no birthday exists to offer) still prints a bare0rather 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.