wallet, bugfix: fix ComputeTimeSmart function during rescanning process.

BitcoinTsunami commented at 5:31 pm on December 7, 2020: none

The function ComputeTimeSmart in wallet.cpp assume that transaction are discovered in the right order. Moreover the ‘smarttime’ determination algorithm is coded with realtime scenario in mind and not rescanning of old block.

The functional test demonstrate that if the user import a wallet, then rescan only recent history, and then rescan the entire history, the older transaction discovered would have an incorrect time determination. In the context of rescanning old block, the only time value that as a meaning is the blocktime.

That’s why I’ve fixed the problem with a simple separation between rescanning of old block and realtime time determination. The fix is written to have no impact on every realtime scenario and only impact the behaviour during a rescanning process. This PR Fixes #20181.

To be fair, I don’t think that this bug could be triggered with the wallet GUI, because it always proceed with a proper rescan. But RPC API provide the possibility to trigger it. I’ve discovered it, because Specter desktop v0.10.0 was impacted. (https://github.com/cryptoadvance/specter-desktop/issues/680).

DrahtBot commented at 6:34 pm on December 7, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)

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.

DrahtBot added the label Wallet on Dec 7, 2020

jonatack commented at 9:16 pm on December 7, 2020: member

If this PR is intended to address issue #20181, can you add “Closes #20181” or “Fixes #20181” to the PR description instead of the PR title?

BitcoinTsunami renamed this:
~~wallet, bugfix: fix ComputeTimeSmart function during rescanning process. fixes #20181~~
wallet, bugfix: fix ComputeTimeSmart function during rescanning process.
on Dec 7, 2020

BitcoinTsunami commented at 9:21 pm on December 7, 2020: none

Is it right this way ?

jonatack commented at 9:33 pm on December 7, 2020: member

Yes, thank you.

sipa commented at 9:45 pm on December 7, 2020: member

Concept ACK.

in src/wallet/wallet.cpp:3688 in 0fdd53f46c outdated

3704-                }
3705-                if (nSmartTime <= latestTolerated) {
3706-                    latestEntry = nSmartTime;
3707-                    if (nSmartTime > latestNow) {
3708-                        latestNow = nSmartTime;
3709+            if(!rescanning_old_block) {

jonatack commented at 10:11 pm on December 7, 2020:

commit 4997a2ea5f2 “wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning fixes #20181”

does the ComputeTimeSmart doxygen documentation need to be updated?
missing space

0            if (!rescanning_old_block) {

maybe start with the truthy case first, e.g. if (rescanning_old_block) {…
can drop the “fixes #20181” from the commit message, same for the test commit message (remove “refs #20181”); having “Fixes #20181” in the PR description is enough
suggestion to reviewers: review this commit diff with -w

BitcoinTsunami commented at 10:51 pm on December 8, 2020:

done locally, except doxygen documentation update, not pushed yet

BitcoinTsunami commented at 12:51 pm on December 9, 2020:

I’m not sure about how to update the doxygen documentation, because I don’t understand the way the logic is explained. The documentation split sending and receiving transaction treatment explanation, but there’s no distinction in ComputeTimeSmart in the code (or I don’t see it).

That’s why I’m not sure between which lines I should insert the new beahaviour description. Maybe something like “- If receiving a transaction during a rescanning process, assign all its (not already known) transactions’ timestamps to the block time.” between the second and the third one :

 0/**
 1 * Compute smart timestamp for a transaction being added to the wallet.
 2 *
 3 * Logic:
 4 * - If sending a transaction, assign its timestamp to the current time.
 5 * - If receiving a transaction outside a block, assign its timestamp to the
 6 *   current time.
 7 * - If receiving a transaction during a rescanning process, assign all its
 8 *   (not already known) transactions' timestamps to the block time.
 9 * - If receiving a block with a future timestamp, assign all its (not already
10 *   known) transactions' timestamps to the current time.
11 * - If receiving a block with a past timestamp, before the most recent known
12 *   transaction (that we care about), assign all its (not already known)
13 *   transactions' timestamps to the same timestamp as that most-recent-known
14 *   transaction.
15 * - If receiving a block with a past timestamp, but after the most recent known
16 *   transaction, assign all its (not already known) transactions' timestamps to
17 *   the block time.
18 *
19 * For more information see CWalletTx::nTimeSmart,
20 * https://bitcointalk.org/?topic=54527, or
21 * [#1393](/bitcoin-bitcoin/1393/).
22 */

BitcoinTsunami commented at 7:37 am on December 11, 2020:

I’ve push the proposition in a dedicated commit and will revise it if needed.

in test/functional/wallet_transactiontime_rescan.py:27 in 0fdd53f46c outdated

22+    def run_test(self):
23+        self.log.info('Prepare nodes and wallet')
24+
25+        minernode = self.nodes[0] # node used to mine BTC and create transactions
26+        usernode = self.nodes[1] # user node with correct time
27+        restorenode = self.nodes[2] # node used to restore user wallet and check time determination in ComputeSmartTime (wallet.cpp)

jonatack commented at 10:21 pm on December 7, 2020:

nit: 2 spaces before each comment per PEP8

0        restorenode = self.nodes[2]  # node used to restore user wallet and check time determination in ComputeSmartTime (wallet.cpp)

BitcoinTsunami commented at 10:50 pm on December 8, 2020:

done locally, not pushed yet

in test/functional/wallet_transactiontime_rescan.py:154 in 0fdd53f46c outdated

147+        self.log.info('Check transactions time after restoration')
148+        for tx in restorewo_wallet.listtransactions():
149+            if tx['address'] == 'bcrt1qqvs4h9g5kemwu8zdffv3c9tm3dm2qp9uzlxwzx':
150+                assert_equal(tx['blocktime'], 1525971225)
151+                assert_equal(tx['time'], 1525971225)
152+            elif tx['address'] == 'bcrt1qnw7peeq89hwrg674zjf4phzzluld798p667q7q':

jonatack commented at 10:32 pm on December 7, 2020:

The test does indeed fail without the change and passes with it.

02020-12-07T22:04:28.394000Z TestFramework (ERROR): Assertion failed
1Traceback (most recent call last):
2  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 126, in main
3    self.run_test()
4  File "test/functional/wallet_transactiontime_rescan.py", line 152, in run_test
5    assert_equal(tx['time'], 1525971225)
6  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 50, in assert_equal
7    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
8AssertionError: not(1527699225 == 1525971225)

But the test commit needs to be after the commit that makes the change, not before, otherwise the test commit fails. The tests and CI should pass for each commit. Also, you can squash the linter fixup commit into the test commit. (Note that you can run the Python test linter on the command line with test/lint/lint-python.sh).

BitcoinTsunami commented at 10:50 pm on December 8, 2020:

done locally, not pushed yet

in test/functional/wallet_transactiontime_rescan.py:30 in 0fdd53f46c outdated

25+        minernode = self.nodes[0] # node used to mine BTC and create transactions
26+        usernode = self.nodes[1] # user node with correct time
27+        restorenode = self.nodes[2] # node used to restore user wallet and check time determination in ComputeSmartTime (wallet.cpp)
28+
29+        # time constant
30+        mocktime = 1525107225

jonatack commented at 10:37 pm on December 7, 2020:

could use current time, grep in the functional tests for int(time.time()

BitcoinTsunami commented at 11:46 am on December 9, 2020:

done locally, not pushed yet

in test/functional/wallet_transactiontime_rescan.py:56 in 0fdd53f46c outdated

49+        usernode.createwallet(wallet_name='wo', disable_private_keys=True)
50+        wo_wallet = usernode.get_wallet_rpc('wo')
51+
52+        wo_wallet.importpubkey(pubkey='02f8802962c2ca756d8961c719f75bdfff2b6dbb775788ed54a68bcb182c0ea1ef')
53+        wo_wallet.importpubkey(pubkey='0330f4a96800a38fa0a49cde4b5f802e95a0c1044e5954389a8d00cecde01b842b')
54+        wo_wallet.importpubkey(pubkey='02f5ccbee49546e0b5bf2e76422ed959e983dafaa94c8abafc9c8f8b99ef23d16a')

jonatack commented at 10:38 pm on December 7, 2020:

The addresses and pubkeys don’t need to be hardcoded. Have a look in other tests how this can be done (grep for “importpubkey”, “deterministic”, “pub_key”, “priv_key”, etc.)

BitcoinTsunami commented at 12:03 pm on December 9, 2020:

I’ve a question about this one. My first local version didn’t use any hardcoded key and I use getaddressinfo and the pubkey field. I found my test a little bit hard to read, because I needed 4 wallets to realise it (and one, just to get a couple of address/pubkey). I find that by hardcoding address and pubkey, I could remove one wallet, and let the reader of the test be more focus on the 3 really important wallets.

Can you confirm me that I need to create a wallet to avoid hardcoded pubkey ? Or is there something I’m missing here.

If another wallet is required, I’ll try to isolate it better from the 3 others than in my first attempt.

BitcoinTsunami commented at 11:27 pm on December 10, 2020:

I’ve find a way with getnewaddress() on the node. The test is rewrited with importaddress() instead of importpubkey(). It’s simplier and demonstrated the same thing. Done locally, not pushed yet.

jonatack commented at 10:38 pm on December 7, 2020: member

Concept ACK

in src/wallet/wallet.h:278 in 0fdd53f46c outdated

663@@ -664,7 +664,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
664      * Abandoned state should probably be more carefully tracked via different
665      * posInBlock signals or by checking mempool presence when necessary.
666      */
667-    bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Confirmation confirm, bool fUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
668+    bool AddToWalletIfInvolvingMe(const CTransactionRef& tx, CWalletTx::Confirmation confirm, bool fUpdate, bool rescanning_old_block) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

jonatack commented at 10:49 pm on December 7, 2020:

It would probaby be good to update the Doxygen documentation here.

BitcoinTsunami commented at 2:05 pm on December 9, 2020:

I need some feedback here too.

I want to add this at the end of the existing comment : “Should be called with rescanning_old_block set to true, if the transaction is not discovered in realtime, but during a rescan of old blocks.”

 0 /**
 1     * Add a transaction to the wallet, or update it.  pIndex and posInBlock should
 2     * be set when the transaction was known to be included in a block.  When
 3     * pIndex == nullptr, then wallet state is not updated in AddToWallet, but
 4     * notifications happen and cached balances are marked dirty.
 5     *
 6     * If fUpdate is true, existing transactions will be updated.
 7     * TODO: One exception to this is that the abandoned state is cleared under the
 8     * assumption that any further notification of a transaction that was considered
 9     * abandoned is an indication that it is not safe to be considered abandoned.
10     * Abandoned state should probably be more carefully tracked via different
11     * posInBlock signals or by checking mempool presence when necessary.
12     *
13     * Should be called with rescanning_old_block set to true, if the transaction is
14     * not discovered in realtime, but during a rescan of old blocks.
15     */

BitcoinTsunami commented at 7:37 am on December 11, 2020:

I’ve push the proposition in a dedicated commit and will revise it if needed.

in src/wallet/wallet.cpp:1791 in 0fdd53f46c outdated

1787@@ -1788,7 +1788,7 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc
1788                 break;
1789             }
1790             for (size_t posInBlock = 0; posInBlock < block.vtx.size(); ++posInBlock) {
1791-                SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate);
1792+                SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate, true /* rescanning_old_block */);

jonatack commented at 10:53 pm on December 7, 2020:

nit

0                SyncTransaction(block.vtx[posInBlock], {CWalletTx::Status::CONFIRMED, block_height, block_hash, (int)posInBlock}, fUpdate, /* rescanning_old_block */ true);

BitcoinTsunami commented at 10:49 pm on December 8, 2020:

done locally, not pushed yet

BitcoinTsunami commented at 7:43 am on December 8, 2020: none

Thanks for all the feedback. I’ll correct everything and ask question if needed.

decryp2kanon commented at 7:38 pm on December 8, 2020: contributor

Concept ACK. Thanks!

jonasschnelli commented at 7:52 am on December 9, 2020: contributor

Concept ACK - code looks good (haven’t looked at the tests). Code is relatively simple, basically just an if to bypass the smart-time calculation.

BitcoinTsunami force-pushed on Dec 9, 2020

BitcoinTsunami commented at 4:32 pm on December 9, 2020: none

I’ve push a new version with the rework marked as resolved done. What is not done yet:

remove hardcoded pubkey in the functionnal test : wait for feedback on the way to do it to limit useless code.
update ComputeTimeSmart doxygen documentation : wait for feedback on my proposal in comment.
update AddToWalletIfInvolvingMe doxygen documentation : wait for feedback on my proposal in comment.

BitcoinTsunami force-pushed on Dec 11, 2020

BitcoinTsunami commented at 7:39 am on December 11, 2020: none

I’ve push the full rework and add documentation update proposal in a separated commit.

BitcoinTsunami force-pushed on Dec 11, 2020

luke-jr commented at 3:53 pm on December 11, 2020: member

Weak NACK. Seems like a hacky “fix” for what is pretty much a user error…?

BitcoinTsunami commented at 2:27 pm on December 12, 2020: none

Thanks for your feedback.

I’ll try to give a broader view of the problem and of my fix, from my perspective.

First of all, the current behaviour seems to be present for years, and no recent modification has changed this. It’s rare that users encounter this one and there’s clearly no emergency here.

The issue that triggered my investigation is this one : https://github.com/bitcoin/bitcoin/issues/20181 But by looking at open issue, I’ve now found two more open issue on Github that should be related to the same behaviour : #11703 #6662 And one pull request abandoned due to the lack of activity : #12024 This pull request proposed in 2018 by @achow101 is more radical than the way I fix it, because it remove the SmartTime computation, but this raises an interesting point for me on which I would like to come back later.

The thing is that when someone encounters this behavior from Bitcoin Core wallet, it clearly has no clue on what’s going on, because it’s hard to reproduce it and the date seems so random and nonsense, that there’s no easy way to request for similar problems on search engines. And when you try to investigate it, nothing points you in the right direction.

If it’s considered as an error from the RPC API user to proceed first with a recent scan, and after with an older one, it must be documented in the RPC API Reference. And maybe it’s the right solution for this problem : simply inform that rescanblockchain function should be manipulated with some rules in mind. In that case, the documentation should precise that a miss handling of this method is fatal. Because if the API user proceeds to an improper rescan, there’s no way to correct transaction datation later : the only way is to destroy the wallet.

Here why I think it might be a good idea to fix it : the API RPC users are mainly teams working around Bitcoin Core to enrich the ecosystem with more tools. This part of the ecosystem is far more experimental (and less strict) than Bitcoin Core itself, and that’s a good thing from my point of the view. The specter desktop team has exploited the RPC rescanblockchain function to enable faster wallet import :

If it’s a segwit wallet, the rescanning process starts automatically on the first segwit enable block.
During the backup process, the first transaction blockheight is saved, to enable auto optimized reimport of this wallet later.
If the user is a power user and knows the height of his first transaction, it could restart manually from here to restore faster his wallet.

I found all these ideas interesting for a faster and better user experience and guess what, they had a bug that broke my wallet, and pushed me to look at the problem. This type of experimentation has not his place in the Bitcoin Core wallet, but is, for me, a net positive for the ecosystem. We could imagine others optimisation for an external wallet project to improve the user experience with this RPC rescanblockchain function, for example : start with a fast utxo rescanning process with UI blocked, then unblock the interface and rescan only the current year, then the previous one, then the previous one,… From an user perspective, I think that the oldest transactions are way less important than the more recents one. It’s just some random thought, but my core point is that RPC API functions are not going to be used the way the internal wallet uses them, and it’s a good thing because it enables leveraged creativity outside the Bitcoin Core project and high quality and reference code for the Bitcoin Core wallet. That’s why I think it might be good to enable a non restricted usage of RPC rescanblockchain function between the two points (start_height and stop_height), even if it’s not used by the wallet internally.

Now let’s go back to the fix itself. I’ve tried to limit the impact on my fix on the existing code, but I’ll try to give a broader view of the way I’ve thought about it to feed the conversation. As I’ve said in the description of my PR : In the context of rescanning old blocks, the only time value that has a meaning is the blocktime. That’s why my first reaction was to look for a way not to call the ComputeTimeSmart function during a rescanning of old blocks, because you don’t want to launch a complex time evaluation function in the first place when you already know that the blocktime is the only time that means something. I then discovered that the ComputeTimeSmart call was a very deep call in the hierarchy of functions and that a lot of wallet logic was shared in the middle part whatever the context. The separation of the code between block rescan processing and realtime processing would have been an incorrect approach, due to this middleware logic. The main wrong thing here, to me, is to compute the time of a specific context (realtime processing) in a shared function called deep in the hierarchy. A good way to fix it, is to push this responsibility outside the hierarchy (at least near the surface of the hierarchy). But this way will require a bigger refactoring for the wallet.cpp file. If someone finds this interesting, I could try to write a proof of concept in a branch outside a pull request. But this patch would have a far bigger impact on the code.

Finally this PR approach was a way of keeping the patch simple while preserving functionality. But If this seems too hacky, there’s 3 other ways :

Keep it as it’s today, don’t touch the wallet code, but update the RPC API Reference to explain restrictions on rescanblockchain function.
Make a bigger refactoring to push ComputeTimeSmart responsibility in the right place in the functions hierarchy.
Fix it like achow101 in his previous PR, by removing the part of ComputeTimeSmart that is only valid in a realtime context. This way, ComputeTimeSmart has not to be moved because it’s not context dependent, but you’ve restricted a part of the current functionality. It’s clearly an interesting approach too, from my point of view because it removes the root cause of the misplacement of the function in the hierarchy call.

Happy to pursue the discussion here.

kristapsk commented at 2:22 am on December 14, 2020: contributor

Seems like a hacky “fix” for what is pretty much a user error…?

How is rescanning blockchain a user error?

luke-jr commented at 4:16 am on December 14, 2020: member

Manually scanning out of order, to be specific, is a user error because the wallet is assumed to always be scanned up to its sync-point.

sipa commented at 4:37 am on December 14, 2020: member

@luke-jr I’d agree the wallet is written with that assumption, but I also don’t see a downside to changing the behavior when the wallet can know it’s dealing with an unusual situation. I haven’t dug into the details, but I suspect this change makes things match expectations in strictly more situations.

decryp2kanon commented at 6:39 pm on January 19, 2021: contributor

Concept ACK

ryanofsky approved

ryanofsky commented at 3:54 pm on February 3, 2021: member

Code review ACK ace3f77597a04785c2ab98708f31970934197044. This is great! A trivial fix replacing currently insane behavior (using latest transaction times to estimate times of transactions in old blocks) with the simple and obvious behavior of using the block timestamp as the transaction timestamp.

The only suggestion I would make is to maybe use block max time instead of block time for these transactions, to ensure that times within the set of rescanned transactions are monotonically increasing. (You can get the max time by chaining the FoundBlock expression FoundBlock().time(blocktime).maxTime(block_max_time)).

I will say I don’t actually understand Luke’s objection to this, and would be curious for an explanation. I understand (despite disagreeing with) the idea that it may be good to alert users that something is wrong by showing bad timestamps when they do something that the wallet can’t support. But it’s not clear what the exact user mistake is here, and if there is a better way to alert users about the bad usage, or just add code supporting the usage.

ryanofsky commented at 4:14 pm on February 3, 2021: member

Another thing I think could be done to improve this is to avoid adding these transactions to wtxOrdered when rescanning_old_block is true:

https://github.com/bitcoin/bitcoin/blob/ea96e17e1f2c2b0a949366260906ef02e560a425/src/wallet/wallet.cpp#L873-L874

If we want to exclude these transactions from smart time calculation, better to fully exclude instead of half-excluding them. Maybe there is a way to adjust the smart time calculations to include them, but that would require more thought and might not be worth the effort.

Another code cleanup that would be nice to see is moving nTimeReceived, nOrderPos, and wtxOrdered updates inside the ComputeTimeSmart function instead of doing them before. ComputeTimeSmart is only called one place and the current way time calculation is split up outside and inside this function is probably unnecessarily confusing.

IMO, previous ACK is still ok and switching from block time to max block time, excluding these transactions from wtxOrdered, and making ComputeTimeSmart more self-contained are all optional improvements that could be followed up later.

DrahtBot added the label Needs rebase on Feb 5, 2021

BitcoinTsunami force-pushed on Feb 12, 2021

DrahtBot removed the label Needs rebase on Feb 12, 2021

BitcoinTsunami force-pushed on Feb 12, 2021

ryanofsky commented at 9:29 pm on February 17, 2021: member

There appear to be spurious “Agent is not responding!” test failures https://cirrus-ci.com/task/5367295805489152, probably could be fixed by a new push or bulk restart

ryanofsky approved

ryanofsky commented at 9:33 pm on February 17, 2021: member

Code review ACK 0072fc36b2e4d1a3660c46dd4d8cd9e551c17917. No changes since last review other than rebase. Suggestions from #20591#pullrequestreview-582518603 to use max block time instead of block time, and #20591#pullrequestreview-582518603 to keep rescan transactions out of wtxOrdered still apply and could be implemented here or in a followup

in test/functional/wallet_transactiontime_rescan.py:64 in 0072fc36b2 outdated

59+
60+        # check blockcount
61+        assert_equal(minernode.getblockcount(), 200)
62+
63+        # generate some btc to create transactions and check blockcount
64+        minernode.generatetoaddress(300, m1)

jonatack commented at 8:01 am on February 18, 2021:

I think 101 blocks would suffice here (the test functions the same but would run faster), unless I’m missing something.

 0         # generate some btc to create transactions and check blockcount
 1-        minernode.generatetoaddress(300, m1)
 2-        assert_equal(minernode.getblockcount(), 500)
 3+        initial_mine = 101
 4+        minernode.generatetoaddress(initial_mine, m1)
 5+        assert_equal(minernode.getblockcount(), initial_mine + 200)
 6 
 7         # synchronize nodes and time
 8         self.sync_all()
 9@@ -75,7 +76,7 @@ class TransactionTimeRescanTest(BitcoinTestFramework):
10 
11         # generate blocks and check blockcount
12         minernode.generatetoaddress(100, m1)
13-        assert_equal(minernode.getblockcount(), 600)
14+        assert_equal(minernode.getblockcount(), initial_mine + 300)
15 
16         # synchronize nodes and time
17         self.sync_all()
18@@ -88,7 +89,7 @@ class TransactionTimeRescanTest(BitcoinTestFramework):
19 
20         # generate blocks and check blockcount
21         minernode.generatetoaddress(100, m1)
22-        assert_equal(minernode.getblockcount(), 700)
23+        assert_equal(minernode.getblockcount(), initial_mine + 400)
24 
25         # synchronize nodes and time
26         self.sync_all()
27@@ -101,7 +102,7 @@ class TransactionTimeRescanTest(BitcoinTestFramework):
28 
29         # generate more blocks and check blockcount
30         minernode.generatetoaddress(100, m1)
31-        assert_equal(minernode.getblockcount(), 800)
32+        assert_equal(minernode.getblockcount(), initial_mine + 500)
33 
34         # check user final balance and transaction count
35         self.log.info('Check user final balance')
36@@ -136,7 +137,7 @@ class TransactionTimeRescanTest(BitcoinTestFramework):
37 
38         # proceed to rescan, first with an incomplete one, then with a full rescan
39         self.log.info('Rescan last history part')
40-        restorewo_wallet.rescanblockchain(650)
41+        restorewo_wallet.rescanblockchain(initial_mine + 350)
42         self.log.info('Rescan all history')
43         restorewo_wallet.rescanblockchain()

BitcoinTsunami commented at 12:07 pm on February 20, 2021:

done.

jonatack commented at 8:32 am on February 18, 2021: member

ACK 0072fc36b2e4d1a3660c46dd4d8cd9e551c17917

Verified the test passes on this branch and fails on master.

Suggestions from #20591 (review) to use max block time instead of block time, and #20591 (review) to keep rescan transactions out of wtxOrdered still apply.

Agree with @ryanofsky here.

One further suggestion below.

BitcoinTsunami commented at 5:41 pm on February 19, 2021: none

Thanks ryanofsky for your feedback and suggestions ! I confirm that my latest push was just to rebase master and I was a bit perplex about CI test failing.

I’ll implement thoses two point :

Suggestions from #20591 (review) to use max block time instead of block time, and #20591 (review) to keep rescan transactions out of wtxOrdered still apply.

I need a little time to fully understand the impact of your recommandation. I may have question about it later.

jonatack : I’ll modify the functional test with your adjustment. You’re not missing anything, my test just using a large number because I was to lazy to look at the minimal working number (If I remember correctly, I was not sure If I should use 100 or 101 to unlock miner coins and I increased the number).

BitcoinTsunami force-pushed on Feb 20, 2021

BitcoinTsunami commented at 11:17 am on February 20, 2021: none

Hey I just push a first update:

fresh rebase
use block_max_time instead of blocktime following ryanofsky advice
faster functional test with jonatack recommandation.

The block_max_time advantage is clear for me now, because blocktime could sometimes not be ordered in strict chronological order. block_max_time prevents this and Foundblock helper is very practical to get it.

Following jonatack recommandation, the test is now faster and validates the exact same thing as my previous iteration.

But this update doesn’t contain any change concerning the wtxOrdered list. I can’t wrap my head around the fact that ryanofsky and jonatack sees a way to change this to obtain a better code.

At first glance, I thought I would do this first:

Another code cleanup that would be nice to see is moving nTimeReceived, nOrderPos, and wtxOrdered updates inside the ComputeTimeSmart function instead of doing them before

To cleanup the code first and change the behavior with rescanning_old_block after. But I don’t think it’s a good idea to do that : currently, ComputeTimeSmart has no side effect on the wtx transaction. It’s a good thing for me. If I do that, and you look at the resulting AddToWallet function : the fInsertedNew path will populate wtx via side effects in the ComputeTimeSmart function and the !fInsertedNew path will populate it directly in AddToWallet function. I don’t like this type of asymetric + side effects behaviors in various execution path, but I’m open to understand the way things are done in Bitcoin Core and in C++ (I don’t have any experience in C++).

After that, I just tried the simplest approach:

0if (fInsertedNew) {
1        wtx.m_confirm = confirm;
2        wtx.nTimeReceived = chain().getAdjustedTime();
3        if(!rescanning_old_block) {
4            wtx.nOrderPos = IncOrderPosNext(&batch);
5            wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
6        }
7        wtx.nTimeSmart = ComputeTimeSmart(wtx, rescanning_old_block);
8        AddToSpends(hash);
9    }

But this make me scratched my head : In the rescanning_old_block case, wtx object will be left with uninitialized field. I’m pretty sure it could produce error somewhere else… maybe not if nOrderPos != -1 is always checked before manipulating m_it_wtxOrdered, but a minima it requires more prudence to manipulate this. To be clear : I’m inconfortable because I don’t visualise all consequences.

And after that , I looked at the wtxOrdered usage, and that’s where my head explode : This list has two usages at the same time:

the first is to maintain an ordered list for ComputeTimeSmart. The ordered aspect is only used here. That’s why I understand why ryanofsky want to remove the transaction from here in the first place.
the second usage is from RPC interface in the listtransactions method. And here I don’t think that transaction order has impact (not sure, but highly probable), but I’m sure that absence of a wallet transaction has impact. I can’t remove the wtxOrdered insertion in rescanning_old_block scenario without breaking things here.

That’s why I’m stuck and don’t understand the way you two see it in the first place. Could you please help me here ? I’ll be happy to better understand implications here and way to move around this problem here !

The only way I could thing about refactoring it, is to separate the two list : maintain one unordered with all transactions and one ordered with transaction for ComputeTimeSmart only… but I doesn’t make any sense for me to do that and I can’t envision a step after this move to obtain a clearer code after that.

BitcoinTsunami commented at 11:22 pm on February 20, 2021: none

More thoughts :

I realise that the “two separate list” already exist with wtxOrdered (ordered) and mapWallet (unordered). It make more sens to me now. Because listtransactions() method in rpcwallet.cpp is dependant from wtxOrdered, I can’t skip some transactions to be added in it.

If you still think that I should avoid adding these transactions to wtxOrdered, maybe a rewrite of listtransactions() is needed to use mapWallet instead of wtxOrdered (like the listsinceblock() method in rpcwallet.cpp). I don’t know exactly how the listtransactions RPC method is used externaly. Is wtxOrdered order really important or is the order just an easy way to ensure repetable output during pagination with moving (skip, count) couple ?

Last point, if we drop this point and keep the fix as it is right now, It should be noted that in case of and RPC API user which proceed first with a recent scan, and after with an older one, the ComputeTimeSmart function is fixed, but wtxOrdered is not sorted correctly. It may not be a big deal for listtransactions() method, but it may induce an unexpected behavior in the futur if some fresh code use this list thinking it’s well sorted.

leonardojobim commented at 5:01 am on February 22, 2021: none

Tested ACK https://github.com/bitcoin/bitcoin/pull/20591/commits/6fb0173427df4b6a63131d0c338b5a2e63805561 The test passes on this branch and fails on master.

If the approach mentioned above is applied, the test will fail with restorewo_wallet.listtransactions() returning 0.

0if(!rescanning_old_block) {
1    wtx.nOrderPos = IncOrderPosNext(&batch);
2    wtx.m_it_wtxOrdered = wtxOrdered.insert(std::make_pair(wtx.nOrderPos, &wtx));
3}

maddadder commented at 5:59 pm on February 22, 2021: none

I tried this pull request today, and I still have the same problem. It’s pulling in the wrong date after a rescan. I am now attempting to reload the entire blockchain. I need this fixed for my IRS audit.

BitcoinTsunami commented at 9:08 pm on February 22, 2021: none

I tried this pull request today, and I still have the same problem. It’s pulling in the wrong date after a rescan. I am now attempting to reload the entire blockchain. I need this fixed for my IRS audit.

You must encounter a similar problem as the one that make me code this PR. You can’t recovered a wallet with incorrect datation. It’s not possible yet, even with this PR : the only way is to destroy and recreate it. This PR only prevent the datation to be corrupted in the first place.

ryanofsky approved

ryanofsky commented at 3:04 am on March 9, 2021: member

Code review ACK bb6bc61bea3098d4dab895a73c2c0f7b8d293e2b. No significant changes since last review other than rebase, optimizing test, and switching to max block time instead of block time.

Thanks for looking into txordered suggestions! It’d be nice to clean up this code more but I can see how listtransaction constrains changing txordered, and I can see upsides of keeping ComputeTimeSmart functional even I might still prefer to see time logic less spread out over two method. There’s space for more improvements later but I think this PR is clearly better than status quo.

One thing I don’t know is what problem may cause the other reported issue #20591 (comment). Might be worth filing separate bug if there are known steps to reproduce.

promag commented at 9:21 am on March 9, 2021: member

How about nTimeSmart = min(nTimeSmart, block_max_time) and avoid rescanning_old_block argument?

BitcoinTsunami commented at 9:14 pm on March 9, 2021: none

How about nTimeSmart = min(nTimeSmart, block_max_time) and avoid rescanning_old_block argument?

Could you elaborate where do you imagine using it ?

Because min function between nTimeSmart and blocktime is already present in the current Bitcoin Core code and doesn’t prevent the original problem.

I’m refering to this line : nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));

If you imagine introduce another minimum function after the previous one, you’ll invalidate the maximum function with lastestEntry and change the original behavior of the ComputeTimeSmart function. In the past achow101 has already purpose a previous PR with this type of approach (#12024). It’s an elegant way to solve the issue, but it restrict existing functionality.

This PR solve the issue without removing existing functionality.

BitcoinTsunami commented at 6:10 pm on March 12, 2021: none

Code review ACK bb6bc61. No significant changes since last review other than rebase, optimizing test, and switching to max block time instead of block time.

Thanks for looking into txordered suggestions! It’d be nice to clean up this code more but I can see how listtransaction constrains changing txordered, and I can see upsides of keeping ComputeTimeSmart functional even I might still prefer to see time logic less spread out over two method. There’s space for more improvements later but I think this PR is clearly better than status quo.

One thing I don’t know is what problem may cause the other reported issue #20591 (comment). Might be worth filing separate bug if there are known steps to reproduce.

Thanks ryanofsky for your feedback and your code review !

For the [#20591 (comment)], I need some confirmation from @charlierlee on my analysis : I think that @charlierlee encountered an incorrect datation in a wallet. After that, he searched for a solution online, found this PR and try it with a rescan of the wallet. His incorrect datation was not corrected after that and he give his feedback here.

My point is that in that case : this PR won’t save you because you can’t recover an incorrect datation. It’s an unrecoverable state. I mention in a previous comment “there’s no way to correct transaction datation later : the only way is to destroy the wallet.”. The rescan function never change a previous datation (even with a full rescan). That’s why I think this PR is important, because it prevent the bad datation to occured in the first place.

maddadder commented at 6:21 pm on March 12, 2021: none

For the [#20591 (comment)], @BitcoinTsunami yes that is correct.

ryanofsky commented at 2:48 pm on March 30, 2021: member

@promag did want to follow up on #20591 (comment)?

To me this PR seems like a straightforward improvement that gets the rescan context taken into account in a simple way and adds test coverage. I’m sure there are other ways it could be improved beyond this, but I think this change should only help make future improvements and not get in the way.

DrahtBot added the label Needs rebase on Sep 26, 2021

BitcoinTsunami force-pushed on Sep 26, 2021

BitcoinTsunami commented at 2:19 pm on September 26, 2021: none

Same code. I’ve just rebased from the latest master code to resolve conflict.

DrahtBot removed the label Needs rebase on Sep 26, 2021

in src/wallet/wallet.h:506 in 22b9a92cd4 outdated

502@@ -503,7 +503,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
503     //! @return true if wtx is changed and needs to be saved to disk, otherwise false
504     using UpdateWalletTxFn = std::function<bool(CWalletTx& wtx, bool new_tx)>;
505 
506-    CWalletTx* AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx=nullptr, bool fFlushOnClose=true);
507+    CWalletTx* AddToWallet(CTransactionRef tx, const CWalletTx::Confirmation& confirm, const UpdateWalletTxFn& update_wtx=nullptr, bool fFlushOnClose=true, bool rescanning_old_block=false);

meshcollider commented at 2:27 am on September 28, 2021:

nit: spaces around = (we use new style guidelines even if it doesn’t match with surrounding code)

BitcoinTsunami commented at 6:54 pm on September 28, 2021:

done locally, not pushed yet

in test/functional/wallet_transactiontime_rescan.py:73 in c308ceba94 outdated

68+        # synchronize nodes and time
69+        self.sync_all()
70+        minernode.setmocktime(cur_time + ten_days)
71+        usernode.setmocktime(cur_time + ten_days)
72+        restorenode.setmocktime(cur_time + ten_days)
73+        # send 10 btc to user first watch-only address

meshcollider commented at 2:32 am on September 28, 2021:

nit: user's

BitcoinTsunami commented at 6:59 pm on September 28, 2021:

done locally, not pushed yet

in test/functional/wallet_transactiontime_rescan.py:127 in c308ceba94 outdated

122+                assert_equal(tx['blocktime'], cur_time + ten_days + ten_days + ten_days)
123+                assert_equal(tx['time'], cur_time + ten_days + ten_days + ten_days)
124+
125+        # restore user wallet without rescan
126+        self.log.info('Restore user wallet on another node without rescan')
127+        restorenode.createwallet(wallet_name='wo', disable_private_keys=True, )

meshcollider commented at 2:34 am on September 28, 2021:

nit: unnecessary , after the last argument

BitcoinTsunami commented at 7:00 pm on September 28, 2021:

done locally, not pushed yet

in test/functional/wallet_transactiontime_rescan.py:134 in c308ceba94 outdated

129+
130+        restorewo_wallet.importaddress(wo1, rescan=False)
131+        restorewo_wallet.importaddress(wo2, rescan=False)
132+        restorewo_wallet.importaddress(wo3, rescan=False)
133+
134+        # check user has 0 balance and no transaction

meshcollider commented at 2:35 am on September 28, 2021:

nit: transactions

BitcoinTsunami commented at 7:01 pm on September 28, 2021:

done locally, not pushed yet

meshcollider commented at 2:45 am on September 28, 2021: contributor

Code review ACK b92d5522782a8d56f71f9ed4d04dbb068b9d65e6

Verified the test fails on current master but passes with this PR. Nits inline are non-blocking. Will merge if @ryanofsky or someone else re-ACKs.

Thanks @BitcoinTsunami!

in test/functional/wallet_transactiontime_rescan.py:78 in b92d552278 outdated

73+        # send 10 btc to user first watch-only address
74+        self.log.info('Send 10 btc to user')
75+        miner_wallet.sendtoaddress(wo1, 10)
76+
77+        # generate blocks and check blockcount
78+        minernode.generatetoaddress(100, m1)

jonatack commented at 9:50 am on September 28, 2021:

c308ceba suggestion, maybe use COINBASE_MATURITY constant

 0@@ -7,6 +7,7 @@
 1 
 2 import time
 3 
 4+from test_framework.blocktools import COINBASE_MATURITY
 5 from test_framework.test_framework import BitcoinTestFramework
 6 from test_framework.util import (
 7     assert_equal
 8@@ -61,7 +62,7 @@ class TransactionTimeRescanTest(BitcoinTestFramework):
 9         assert_equal(minernode.getblockcount(), 200)
10 
11         # generate some btc to create transactions and check blockcount
12-        initial_mine = 101
13+        initial_mine = COINBASE_MATURITY + 1
14         minernode.generatetoaddress(initial_mine, m1)
15         assert_equal(minernode.getblockcount(), initial_mine + 200)
16 
17@@ -75,7 +76,7 @@ class TransactionTimeRescanTest(BitcoinTestFramework):
18         miner_wallet.sendtoaddress(wo1, 10)
19 
20         # generate blocks and check blockcount
21-        minernode.generatetoaddress(100, m1)
22+        minernode.generatetoaddress(COINBASE_MATURITY, m1)
23         assert_equal(minernode.getblockcount(), initial_mine + 300)
24 
25         # synchronize nodes and time
26@@ -88,7 +89,7 @@ class TransactionTimeRescanTest(BitcoinTestFramework):
27         miner_wallet.sendtoaddress(wo2, 5)
28 
29         # generate blocks and check blockcount
30-        minernode.generatetoaddress(100, m1)
31+        minernode.generatetoaddress(COINBASE_MATURITY, m1)
32         assert_equal(minernode.getblockcount(), initial_mine + 400)
33 
34         # synchronize nodes and time
35@@ -101,7 +102,7 @@ class TransactionTimeRescanTest(BitcoinTestFramework):
36         miner_wallet.sendtoaddress(wo3, 1)
37 
38         # generate more blocks and check blockcount
39-        minernode.generatetoaddress(100, m1)
40+        minernode.generatetoaddress(COINBASE_MATURITY, m1)
41         assert_equal(minernode.getblockcount(), initial_mine + 500)

BitcoinTsunami commented at 7:03 pm on September 28, 2021:

done locally, not pushed yet

in src/wallet/wallet.h:276 in b92d552278 outdated

270@@ -271,8 +271,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
271      * abandoned is an indication that it is not safe to be considered abandoned.
272      * Abandoned state should probably be more carefully tracked via different
273      * posInBlock signals or by checking mempool presence when necessary.
274+     *
275+     * Should be called with rescanning_old_block set to true, if the transaction is
276+     * not discovered in realtime, but during a rescan of old blocks.

jonatack commented at 9:51 am on September 28, 2021:

b92d5522 nit, if you retouch

0     * not discovered in real time, but during a rescan of old blocks.

BitcoinTsunami commented at 7:25 pm on September 28, 2021:

done locally, not pushed yet

in test/functional/wallet_transactiontime_rescan.py:113 in b92d552278 outdated

108+        self.log.info('Check user final balance')
109+        assert_equal(wo_wallet.getbalance(), 16)
110+        assert_equal(len(wo_wallet.listtransactions()), 3)
111+
112+        # check transactions time
113+        self.log.info('Check transactions time')

jonatack commented at 9:57 am on September 28, 2021:

c308ceba can omit the comment line 113 that is redundant with line 114, and:

0        self.log.info('Check transaction times')

same for lines 149-150 below

BitcoinTsunami commented at 7:08 pm on September 28, 2021:

done locally, not pushed yet

in test/functional/wallet_transactiontime_rescan.py:108 in b92d552278 outdated

103+        # generate more blocks and check blockcount
104+        minernode.generatetoaddress(100, m1)
105+        assert_equal(minernode.getblockcount(), initial_mine + 500)
106+
107+        # check user final balance and transaction count
108+        self.log.info('Check user final balance')

jonatack commented at 10:01 am on September 28, 2021:

c308ceba94e0d8fced7cf70ec0227f7f0d80cf8c nit, can omit the comment on line 107 and move it into line 108

0        self.log.info('Check user's final balance and transaction count')

BitcoinTsunami commented at 7:10 pm on September 28, 2021:

done locally, not pushed yet

jonatack commented at 10:01 am on September 28, 2021: member

Code review re-ACK b92d5522782a8d56f71f9ed4d04dbb068b9d65e6

Edit, also re-verified the debug build is clean and the test passes on the branch and fails on master.

02021-09-28T10:46:57.258000Z TestFramework (INFO): Check transactions time after restoration
12021-09-28T10:46:57.260000Z TestFramework (ERROR): Assertion failed
2  File "/home/jon/projects/bitcoin/bitcoin/test/functional/wallet_transactiontime_rescan.py", line 154, in run_test
3    assert_equal(tx['time'], cur_time + ten_days)
4AssertionError: not(1635418013 == 1633690013)

Happy to re-review quickly for the suggestions.

ryanofsky approved

ryanofsky commented at 4:03 pm on September 28, 2021: member

Code review ACK b92d5522782a8d56f71f9ed4d04dbb068b9d65e6. No changes since previous review other than rebase (conflicts on nearby lines)

wallet: fix ComputeTimeSmart algorithm to use blocktime during old block rescanning 07b44f16e7

test: add functional test to check transaction time determination during block rescanning d6eb39af21

doc: update doxygen documention of ComputeTimeSmart() and AddToWalletIfInvolvingMe() regarding rescanning_old_block parameter 240ea294d5

BitcoinTsunami force-pushed on Sep 28, 2021

BitcoinTsunami commented at 8:07 pm on September 28, 2021: none

Thanks meshcollider and jonatack for your feedback. All your recommendations are included in the new code.

jonatack commented at 9:30 pm on September 28, 2021: member

ACK 240ea294d5e899a906f213f039b21e94c24d6018 per git diff b92d552 240ea29, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions

meshcollider commented at 10:11 pm on September 28, 2021: contributor

re-utACK 240ea294d5e899a906f213f039b21e94c24d6018

meshcollider merged this on Sep 28, 2021

meshcollider closed this on Sep 28, 2021

sidhujag referenced this in commit 89faa29cfa on Sep 28, 2021

DrahtBot locked this on Oct 30, 2022

wallet, bugfix: fix ComputeTimeSmart function during rescanning process. #20591

Conflicts