wallet: Write best block to disk before backup

fjahr commented at 11:35 pm on August 19, 2024: contributor

I discovered that we don’t write the best block to disk when trying to explain the behavior described here: #30455 (review)

In the context of that test, the behavior is confusing and I think it also shows that one of the already existing tests in wallet_assumeutxo.py doesn’t actually test what it says. It only fails because the best block isn’t written and actually, the height of the backup that is loaded is at the snapshot height during backup. So it really shouldn’t fail since it’s past the background sync blocks already.

I’m not sure if this is super relevant in practice though so I am first looking for concept ACKs on the BackupWallet code change. Either way, I think this behavior should be documented better if it is left as is and the test should be changed.

DrahtBot commented at 11:35 pm on August 19, 2024: contributor

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	furszy, achow101
Concept ACK	sipa
Stale ACK	ryanofsky, ismaelsadeeq

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:

#30909 (AssumeUTXO: Don’t Assume m_chain_tx_count in GuessVerificationProgress by fjahr)
#30455 (test: assumeutxo: add missing tests in wallet_assumeutxo.py by alfonsoromanz)
#30221 (wallet: Ensure best block matches wallet scan state by achow101)

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 Aug 19, 2024

in src/wallet/wallet.cpp:3420 in 4ffdc9b707 outdated

3410+        WalletBatch batch{GetDatabase()};
3411+        CBlockLocator loc{m_chain->getTipLocator()};
3412+        if (!loc.IsNull()) {
3413+            batch.WriteBestBlock(loc);
3414+        }
3415+    }

furszy commented at 2:24 am on August 20, 2024:

If the node’s chain is ahead of the wallet, you would be writing a locator in the future. Which would make the wallet skip blocks during the next rescan attempt. Should lock the wallet mutex and obtain the wallet’s best locator from the wallet’s last processed block (m_last_block_processed).

ryanofsky commented at 8:15 pm on August 20, 2024:

re: #30678 (review)

Should lock the wallet mutex and obtain the wallet’s best locator from the wallet’s last processed block (m_last_block_processed).

Left a similar suggestion #30686 (comment) implementing that

sipa commented at 2:24 am on August 20, 2024: member

This could cause a wallet (and its backup) to have a best block locator that’s unknown to the block index, if a crash happens in between the backup and the next database flush. How do we behave when loading such a wallet file?

Other than that, Concept ACK.

furszy commented at 2:31 am on August 20, 2024: member

See #30221. It fixes this in a more comprehensive way. The comment I left is secondary.

achow101 commented at 2:39 am on August 20, 2024: member

This could cause a wallet (and its backup) to have a best block locator that’s unknown to the block index, if a crash happens in between the backup and the next database flush. How do we behave when loading such a wallet file?

No issues, we just rescan from the most recent fork indicated by the locator. It’s the same as loading a wallet when the node isn’t synced yet.

fjahr commented at 10:52 am on August 20, 2024: contributor

See #30221. It fixes this in a more comprehensive way.

Ok, I didn’t find it and weirdly there seems to be no conflict…

fjahr closed this on Aug 20, 2024

fjahr commented at 10:11 pm on August 20, 2024: contributor

Reopening due to interest from @ryanofsky in #30686. I implemented the suggestion improvements and expanded the test a little including some more detailed comments. If this is an improvement already maybe this has a chance to improve things while the approach of #30221 is still being discussed.

fjahr reopened this on Aug 20, 2024

fjahr force-pushed on Aug 20, 2024

in src/wallet/wallet.cpp:3420 in 888a167446 outdated

3411+        WITH_LOCK(cs_wallet, chain().findBlock(m_last_block_processed, FoundBlock().locator(loc)));
3412+        if (!loc.IsNull()) {
3413+            WalletBatch batch(GetDatabase());
3414+            batch.WriteBestBlock(loc);
3415+        }
3416+    }

ryanofsky commented at 10:38 pm on August 20, 2024:

In commit “wallet: Write best block to disk before backup” (888a167446901c12960e6776613a3a1fb9789f59)

I wonder if it makes sense to put this in the CWallet::Flush method and call Flush() here. For example, this also seems like a good thing to do when unloading a wallet.

fjahr commented at 8:09 pm on August 21, 2024:

I just looked into this and from my understanding CWallet::Flush() delegates to the Flush() method of the underlying Database and that method is a no-op in SQLite. Since we are moving to SQLite and remove BDB entirely my guess is that the Flush() method will be removed at some point in the future as well.

ryanofsky commented at 1:00 pm on August 22, 2024:

re: #30678 (review)

I think that’s right, but I don’t see these sqlite or bdb implementation details being relevant here. My point is that before unloading a wallet, and before backing up a wallet, it would seem like a good idea to flush wallet state that’s in memory to disk, so information is not lost. So it would seem logical to do that in the Flush method and to call Flush before backing up the wallet, since it is already called before unloading a wallet

furszy commented at 3:21 pm on August 25, 2024:

The idea of moving this to Flush() sounded promising to me too but I don’t think we can/should do it after some digging. At least not without doing some extra restructuring.

The shutdown procedure stops the wallet, closing the db connection, before the wallet is destructed (which is when the flushing occurs). So any db write during Flush should cause a segfault due to the db handler access.

Also, moving this to Flush() would write the same locator twice during shutdown. And this is because we force flushing everything to disk via the chainStateFlushed validation signal triggered from the init Shutdown() -> ForceFlushStateToDisk.

ryanofsky approved

ryanofsky commented at 2:47 pm on August 21, 2024: contributor

Code review ACK 888a167446901c12960e6776613a3a1fb9789f59. I think this would be a good change and improvement, however:

I don’t have a high level of confidence about this so would be interested in opinions from achow, furszy, and others who could point out other effects and drawbacks.
If we are going to update the locator when backing up wallets, it seems like it might make sense to update it in other cases like when unloading wallets (https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1724059212)
Concern that this might make wallet sync less efficient in case of a node crash where the node’s block index is not flushed #30678 (comment) seems worth nothing somewhere, maybe in a code comment.

DrahtBot requested review from sipa on Aug 21, 2024

furszy commented at 1:41 pm on August 26, 2024: member

I’m not seeing any drawback from doing it prior to a backup so far.

But the Flush() discussion (https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1724059212) made me think about the migration process. We aren’t updating the best locator before unloading the wallet. So, might also want to add https://github.com/furszy/bitcoin-core/commit/5b618f1b024f16640a2a05f1e269f61b33b86577.

luke-jr commented at 0:09 am on August 27, 2024: member

Maybe we should have a custom locator with the last-change block, and last-scanned block?

in test/functional/wallet_assumeutxo.py:71 in 888a167446 outdated

61@@ -62,9 +62,15 @@ def run_test(self):
62         for n in self.nodes:
63             n.setmocktime(n.getblockheader(n.getbestblockhash())['time'])
64 
65+        # Create a wallet that we will create a backup for later (at snapshot height)
66         n0.createwallet('w')
67         w = n0.get_wallet_rpc("w")
68

ismaelsadeeq commented at 4:24 pm on September 16, 2024:

It would be beneficial to have this in a separate test method so that we can add more test cases cleanly without associating it with the assume UTXO test. For example, we can test https://github.com/furszy/bitcoin-core/commit/5b618f1b024f16640a2a05f1e269f61b33b86577 by creating a non-descriptor wallet, migrating it, and attempt to restore it.

fjahr commented at 10:03 pm on September 18, 2024:

Point taken that this is about backups and not assumeutxo primarily, so I have added another test for this inwallet_backup.py.

It would be beneficial to have this in a separate test method so that we can add more test cases cleanly without associating it with the assume UTXO test.

I don’t fully get this comment though. I don’t know how I could make a method that would make this code reusable. The code in wallet_assumeutxo.py is spread across the test and pretty specific to the assumeutxo context. I don’t know how that would have help me in the other test I added.

For example, we can test https://github.com/furszy/bitcoin-core/commit/5b618f1b024f16640a2a05f1e269f61b33b86577 by creating a non-descriptor wallet, migrating it, and attempt to restore it.

I don’t know how to do this or rather I think it’s much more complicated than you make it sound. There needs to be a way for the blocks close to the backup height to be missing in order to hit this issue. This can happen because we are using assumeutxo, which was my initial use case for this, and in the new test I added, I achieved this with pruning. But in the walletmigration context, both wallets (before and after migration) are on the same node and the backup is created as part of the process but without any actual user interaction in that part of the process. So to hit this case we would make the wallet migration fail after the backup is created, mine a few more blocks, then prune the node to the height where the migration failed, and then try to use the backup from the failed migration.

Maybe I am missing something because I am not super familiar with the walletmigration code though.

ismaelsadeeq commented at 3:34 pm on September 24, 2024:

The code in wallet_assumeutxo.py is spread across the test and pretty specific to the assumeutxo context. I don’t know how that would have help me in the other test I added.

Never mind test_pruned_wallet_backup is even better than what I was thinking.

So to hit this case we would make the wallet migration fail after the backup is created, mine a few more blocks, then prune the node to the height where the migration failed, and then try to use the backup from the failed migration.

I dont think it needs to fail, because the migration process will create a backup of the wallet before migrating. which is still available after the wallet successfully finish migrating.

So we can just try restoring the backed up file created in that process in test_pruned_wallet_backup
see https://github.com/ismaelsadeeq/bitcoin/commit/725b4975cd7d3f38a7b24f749c0342eeb522f0ee

ismaelsadeeq commented at 4:26 pm on September 16, 2024: member

Concept ACK, and ACK after resolving #30678#pullrequestreview-2260723558

Should also add a test for when a wallet is migrated, I left a suggested for that.

fjahr force-pushed on Sep 18, 2024

fjahr commented at 10:06 pm on September 18, 2024: contributor

Sorry for the long wait. I added the following changes:

rebased to get rid of autotools here
added commit from @furszy for the migration context as suggested here: #30678#pullrequestreview-2260723558
added a separate test for this in wallet_backup.py to make sure this is covered not only in the assumutxo context but also the pruning context

fjahr commented at 10:11 pm on September 18, 2024: contributor

Maybe we should have a custom locator with the last-change block, and last-scanned block?

Seems like a bigger change, I will leave this to be explored in a follow-up. It should probably be weighed against the potential follow-up changes proposed in #30221.

fjahr force-pushed on Sep 18, 2024

in test/functional/wallet_assumeutxo.py:186 in 8f66a4bfd6 outdated

170@@ -160,7 +171,7 @@ def run_test(self):
171         self.wait_until(lambda: len(n1.getchainstates()['chainstates']) == 1)
172 
173         self.log.info("Ensuring wallet can be restored from backup")
174-        n1.restorewallet("w", "backup_w.dat")
175+        n1.restorewallet("w2", "backup_w2.dat")

furszy commented at 8:46 pm on September 19, 2024:

nit: would be good to mention this backup is from before the snapshot in the logging line. And maybe perform few extra checks after wise? maybe the resulting balance (this last point could be left for another PR too)

fjahr commented at 4:11 pm on September 20, 2024:

I have amended the log message and I have added basic coverage for the balances of the wallet. That required some further changes to the test as a whole since there were no balances before due to the use of miniwallet, so I have done this in a separate commit.

furszy commented at 9:20 pm on September 19, 2024: member

Code review ACK 38648f4940eb13ebc858081c63e587156428107a

DrahtBot requested review from ismaelsadeeq on Sep 19, 2024

DrahtBot requested review from ryanofsky on Sep 19, 2024

in test/functional/wallet_backup.py:149 in 38648f4940 outdated

139@@ -140,6 +140,23 @@ def restore_wallet_existent_name(self):
140         assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file)
141         assert wallet_file.exists()
142 
143+    def test_pruned_wallet_backup(self):
144+        self.log.info("Test loading backup on a pruned node with backup was created close to the prune height of the restoring node")
145+        node = self.nodes[3]
146+        self.restart_node(3, ["-prune=1", "-fastprune=1"])
147+        # We need a few more blocks so we can actually get above an realistic

ismaelsadeeq commented at 11:14 am on September 20, 2024:

nit: Assert that the chain height is 214, because the test assume so?

0        # Ensure the chain tip is at height 214, because this test assume it is.
1        assert_equal(node.getchaintips()[0]["height"], 214)
2        # We need a few more blocks so we can actually get above an realistic

fjahr commented at 4:09 pm on September 20, 2024:

added

in test/functional/wallet_backup.py:144 in 38648f4940 outdated

139@@ -140,6 +140,23 @@ def restore_wallet_existent_name(self):
140         assert_raises_rpc_error(-36, error_message, node.restorewallet, wallet_name, backup_file)
141         assert wallet_file.exists()
142 
143+    def test_pruned_wallet_backup(self):
144+        self.log.info("Test loading backup on a pruned node with backup was created close to the prune height of the restoring node")

ismaelsadeeq commented at 11:17 am on September 20, 2024:

nit

0        self.log.info("Test loading backup on a pruned node when the backup was created close to the prune height of the restoring node")

fjahr commented at 4:09 pm on September 20, 2024:

done

DrahtBot requested review from ismaelsadeeq on Sep 20, 2024

ismaelsadeeq commented at 11:33 am on September 20, 2024: member

code review ACK 38648f4940eb13ebc858081c63e587156428107a

nice adding 38648f4940eb13ebc858081c63e587156428107a 👍🏾

wallet: Write best block to disk before backup

This ensures that the best block is included in the backup which leads to a more consistent behavior when loading the backup.

7e3dbe4180

wallet: migration, write best locator before unloading wallet 31c0df0389

test: Add coverage for best block locator write in wallet_backup 037b101e80

fjahr force-pushed on Sep 20, 2024

fjahr commented at 8:30 pm on September 20, 2024: contributor

Addressed feedback from @ismaelsadeeq and @furszy , fixing logs and adding more test coverage.

in test/functional/wallet_assumeutxo.py:193 in bd8fee5123 outdated

186+        assert_equal(n1.getbalance(), 340)
187+
188+        # Check balance of w wallet after node is synced
189+        w = n1.get_wallet_rpc("w")
190+        n1.loadwallet("w")
191+        assert_equal(w.getbalance(), 34)

furszy commented at 2:28 pm on September 22, 2024:

nit: w would throw an error if used prior to loading the wallet. Should first load the wallet and secondly call get_wallet_rpc.

fjahr commented at 5:19 pm on September 22, 2024:

done

in test/functional/wallet_assumeutxo.py:121 in bd8fee5123 outdated

114@@ -111,7 +115,11 @@ def run_test(self):
115 
116         # Mine more blocks on top of the snapshot that n1 hasn't yet seen. This
117         # will allow us to test n1's sync-to-tip on top of a snapshot.
118-        self.generate(n0, nblocks=100, sync_fun=self.no_op)
119+        for i in range(100):
120+            if i % 3 == 0:
121+                self.mini_wallet.send_to(from_node=n0, scriptPubKey=address_to_scriptpubkey(w_address), amount=1 * COIN)
122+                self.mini_wallet.send_to(from_node=n0, scriptPubKey=address_to_scriptpubkey(w2_address), amount=10 * COIN)

furszy commented at 2:29 pm on September 22, 2024:

nit: could call address_to_scriptpubkey() only once outside the for loop.

fjahr commented at 5:19 pm on September 22, 2024:

done

furszy commented at 2:30 pm on September 22, 2024: member

ACK bd8fee512301059cc03673099422c19b39f2ed65

DrahtBot requested review from ismaelsadeeq on Sep 22, 2024

test: Add basic balance coverage to wallet_assumeutxo.py f20fe33e94

fjahr force-pushed on Sep 22, 2024

fjahr commented at 5:19 pm on September 22, 2024: contributor

Added improvements suggested by @furszy , thanks!

furszy commented at 11:16 am on September 23, 2024: member

ACK f20fe33

achow101 commented at 7:41 pm on September 23, 2024: member

ACK f20fe33e94c6752e5d2ed92511c0bf51a10716ee

achow101 merged this on Sep 23, 2024

achow101 closed this on Sep 23, 2024

ismaelsadeeq commented at 3:35 pm on September 24, 2024: member

post-merge code review and tested re-ACK f20fe33e94c6752e5d2ed92511c0bf51a10716ee

wallet: Write best block to disk before backup #30678

Code Coverage

Reviews

Conflicts