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

fjahr commented at 4:19 pm on September 16, 2024: contributor

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

DrahtBot commented at 4:19 pm on September 16, 2024: contributor

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/30909.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	mzumsande, furszy, achow101
Stale ACK	alfonsoromanz

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:

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

alfonsoromanz approved

alfonsoromanz commented at 7:09 pm on September 16, 2024: contributor

tACK 9be080abfa87543652c415af4e239a6ba4d2b2e1

The test passes locally, and I see that all the CI checks have passed, so it looks good to me.

It took me some time today to get familiar with the CMake-based build system, but I managed to get it working.

Thanks!

in src/validation.cpp:5626 in 9be080abfa outdated

5565@@ -5566,9 +5566,8 @@ double GuessVerificationProgress(const ChainTxData& data, const CBlockIndex *pin
5566     if (pindex == nullptr)
5567         return 0.0;
5568 
5569-    if (!Assume(pindex->m_chain_tx_count > 0)) {
5570-        LogWarning("Internal bug detected: block %d has unset m_chain_tx_count (%s %s). Please report this issue here: %s\n",
5571-                   pindex->nHeight, PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT);
5572+    if (pindex->m_chain_tx_count == 0) {

jonatack commented at 7:16 pm on September 16, 2024:

Question, is debug logging this enough, unsure but e.g. in the various RPCs (and CLI -getinfo) that return a “verificationprogress” field, are there any where it would be a good idea to describe the what/why of a returned 0.0 value due to this case.

mzumsande commented at 9:14 pm on September 18, 2024:

I think this should only be possible to trigger with the rescan, the various RPCs are always called with a block (usually the tip) that is guaranteed to have m_chain_tx_count set.

fjahr commented at 7:11 pm on September 23, 2024:

And aside from only hitting this in the context of rescan + blocks missing this is just about showing the progress and not the actual functionality the user seeks. So it didn’t feel important enough for me to use another category mainly because of that. Unless there are strong opinions against it I will keep this as is.

maflcko commented at 6:42 am on September 17, 2024: member

However, as the test uncovered, GuessVerificationProgress is called with snapshot blocks that have m_chaint_tx_count = 0 when the assumeutxo background sync is in progress.

Can you explain this a bit better? IIRC the snapshot base block itself has the count set, as well as any validated blocks after it. See src/node/blockstorage.cpp: base->m_chain_tx_count = au_data.m_chain_tx_count;. So I think calling GuessVerificationProgress in this case shouldn’t lead to issues. Whereas calling a rescan on the (missing) background blocks seems like a bug that should be avoided at the call site, no?

mzumsande commented at 9:09 pm on September 18, 2024: contributor

Whereas calling a rescan on the (missing) background blocks seems like a bug that should be avoided at the call site, no?

Well, the approach seems to be similar with pruning: importdescriptors tries to scan as many blocks as possible (no stopping after a failure due to missing block data), and then returns a failure / warns the user that the result may be incomplete. It could probably have been implentend differently, checking if we are missing blocks due to pruning and refuse to import the descriptors, but it doesn’t do that.

If that is seen as a viable way for the pruning case, maybe it would be a bit inconsistent to just forbid it in the assumeutxo background sync case?! In which case just dealing with the progress estimation error would be ok?

maflcko commented at 6:13 am on September 19, 2024: member

Well, the approach seems to be similar with pruning:

Sure, if that is the case, the pull request description should mention it. Also, the error message could probably be updated, because it only mentions pruning, no AU. (Note to self: Reminds me of #26282)

mzumsande commented at 3:38 pm on September 19, 2024: contributor

Though one could also make a case for not allowing importdescriptor/rescan during background sync if non-downloaded blocks are affected, with the reasoning “just wait a little bit longer” that doesn’t really apply to the pruning case. Would be interesting to know if this has ever been discussed for assumeutxo.

fjahr force-pushed on Sep 23, 2024

fjahr commented at 7:51 pm on September 23, 2024: contributor

Can you explain this a bit better?

Yeah, my statement was wrong. I think I managed to confuse myself with some wrongfully placed debug logging. I have fixed the description.

Well, the approach seems to be similar with pruning:

Sure, if that is the case, the pull request description should mention it. Also, the error message could probably be updated, because it only mentions pruning, no AU.

Updated the error message and realized that he same issue exists in rescanblockchain, too. So fixed it there as well.

EDIT: I also add coverage for rescanblockchain in wallet_assumeutxo.

Though one could also make a case for not allowing importdescriptor/rescan during background sync if non-downloaded blocks are affected, with the reasoning “just wait a little bit longer” that doesn’t really apply to the pruning case. Would be interesting to know if this has ever been discussed for assumeutxo.

I had a similar thoughts when I looked into this earlier. But I think going that route might either break workflows with importdescriptors or it’s a pretty invasive change. The RPC takes in multiple descriptors (requests) with timestamps for each and then rescans blocks starting from around the lowest timestamp. The simplest approach would be to check all the blocks from the lowest timestamp and abort the whole RPC if any of them don’t have block data available for any block since the timestamp. Or we could exclude the offending requests and only import the remaining requests, starting rescan from the lowest timestamp with available block data. Both of these approaches seem pretty invasive and may lead to unexpected outcomes for users. I think people may undershoot the timestamp for example. We could then help them by returning the lowest timestamp that works (the snapshot height one) with the error but then we basically recreate the current behavior with extra steps. The ideal solution would be that the wallet rescans the missing blocks as they come in but that’s probably a much bigger change, and also kind of tricky to communicate to the user.

Also, while “just wait a little bit longer” might be a good answer, we could also say “just run another rescan later” and that may ultimately be a better experience for the users.

I don’t have a strong opinion here but it seems like these ideas may better be scoped out in a separate issue first and then potentially be implemented as a follow-up.

fjahr force-pushed on Sep 23, 2024

fjahr commented at 8:17 pm on September 23, 2024: contributor

@achow101 do you have thoughts on descriptor import/rescan behavior with missing blocks because of ongoing assumeutxo background sync?

fjahr renamed this:
~~AssumeUTXO: Don't Assume m_chain_tx_count in GuessVerificationProgress~~
wallet, assumeutxo: Don't Assume m_chain_tx_count in GuessVerificationProgress
on Sep 23, 2024

fjahr force-pushed on Sep 23, 2024

DrahtBot added the label CI failed on Sep 23, 2024

DrahtBot removed the label CI failed on Sep 23, 2024

in src/wallet/rpc/transactions.cpp:914 in 58a4622378 outdated

908@@ -909,9 +909,9 @@ RPCHelpMan rescanblockchain()
909             }
910         }
911 
912-        // We can't rescan beyond non-pruned blocks, stop and throw an error
913+        // We can't rescan unavailable blocks, stop and throw an error
914         if (!pwallet->chain().hasBlocks(pwallet->GetLastBlockHash(), start_height, stop_height)) {
915-            throw JSONRPCError(RPC_MISC_ERROR, "Can't rescan beyond pruned data. Use RPC call getblockchaininfo to determine your pruned height.");
916+            throw JSONRPCError(RPC_MISC_ERROR, "Failed to rescan unavailable blocks. This may be caused by pruning or an in-progress assumeutxo background sync. Use RPC call getblockchaininfo to determine your pruned height or check your logs for assumeutxo progress.");

maflcko commented at 9:23 am on September 24, 2024:

I wonder if it is easy to provide an accurate error message. Otherwise it could be a bit confusing for a user that had AU enabled (with 100% progress) as well as pruning, not knowing the exact cause and what they need to do to work around the error. Not sure how easy that is, so up to you.

fjahr commented at 11:43 pm on September 30, 2024:

Improving the error message is reasonably easy but I didn’t see a way without expanding the chain interface. I have implemented what I think is a step in the right direction but not a full solution to every edge case in order to keep the scope of this PR reasonable. Looking for feedback if people still think this is right approach and if I should expand it further or if adding more is going too far.

I think the assumeutxo + pruning use-case is pretty hard to deal with accurately for these wallet use-cases, especially if the pruning target is small. I am not sure if we can really give users a perfect answer in that case. Users could request a window for rescan that is larger than the prune target so I think pruning takes precedent because it requires a reindex to “fix” while AU just takes some time and resolves itself. Also, in the case of au+pruning a block could be not available because it wasn’t synced yet during the first time the user tries to import a descriptor. But then the second time the user tries the same block that wasn’t synced yet before could now be pruned already, assuming a small prune target. I mean to say, a perfect solution requires changing how the RPC works and I think that’s beyond the scope of this PR. I could think about it a bit more and open an issue on it though if that is desirable.

fjahr force-pushed on Sep 30, 2024

fjahr commented at 11:44 pm on September 30, 2024: contributor

Improved the error messages for importdescriptors and rescanblockchain to better match the reality (usage of pruning and assumeutxo).

fjahr renamed this:
~~wallet, assumeutxo: Don't Assume m_chain_tx_count in GuessVerificationProgress~~
wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors
on Sep 30, 2024

DrahtBot added the label Needs rebase on Oct 28, 2024

in src/node/interfaces.cpp:775 in 589b849c5f outdated

769@@ -769,6 +770,16 @@ class ChainImpl : public Chain
770         LOCK(::cs_main);
771         return chainman().m_blockman.m_have_pruned;
772     }
773+    std::optional<int> getPruneHeight() override
774+    {
775+        LOCK(::cs_main);

maflcko commented at 7:51 am on October 29, 2024:

nit: For new code it would be good to use the alias chainman().GetMutex() member instead, when chainman() members are accessed.

fjahr commented at 4:59 pm on November 5, 2024:

done

mzumsande commented at 10:08 pm on January 3, 2025:

this went into the wrong commit (2nd instead of 1st)

fjahr commented at 4:40 pm on January 5, 2025:

fixed

fjahr force-pushed on Nov 5, 2024

fjahr commented at 4:59 pm on November 5, 2024: contributor

Rebased and addressed nit from @maflcko

DrahtBot removed the label Needs rebase on Nov 5, 2024

DrahtBot added the label CI failed on Dec 10, 2024

DrahtBot removed the label CI failed on Dec 16, 2024

DrahtBot added the label Needs rebase on Dec 30, 2024

fjahr force-pushed on Jan 1, 2025

DrahtBot removed the label Needs rebase on Jan 1, 2025

in src/interfaces/chain.h:293 in 0230ecafad outdated

288@@ -289,6 +289,12 @@ class Chain
289     //! Check if any block has been pruned.
290     virtual bool havePruned() = 0;
291 
292+    //! Get the current prune height.
293+    virtual std::optional<int> getPruneHeight() = 0;

mzumsande commented at 10:23 pm on January 3, 2025:

did you consider using the existing hasBlocks() function instead of adding this function? (I didn’t check if it works, but at a glance it looks like it might).

The reason why I looked for other functions is because I wasn’t sure if it’s good to expose RPC functions such as GetPruneHeight() through the chain interface, so that non-RPC code could use it too.

fjahr commented at 4:40 pm on January 5, 2025:

hasBlocks() is already used one line above where I use getPruneHeight(). The problem is it doesn’t tell us if we have been pruning or not.

Is there a concerns with exposing GetPruneHeight() explicitly? I think the function could be moved outside of RPC code, it just wasn’t needed anywhere else so far.

mzumsande commented at 4:08 pm on January 6, 2025:

Makes sense, I got confused there! It just seemed a bit unusual, plus we generally use CHECK_NONFATAL instead of assert in rpc code, which may not be the ideal choice for possible future interface users (though it is currently just used in another rpc call, so it’s not an actual problem). But I’m not an expert on the design of our interfaces, so I don’t know if moving it somewhere else is necessary.

in src/interfaces/chain.h:296 in 0230ecafad outdated

288@@ -289,6 +289,12 @@ class Chain
289     //! Check if any block has been pruned.
290     virtual bool havePruned() = 0;
291 
292+    //! Get the current prune height.
293+    virtual std::optional<int> getPruneHeight() = 0;
294+
295+    //! Check if a snapshot chainstate exists.
296+    virtual bool haveSnapshotChainstate() = 0;

mzumsande commented at 10:26 pm on January 3, 2025:

similar here: could you use the existing hasAssumedValidChain?

fjahr commented at 4:41 pm on January 5, 2025:

Yeah, this works, I replaced it. Thanks!

mzumsande commented at 0:01 am on January 4, 2025: contributor

Concept ACK

interfaces: Add helper function for wallet on pruning 42d5d53363

validation: Don't assume m_chain_tx_count in GuessVerificationProgress

In the context of an a descriptor import during assumeutxo background sync, the progress can not be estimated due to m_chain_tx_count being set to 0.

27f99b6d63

rpc: Improve importdescriptor RPC error messages

Particularly add more details in the case of pruning or assumeutxo.

d73ae603d4

test, assumeutxo: import descriptors during background sync 595edee169

rpc: Include assumeutxo as a failure reason of rescanblockchain 9d2d9f7ce2

fjahr force-pushed on Jan 5, 2025

fjahr commented at 4:41 pm on January 5, 2025: contributor

Addressed feedback from @mzumsande

mzumsande commented at 4:22 pm on January 6, 2025: contributor

Code Review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727

DrahtBot requested review from alfonsoromanz on Jan 6, 2025

in src/validation.cpp:5627 in 27f99b6d63 outdated

5622@@ -5623,9 +5623,8 @@ double ChainstateManager::GuessVerificationProgress(const CBlockIndex* pindex) c
5623         return 0.0;
5624     }
5625 
5626-    if (!Assume(pindex->m_chain_tx_count > 0)) {
5627-        LogWarning("Internal bug detected: block %d has unset m_chain_tx_count (%s %s). Please report this issue here: %s\n",
5628-                   pindex->nHeight, CLIENT_NAME, FormatFullVersion(), CLIENT_BUGREPORT);
5629+    if (pindex->m_chain_tx_count == 0) {
5630+        LogDebug(BCLog::VALIDATION, "Block %d has unset m_chain_tx_count. Unable to estimate verification progress.\n", pindex->nHeight);

furszy commented at 9:14 pm on January 8, 2025:

tiny nit: the validation category is a bit odd in the wallet scan context or GUI updates.

furszy commented at 9:19 pm on January 8, 2025: member

Code review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727

achow101 commented at 8:37 pm on January 31, 2025: member

ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727

achow101 merged this on Jan 31, 2025

achow101 closed this on Jan 31, 2025

bitcoin deleted a comment on Feb 1, 2025

TheCharlatan referenced this in commit e1405325c9 on Feb 3, 2025

Armss9936 approved

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

Code Coverage & Benchmarks

Reviews

Conflicts