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 scenarios:

test import descriptors while background sync is in progress
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
Concept 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

Reviewers, this pull request conflicts with the following ones:

#30909 (AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress by fjahr)
#30678 (wallet: Write best block to disk before backup by fjahr)

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.

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

DrahtBot · 2024-09-23T20:26:30Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot · 2024-12-21T00:57:26Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

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

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 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

Open

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

Open

alfonsoromanz added 2 commits August 22, 2024 10:47

test, assumeutxo: import descriptors during background sync

3c1a27d

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

f522bca

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

Open

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alfonsoromanz commented Jul 16, 2024

DrahtBot commented Jul 16, 2024 •

edited

Loading

fjahr left a comment

fjahr commented Aug 22, 2024

maflcko commented Sep 16, 2024

alfonsoromanz commented Sep 16, 2024

maflcko commented Sep 16, 2024

alfonsoromanz commented Sep 16, 2024

fjahr commented Sep 16, 2024

maflcko commented Sep 16, 2024

fjahr commented Sep 16, 2024

alfonsoromanz commented Sep 16, 2024

fjahr commented Sep 17, 2024

DrahtBot commented Sep 23, 2024

DrahtBot commented Dec 21, 2024

fjahr commented Dec 22, 2024

alfonsoromanz commented Dec 27, 2024

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

DrahtBot commented Jul 16, 2024 • edited Loading

Code Coverage & Benchmarks

Reviews

Conflicts

fjahr left a comment

Choose a reason for hiding this comment

fjahr commented Aug 22, 2024

maflcko commented Sep 16, 2024

alfonsoromanz commented Sep 16, 2024

maflcko commented Sep 16, 2024

alfonsoromanz commented Sep 16, 2024

fjahr commented Sep 16, 2024

maflcko commented Sep 16, 2024

fjahr commented Sep 16, 2024

alfonsoromanz commented Sep 16, 2024

fjahr commented Sep 17, 2024

DrahtBot commented Sep 23, 2024

DrahtBot commented Dec 21, 2024

fjahr commented Dec 22, 2024

alfonsoromanz commented Dec 27, 2024

DrahtBot commented Jul 16, 2024 •

edited

Loading