Fixing #28897.
As the user may have imported a descriptor with a timestamp newer than the actual birth time of the first key (by setting ’timestamp=now’), the wallet needs to update the birth time when it detects a transaction older than the oldest descriptor timestamp.
Testing Notes: Can cherry-pick the test commit on top of master. It will fail there.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, achow101 |
| Approach ACK | S3RK |
| Stale ACK | BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Concept ACK
You could also add a oldest_timestamp (or birth) field to getwalletinfo which would contain the birth date.
685@@ -686,7 +686,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
686 bool ImportScriptPubKeys(const std::string& label, const std::set<CScript>& script_pub_keys, const bool have_solving_data, const bool apply_label, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
687
688 /** Updates wallet birth time if 'new_birth_time' is below it */
689- void FirstKeyTimeChanged(const ScriptPubKeyMan* spkm, int64_t new_birth_time);
690+ void TryUpdateBirthTime(int64_t new_birth_time);
MaybeUpdateBirthTime( )
1746@@ -1747,7 +1747,7 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
1747 return true;
1748 }
1749
1750-void CWallet::FirstKeyTimeChanged(const ScriptPubKeyMan* spkm, int64_t new_birth_time)
1751+void CWallet::TryUpdateBirthTime(int64_t new_birth_time)
3482@@ -3483,7 +3483,7 @@ void CWallet::AddScriptPubKeyMan(const uint256& id, std::unique_ptr<ScriptPubKey
3483 const auto& spkm = m_spk_managers[id] = std::move(spkm_man);
4bfb243ac41cf1396cf55f8b24de2ba52186c7a4: this seems simpler:
0TryUpdateBirthTime(spkm_man->GetTimeFirstKey());
1m_spk_managers[id] = std::move(spkm_man);
Ah, that makes sense, can you add a comment? Something like:
0// Add spkm_man to m_spk_managers before calling any method
1// that might access it.
2const auto& spkm = m_spk_managers[id] = std::move(spkm_man);
3515@@ -3516,7 +3516,7 @@ void CWallet::ConnectScriptPubKeyManNotifiers()
3516 for (const auto& spk_man : GetActiveScriptPubKeyMans()) {
3517 spk_man->NotifyWatchonlyChanged.connect(NotifyWatchonlyChanged);
3518 spk_man->NotifyCanGetAddressesChanged.connect(NotifyCanGetAddressesChanged);
3519- spk_man->NotifyFirstKeyTimeChanged.connect(std::bind(&CWallet::FirstKeyTimeChanged, this, std::placeholders::_1, std::placeholders::_2));
3520+ spk_man->NotifyFirstKeyTimeChanged.connect(std::bind(&CWallet::TryUpdateBirthTime, this, std::placeholders::_2));
4bfb243ac41cf1396cf55f8b24de2ba52186c7a4: you can use std::placeholders::_1 here, by dropping this from NotifyFirstKeyTimeChanged calls and changing:
0--- a/src/wallet/scriptpubkeyman.h
1+++ b/src/wallet/scriptpubkeyman.h
2@@ -255,11 +255,11 @@ public:
3
4 /** Keypool has new keys */
5 boost::signals2::signal<void ()> NotifyCanGetAddressesChanged;
6
7 /** Birth time changed */
8- boost::signals2::signal<void (const ScriptPubKeyMan* spkm, int64_t new_birth_time)> NotifyFirstKeyTimeChanged;
9+ boost::signals2::signal<void (int64_t new_birth_time)> NotifyFirstKeyTimeChanged;
10 };
Passing fewer (unused) complex objects around is always better…
I didn’t drop the spkm arg on purpose. It was a design decision. The idea is the following one: The wallet stores multiple spkms, and it registers to multiple signals from each of them. If we don’t provide the pointer to the originator spkm, we will not be able to distinguish them. And, I’m pretty confident we will need this data, or need to access the origin spkm during an event, sooner or later.
But if you are strong on this, I could change it. We could always re-add the spkm pointer when/if we need it.
If you have good reasons to believe we need these events later, then keeping them is fine by me. But why do you think this?
Presumable related to #28920 (review)
Updated per feedback, thanks @Sjors.
You could also add a oldest_timestamp (or birth) field to getwalletinfo which would contain the birth date.
Sounds good 👌🏼. Added it in another commit to not disable the possibility of verifying the failure by cherry-picking the test commit on top of master.
In the following-up commit, the wallet birth time will also
be modified by the transactions scanning process. When a tx
older than all descriptor's timestamp is detected.
As the user could have imported a descriptor with
a newer timestamp (by blindly setting 'timestamp=now'),
the wallet needs to update the birth time when it detects
a transaction older than the oldest descriptor timestamp.
35+ def birthtime_test(self, node, miner_wallet):
36+ self.log.info("Test birth time update during tx scanning")
37+ # Fund address to test
38+ wallet_addr = miner_wallet.getnewaddress()
39+ tx_id = miner_wallet.sendtoaddress(wallet_addr, 2)
40+ miner_wallet.syncwithvalidationinterfacequeue()
q: Is this needed? It should be called internally for most stuff that needs it.
Good eye. No, it is not needed.
I was probably worried about the tx not reaching the mempool prior to generating block. But.. sendtoaddress (same as every other send mechanism) always reaches the mempool. Returning a mempool error if it fails to get added there.
And.. this is independent to the validation interface queue. So, I’m not sure why my past-self added it.
42+ for _ in range(50):
43+ self.generate(node, 1)
44+ self.advance_time(node, BLOCK_TIME)
45+
46+ # Now create a new wallet, and import the descriptor
47+ node.createwallet(wallet_name='watch_only', disable_private_keys=True, blank=True, load_on_startup=True)
0 node.createwallet(wallet_name='watch_only', disable_private_keys=True, load_on_startup=True)
nit: blank is not necessary.
ACK 8cd2fedb3ab283f9dbabd8cf1827e63f4f0db8d7
left a nit comment
205@@ -206,6 +206,7 @@
206 'wallet_createwallet.py --descriptors',
207 'wallet_watchonly.py --legacy-wallet',
208 'wallet_watchonly.py --usecli --legacy-wallet',
209+ 'wallet_reindex.py',
In bc8fb2a9d5fe941bcc77eacb23d72c663828f8e3 “test: coverage for wallet birth time interaction with -reindex”
Needs --legacy-wallet and --descriptor.
Updated per feedback, thanks for the reviews.
While updating the test suite to incorporate the --legacy-wallet test counterpart, discovered an issue with the first key time in blank legacy wallets. It was incorrectly set to 0 (aka scan from the beginning of time) instead of being set to the maximum timestamp value. Therefore, in master, blank legacy wallets are currently scanning the entire chain instead of utilizing the birth time functionality. Added 0a1107d to fix the issue.
132@@ -132,6 +133,9 @@ static RPCHelpMan getwalletinfo()
133 obj.pushKV("descriptors", pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS));
134 obj.pushKV("external_signer", pwallet->IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER));
135 obj.pushKV("blank", pwallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET));
136+ if (int64_t birthtime = pwallet->GetBirthTime(); birthtime != std::numeric_limits<int64_t>::max()) {
NO_KEY_TIME explicitly?
yeah, it could be NO_KEY_TIME or could also return an optional.
The issue that I see with the NO_KEY_TIME constant is where to place it. Because, if we place it at the wallet.h level, we would add a circular dependency wallet -> spkm -> wallet.
NO_KEY_TIME. Since wallet already includes spkm, leaving it there will not be circular, and it can still be accessed by things that include wallet. It just needs to be pulled out of LegacyScriptPubKeyMan.
285@@ -286,7 +286,9 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
286 WatchOnlySet setWatchOnly GUARDED_BY(cs_KeyStore);
287 WatchKeyMap mapWatchKeys GUARDED_BY(cs_KeyStore);
288
289- int64_t nTimeFirstKey GUARDED_BY(cs_KeyStore) = 0;
290+ // By default, do not scan any block until keys/scripts are generated/imported
291+ int64_t NO_KEY_TIME = std::numeric_limits<int64_t>::max();
NO_KEY_TIME ought to be 0 so it triggers a full scan
Seems like
NO_KEY_TIMEought to be0so it triggers a full scan
That was the bug I solved 0a1107d0.
By default, the legacy spkm is empty. It has no keys nor scripts, thus it should not trigger a full scan.
Once the first key is generated or the first script is imported, the legacy spkm will update the nTimeFirstKey time automatically and start scanning blocks.
To avoid scanning blocks, as assumed by a wallet with no
generated keys or imported scripts, the default value for
the birth time needs to be set to the maximum int64_t value.
Once the first key is generated or the first script is imported,
the legacy SPKM will update the birth time automatically.
Verifying the wallet updates the birth time accordingly when it
detects a transaction with a time older than the oldest descriptor
timestamp.
This could happen when the user blindly imports a descriptor with
'timestamp=now'.
And add coverage for it
NO_KEY_TIME to UNKNOWN_TIME and made it accessible for other modules.
re-utACK 1ce45baed7dd2da3f1cb85c9c25110e5537451ae
Should we backport 6f497377aa17cb8a590fd7717fa8ededf4249999 or the whole PR, and if so how far?
