wallet: birth time update during tx scanning #28920

furszy · 2023-11-20T16:05:49Z

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.

DrahtBot · 2023-11-20T16:05:53Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
#22693 (RPC/Wallet: Add "use_txids" to output of getaddressinfo by luke-jr)

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.

Sjors · 2023-11-21T08:49:23Z

Concept ACK

You could also add a oldest_timestamp (or birth) field to getwalletinfo which would contain the birth date.

Sjors

I checked that the test fails if you drop everything but 134960c.

src/wallet/wallet.h

Sjors · 2023-11-21T08:53:32Z

src/wallet/wallet.cpp

@@ -1747,7 +1747,7 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
    return true;
 }

-void CWallet::FirstKeyTimeChanged(const ScriptPubKeyMan* spkm, int64_t new_birth_time)
+void CWallet::TryUpdateBirthTime(int64_t new_birth_time)


4bfb243: I'm a bit puzzled why this function ever accepted an argument that it didn't use.

I think I had a birth time per descriptor before. This work was taken from my bip157/bip158 light client branch. Where I only use the filters that have a birth time below the header time to decrease the false-positive rate.
I was thinking on porting it to the wallet rescan process when I did it. But.. I decided to leave it for when the p2p related PRs get merged. And.. #28170, #28120 and #27837 are still open.

src/wallet/wallet.cpp

furszy · 2023-11-22T00:58:23Z

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.

furszy · 2023-11-23T13:07:51Z

Updated per feedback. Tiny diff.
Only added a small explanatory comment.

test/functional/wallet_reindex.py

BrandonOdiwuor

Concept ACK

Sjors · 2023-11-28T13:49:11Z

ACK 8cd2fed

BrandonOdiwuor

ACK 8cd2fed

left a nit comment

test/functional/wallet_reindex.py

test/functional/test_runner.py

furszy · 2023-11-28T21:19:25Z

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.

achow101 · 2023-12-14T20:48:48Z

Should we backport 6f49737 or the whole PR, and if so how far?

I think it would be good to backport the entire thing to 26.1.

achow101 · 2023-12-14T21:13:24Z

ACK 1ce45ba

fanquake · 2023-12-15T10:38:12Z

Adding to #29011 for backporting to 26.x.

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. Github-Pull: bitcoin#28920 Rebased-From: b4306e3

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. Github-Pull: bitcoin#28920 Rebased-From: 75fbf44

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. Github-Pull: bitcoin#28920 Rebased-From: 6f49737

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'. Github-Pull: bitcoin#28920 Rebased-From: 83c6644

And add coverage for it Github-Pul: bitcoin#28920 Rebased-From: 1ce45ba

And add coverage for it Github-Pull: bitcoin#28920 Rebased-From: 1ce45ba

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. Github-Pull: bitcoin#28920 Rebased-From: b4306e3

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. Github-Pull: bitcoin#28920 Rebased-From: 75fbf44

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. Github-Pull: bitcoin#28920 Rebased-From: 6f49737

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'. Github-Pull: bitcoin#28920 Rebased-From: 83c6644

And add coverage for it Github-Pull: bitcoin#28920 Rebased-From: 1ce45ba

7b79e54 doc: update release notes for 26.x (fanquake) ccf00b1 wallet: Fix use-after-free in WalletBatch::EraseRecords (MarcoFalke) 40252e1 ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid failures (Hennadii Stepanov) b06b14e rpc: getwalletinfo, return wallet 'birthtime' (furszy) 1283401 test: coverage for wallet birth time interaction with -reindex (furszy) 0fa47e2 wallet: fix legacy spkm default birth time (furszy) 84f4a6c wallet: birth time update during tx scanning (furszy) 074296d refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime (furszy) 35039ac fuzz: disable BnB when SFFO is enabled (furszy) 903b462 test: add coverage for BnB-SFFO restriction (furszy) 05d0576 wallet: create tx, log resulting coin selection info (furszy) 5493ebb wallet: skip BnB when SFFO is active (Murch) b15e2e2 test: add regression test for the getrawtransaction segfault (Martin Zumsande) 5097bb3 rpc: fix getrawtransaction segfault (Martin Zumsande) 81e744a ci: Use Ubuntu 24.04 Noble for asan (MarcoFalke) 69e53d1 ci: Use Ubuntu 24.04 Noble for tsan,tidy,fuzz (MarcoFalke) d2c80b6 doc: Missing additions to 26.0 release notes (fanquake) 8dc2c75 doc: add historical release notes for 26.0 (fanquake) Pull request description: Backports for `26.x`. Currently: * #28920 * #28992 * #28994 * #29003 * #29023 * #29080 * #29176 ACKs for top commit: TheCharlatan: ACK 7b79e54 glozow: ACK 7b79e54, matches mine Tree-SHA512: 898aec76ed3ad35e0edd0980af5bcc21bd60003bbf69e0b4f473ed2aa38c4e3b360b930bc3747cf798195906a8f9fe66417524f5e5ef40fa68f1c1aaceebdeb0

DrahtBot added Wallet CI failed labels Nov 20, 2023

furszy force-pushed the 2023_wallet_birhtime_update branch from f57aa27 to 621dee2 Compare November 20, 2023 19:35

DrahtBot removed the CI failed label Nov 20, 2023

DrahtBot mentioned this pull request Nov 20, 2023

wallet: Track no-longer-spendable TXOs separately #27865

Draft

furszy force-pushed the 2023_wallet_birhtime_update branch from 621dee2 to 134960c Compare November 20, 2023 21:10

This was referenced Nov 20, 2023

wallet: Keep track of the wallet's own transaction outputs in memory #27286

Open

Remove the legacy wallet and BDB dependency #28710

Draft

Sjors reviewed Nov 21, 2023

View reviewed changes

furszy force-pushed the 2023_wallet_birhtime_update branch from 134960c to 44e2c74 Compare November 22, 2023 00:49

furszy added 2 commits November 23, 2023 09:55

refactor: rename FirstKeyTimeChanged to MaybeUpdateBirthTime

b4306e3

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.

wallet: birth time update during tx scanning

75fbf44

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.

furszy force-pushed the 2023_wallet_birhtime_update branch from 44e2c74 to 25de6ff Compare November 23, 2023 12:56

furszy mentioned this pull request Nov 24, 2023

wallet RPC to double-check the calculated balance #28898

Open

maflcko reviewed Nov 24, 2023

View reviewed changes

test/functional/wallet_reindex.py Outdated Show resolved Hide resolved

furszy force-pushed the 2023_wallet_birhtime_update branch from 25de6ff to 8cd2fed Compare November 24, 2023 16:23

DrahtBot mentioned this pull request Nov 25, 2023

RPC/Wallet: Add "use_txids" to output of getaddressinfo #22693

Open

BrandonOdiwuor reviewed Nov 27, 2023

View reviewed changes

DrahtBot requested a review from BrandonOdiwuor November 28, 2023 13:49

BrandonOdiwuor approved these changes Nov 28, 2023

View reviewed changes

test/functional/wallet_reindex.py Outdated Show resolved Hide resolved

achow101 reviewed Nov 28, 2023

View reviewed changes

test/functional/test_runner.py Outdated Show resolved Hide resolved

furszy force-pushed the 2023_wallet_birhtime_update branch from 8cd2fed to a8b15f3 Compare November 28, 2023 20:52

furszy force-pushed the 2023_wallet_birhtime_update branch 2 times, most recently from 2619e24 to a41393d Compare November 28, 2023 22:28

DrahtBot requested review from BrandonOdiwuor and S3RK and removed request for BrandonOdiwuor December 14, 2023 13:12

achow101 added the Needs backport (26.x) label Dec 14, 2023

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor December 14, 2023 20:48

DrahtBot requested review from BrandonOdiwuor and removed request for BrandonOdiwuor December 14, 2023 21:13

achow101 merged commit 08e6aaa into bitcoin:master Dec 14, 2023
16 checks passed

furszy deleted the 2023_wallet_birhtime_update branch December 14, 2023 21:47

fanquake mentioned this pull request Dec 15, 2023

[26.x] Backports #29011

Merged

fanquake removed the Needs backport (26.x) label Dec 15, 2023

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Dec 15, 2023

rpc: getwalletinfo, return wallet 'birthtime'

7e90b59

And add coverage for it Github-Pul: bitcoin#28920 Rebased-From: 1ce45ba

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Dec 18, 2023

rpc: getwalletinfo, return wallet 'birthtime'

3704e3a

And add coverage for it Github-Pull: bitcoin#28920 Rebased-From: 1ce45ba

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 4, 2024

rpc: getwalletinfo, return wallet 'birthtime'

b06b14e

And add coverage for it Github-Pull: bitcoin#28920 Rebased-From: 1ce45ba

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: birth time update during tx scanning #28920

wallet: birth time update during tx scanning #28920

furszy commented Nov 20, 2023 •

edited

Loading

DrahtBot commented Nov 20, 2023 •

edited

Loading

Sjors commented Nov 21, 2023

Sjors left a comment

Sjors Nov 21, 2023

furszy Nov 21, 2023 •

edited

Loading

furszy commented Nov 22, 2023

furszy commented Nov 23, 2023

BrandonOdiwuor left a comment

Sjors commented Nov 28, 2023

BrandonOdiwuor left a comment

furszy commented Nov 28, 2023

achow101 commented Dec 14, 2023 •

edited

Loading

achow101 commented Dec 14, 2023

fanquake commented Dec 15, 2023

wallet: birth time update during tx scanning #28920

wallet: birth time update during tx scanning #28920

Conversation

furszy commented Nov 20, 2023 • edited Loading

DrahtBot commented Nov 20, 2023 • edited Loading

Code Coverage

Reviews

Conflicts

Sjors commented Nov 21, 2023

Sjors left a comment

Choose a reason for hiding this comment

Sjors Nov 21, 2023

Choose a reason for hiding this comment

furszy Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

furszy commented Nov 22, 2023

furszy commented Nov 23, 2023

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Sjors commented Nov 28, 2023

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

furszy commented Nov 28, 2023

achow101 commented Dec 14, 2023 • edited Loading

achow101 commented Dec 14, 2023

fanquake commented Dec 15, 2023

furszy commented Nov 20, 2023 •

edited

Loading

DrahtBot commented Nov 20, 2023 •

edited

Loading

furszy Nov 21, 2023 •

edited

Loading

achow101 commented Dec 14, 2023 •

edited

Loading