test: assumeutxo: add missing tests in wallet_assumeutxo.py #30455

alfonsoromanz · 2024-07-16T16:11:06Z

Adding tests in ./test/functional/wallet_assumeutxo.py to cover the following scenario:

test loading a wallet (backup) on a pruned node

DrahtBot · 2024-07-16T16:11:09Z

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30455.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

test/functional/wallet_assumeutxo.py

fjahr

Concept ACK

test/functional/wallet_assumeutxo.py

fjahr · 2024-08-22T16:09:44Z

CI seems to have an issue with the import descriptor test

 test  2024-08-22T15:25:46.161000Z TestFramework (ERROR): Unexpected exception caught during testing 
                                   Traceback (most recent call last):
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
                                       self.run_test()
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_assumeutxo.py", line 167, in run_test
                                       result = self.import_descriptor(n1, wallet_name, key, timestamp)
                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_assumeutxo.py", line 51, in import_descriptor
                                       return wrpc.importdescriptors(import_request)
                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/coverage.py", line 50, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 127, in __call__
                                       response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 106, in _request
                                       return self._get_response()
                                              ^^^^^^^^^^^^^^^^^^^^
                                     File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 164, in _get_response
                                       http_response = self.__conn.getresponse()
                                                       ^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/usr/lib/python3.12/http/client.py", line 1428, in getresponse
                                       response.begin()
                                     File "/usr/lib/python3.12/http/client.py", line 331, in begin
                                       version, status, reason = self._read_status()
                                                                 ^^^^^^^^^^^^^^^^^^^
                                     File "/usr/lib/python3.12/http/client.py", line 300, in _read_status
                                       raise RemoteDisconnected("Remote end closed connection without"
                                   http.client.RemoteDisconnected: Remote end closed connection without response

maflcko · 2024-09-16T10:48:21Z

Are you still working on this?

alfonsoromanz · 2024-09-16T11:18:31Z

Sorry for not providing an update earlier. But yes, I continue working on this. I am trying to debug the CI issues with no success so far.

I mounted a virtual machine with ubuntu 22.04 (8GB ram), and after dealing with some libraries and memory issues I was able to run the CI job as described here .

However the jobs takes around 4-5 hours to run in the VM and I was not able to reproduce the issue yet. I was using the root user to run the command.

I am running it on a Macbook Pro with M2 Pro chip and 16GB of RAM.

Any recommendations on how to test this in my mac? I was using Parallels Desktop to virtualize but my license expired so I will move on to another alternative like VirtualBox.

maflcko · 2024-09-16T11:32:50Z

Ugh, the error is https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535

 node1 stderr ./src/validation.cpp:5569 GuessVerificationProgress: Assertion `pindex->m_chain_tx_count > 0' failed. `

This means that this is a real issue that should be fixed in the real code. This test is just surfacing it.

alfonsoromanz · 2024-09-16T12:38:00Z

Ugh, the error is https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535
 node1 stderr ./src/validation.cpp:5569 GuessVerificationProgress: Assertion `pindex->m_chain_tx_count > 0' failed. `
This means that this is a real issue that should be fixed in the real code. This test is just surfacing it.

Thanks for pointing this out. I hadn't noticed the assertion was failing.

It seems the assertion was introduced here, and my branch doesn't include it.

I think I'll need to rebase my local branch to see if I can reproduce the issue.

fjahr · 2024-09-16T15:35:47Z

It seems the assertion was introduced here, and my branch doesn't include it.

No, that's just a commit where the variable is renamed. The Assume was introduced here: 0fd915e. The assumption (pun intended) was that GuessVerificationProgress wouldn't be called in the context of assumeutxo but that was obviously not right.

I think we'll want to do something like this for now: fjahr@97a7261 Every other fix I could think of seems much more invasive and I don't think we need a perfect solution for this edge case right now.

I am thinking if this should still be fixed in v28, so that users don't see this scary Internal bug detected log, what do you think @maflcko ?

maflcko · 2024-09-16T15:47:08Z

I am thinking if this should still be fixed in v28, so that users don't see this scary Internal bug detected log, what do you think @maflcko ?

Yeah, it should probably be fixed or worked around in v28. But AU is marked experimental (if it isn't it should be done, for at least one release), so a fix for a debug log warning doesn't have to be rushed and can be done for rc3, or even 28.1, imo.

fjahr · 2024-09-16T16:21:56Z

I have opened a separate PR #30909 with my suggested fix and your test commit cherry-picked on top of it @alfonsoromanz .

alfonsoromanz · 2024-09-16T19:17:10Z

I have opened a separate PR #30909 with my suggested fix and your test commit cherry-picked on top of it @alfonsoromanz .

Thanks a lot @fjahr !

Should I modify this PR now to be on top of yours, including only the second test? Or would it be better to wait until your PR gets merged and then modify mine?

fjahr · 2024-09-17T13:57:18Z

Should I modify this PR now to be on top of yours, including only the second test? Or would it be better to wait until your PR gets merged and then modify mine?

I would recommend you wait a bit to see what the feedback on that PR is.

f20fe33 test: Add basic balance coverage to wallet_assumeutxo.py (Fabian Jahr) 037b101 test: Add coverage for best block locator write in wallet_backup (Fabian Jahr) 31c0df0 wallet: migration, write best locator before unloading wallet (furszy) 7e3dbe4 wallet: Write best block to disk before backup (Fabian Jahr) Pull request description: I discovered that we don't write the best block to disk when trying to explain the behavior described here: #30455 (comment) 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. ACKs for top commit: achow101: ACK f20fe33 furszy: ACK f20fe33 Tree-SHA512: bb384a940df5c942fffe2eb06314ade4fc5d9b924012bfef3b1c456c4182a30825d1e137d8ae561d93d3a8a2f4d1c1ffe568132d20fa7d04844f1e289ab4a28b

fjahr · 2024-12-22T21:57:57Z

@alfonsoromanz While #30909 is stalling a bit it seems like it is the way we'll move forward when there is some more review since there was no push back on concept/approach. So I would suggest you proceed here assuming that #30909 is acceptable. Ideally you would change this PR to have the second test standalone, if that's not possible you can base it on #30909.

alfonsoromanz · 2024-12-27T15:37:05Z

@alfonsoromanz While #30909 is stalling a bit it seems like it is the way we'll move forward when there is some more review since there was no push back on concept/approach. So I would suggest you proceed here assuming that #30909 is acceptable. Ideally you would change this PR to have the second test standalone, if that's not possible you can base it on #30909.

Thanks for the feedback @fjahr! I will be working on this

alfonsoromanz · 2025-01-16T13:12:33Z

Pushed b1affc7 to address the latest feedback provided by @fjahr. Also updated the docs as mentioned here #31556 (comment)

fjahr

Did another pass, mostly minor comments, otherwise looks pretty good. Will do a final pass when my comments have been addressed.

test/functional/wallet_assumeutxo.py

alfonsoromanz · 2025-01-19T22:29:43Z

Did another pass, mostly minor comments, otherwise looks pretty good. Will do a final pass when my comments have been addressed.

Thanks for the detailed feedback, @fjahr!. I pushed 2f19e42 with your requested changes.

fjahr · 2025-01-20T15:02:18Z

Code review ACK 2f19e42

…ove wallet RPC errors 9d2d9f7 rpc: Include assumeutxo as a failure reason of rescanblockchain (Fabian Jahr) 595edee test, assumeutxo: import descriptors during background sync (Alfonso Roman Zubeldia) d73ae60 rpc: Improve importdescriptor RPC error messages (Fabian Jahr) 27f99b6 validation: Don't assume m_chain_tx_count in GuessVerificationProgress (Fabian Jahr) 42d5d53 interfaces: Add helper function for wallet on pruning (Fabian Jahr) Pull request description: A test that is added as part of #30455 uncovered this issue: The `GuessVerificationProgress` function is used during during descriptor import and relies on `m_chain_tx_count`. In #29370 an [`Assume` was added](0fd915e) expecting the `m_chaint_tx_count` to be set. However, as the test uncovered, `GuessVerificationProgress` is called with background sync blocks that have `m_chaint_tx_count = 0` when they have not been downloaded and processed yet. The simple fix is to remove the `Assume`. Users should not be thrown off by the `Internal bug detected` error. The behavior of `importdescriptor` is kept consistent with the behavior for blocks missing due to pruning. The test by alfonsoromanz is cherry-picked here to show that the [CI errors](https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535) should be fixed by this change. This PR also improves error messages returned by the `importdescriptors` and `rescanblockchain` RPCs. The error message now changes depending on the situation of the node, i.e. if pruning is happening or an assumutxo backgroundsync is active. ACKs for top commit: achow101: ACK 9d2d9f7 mzumsande: Code Review ACK 9d2d9f7 furszy: Code review ACK 9d2d9f7 Tree-SHA512: b841a9b371e5eb8eb3bfebca35645ff2fdded7a3e5e06308d46a33a51ca42cc4c258028c9958fbbb6cda9bb990e07ab8d8504dd9ec6705ef78afe0435912b365

alfonsoromanz · 2025-02-02T10:56:42Z

Rebased

fjahr · 2025-02-02T13:13:49Z

Re-ACK bdec262

Only rebased since last review.

fanquake · 2025-02-20T19:38:31Z

@furszy you might want to review here?

furszy

Could you decouple this test into its own isolated function?
The current monolithic approach makes it harder to review, even when the changes seem simple.
See for example how wallet_migration.py is structured.

alfonsoromanz · 2025-03-21T15:00:43Z

Force-pushed to address @furszy's feedback

fjahr

re-ACK 55c6a69

Just moved the newly added tests into separate functions (which necessitated duplicating the error message).

DrahtBot added the Tests label Jul 16, 2024

maflcko reviewed Jul 16, 2024

View reviewed changes

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

alfonsoromanz force-pushed the wallet_assumeutxo_tests branch from 776b207 to 2df9980 Compare July 16, 2024 17:27

fjahr mentioned this pull request Jul 16, 2024

AssumeUTXO Mainnet Readiness Tracking #29616

Closed

32 tasks

fjahr reviewed Jul 16, 2024

View reviewed changes

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

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

DrahtBot added the CI failed label Jul 16, 2024

danielabrozzoni reviewed Jul 17, 2024

View reviewed changes

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

fjahr reviewed Aug 15, 2024

View reviewed changes

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

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

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

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

This was referenced Aug 19, 2024

wallet: Write best block to disk before backup #30678

Merged

wallet: Ensure best block matches wallet scan state #30221

Merged

wallet: lastprocessedblock can be inconsistent with internal best block #30686

Open

alfonsoromanz force-pushed the wallet_assumeutxo_tests branch from 2df9980 to f522bca Compare August 22, 2024 14:37

fjahr mentioned this pull request Sep 16, 2024

wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors #30909

Merged

DrahtBot marked this pull request as draft September 23, 2024 12:26

DrahtBot added the Needs rebase label Sep 23, 2024

mzumsande mentioned this pull request Dec 30, 2024

validation: Send correct notification during snapshot completion #31556

Merged

alfonsoromanz force-pushed the wallet_assumeutxo_tests branch from f522bca to b1affc7 Compare January 15, 2025 22:38

DrahtBot removed the Needs rebase label Jan 16, 2025

alfonsoromanz marked this pull request as ready for review January 16, 2025 13:10

maflcko removed the CI failed label Jan 16, 2025

fjahr reviewed Jan 19, 2025

View reviewed changes

alfonsoromanz force-pushed the wallet_assumeutxo_tests branch from b1affc7 to 2f19e42 Compare January 19, 2025 21:12

DrahtBot added the Needs rebase label Jan 31, 2025

alfonsoromanz force-pushed the wallet_assumeutxo_tests branch from 2f19e42 to bdec262 Compare February 2, 2025 00:38

DrahtBot removed the Needs rebase label Feb 4, 2025

furszy reviewed Mar 20, 2025

View reviewed changes

test, assumeutxo: loading a wallet (backup) on a pruned node

55c6a69

alfonsoromanz force-pushed the wallet_assumeutxo_tests branch from bdec262 to 55c6a69 Compare March 21, 2025 13:11

fjahr reviewed Mar 22, 2025

View reviewed changes

fanquake requested a review from furszy March 23, 2025 01:29

DrahtBot added CI failed and removed CI failed labels Jul 3, 2025

test: assumeutxo: add missing tests in wallet_assumeutxo.py #30455

Are you sure you want to change the base?

test: assumeutxo: add missing tests in wallet_assumeutxo.py #30455

Conversation

alfonsoromanz commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjahr commented Aug 22, 2024

Uh oh!

maflcko commented Sep 16, 2024

Uh oh!

alfonsoromanz commented Sep 16, 2024

Uh oh!

maflcko commented Sep 16, 2024

Uh oh!

alfonsoromanz commented Sep 16, 2024

Uh oh!

fjahr commented Sep 16, 2024

Uh oh!

maflcko commented Sep 16, 2024

Uh oh!

fjahr commented Sep 16, 2024

Uh oh!

alfonsoromanz commented Sep 16, 2024

Uh oh!

fjahr commented Sep 17, 2024

Uh oh!

fjahr commented Dec 22, 2024

Uh oh!

alfonsoromanz commented Dec 27, 2024

Uh oh!

alfonsoromanz commented Jan 16, 2025

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alfonsoromanz commented Jan 19, 2025

Uh oh!

fjahr commented Jan 20, 2025

Uh oh!

alfonsoromanz commented Feb 2, 2025

Uh oh!

fjahr commented Feb 2, 2025

Uh oh!

fanquake commented Feb 20, 2025

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

alfonsoromanz commented Mar 21, 2025

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alfonsoromanz commented Jul 16, 2024 •

edited

Loading

DrahtBot commented Jul 16, 2024 •

edited

Loading