rpc: Return errors in loadtxoutset that currently go to logs

maflcko commented at 10:26 am on July 22, 2024: member

The error messages should never happen in normal operation. However, if they do, they are helpful to return to the user to debug the issue. For example, to notice a truncated file.

This fixes #28621

Also includes a minor refactor commit.

DrahtBot commented at 10:26 am on July 22, 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	ryanofsky, fjahr
Concept ACK	Sjors
Stale ACK	danielabrozzoni

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:

#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

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 renamed this:
~~rpc: Return precise loadtxoutset error messages~~
rpc: Return precise loadtxoutset error messages
on Jul 22, 2024

DrahtBot added the label RPC/REST/ZMQ on Jul 22, 2024

DrahtBot commented at 11:41 am on July 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27742334614

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Jul 22, 2024

DrahtBot removed the label CI failed on Jul 22, 2024

fjahr commented at 2:55 pm on July 22, 2024: contributor

Concept ACK

I think the title is a bit confusing here. It sounds like the error message content changes but they are just moved from logs to RPC. I think the title of the the issue that is fixed is a better description.

maflcko renamed this:
~~rpc: Return precise loadtxoutset error messages~~
rpc: Return errors in loadtxoutset that currently go to logs
on Jul 22, 2024

DrahtBot renamed this:
~~rpc: Return errors in loadtxoutset that currently go to logs~~
rpc: Return errors in loadtxoutset that currently go to logs
on Jul 22, 2024

maflcko commented at 3:09 pm on July 22, 2024: member

I think the title is a bit confusing here. It sounds like the error message content changes but they are just moved from logs to RPC. I think the title of the the issue that is fixed is a better description.

Good point, fixed!

danielabrozzoni commented at 2:44 pm on July 23, 2024: contributor

ACK fa5c253b4e9e416a6ff729365cb66e13ac0f66e7 - code looks good to me, tests are passing locally, but I didn’t do any manual testing.

DrahtBot requested review from fjahr on Jul 23, 2024

maflcko requested review from Sjors on Jul 23, 2024

Sjors commented at 4:51 pm on July 23, 2024: member

Concept ACK

DrahtBot added the label Needs rebase on Jul 23, 2024

refactor: Use untranslated error message in ActivateSnapshot

The message is not exposed in the GUI or another translated context, so
translating it is useless for now.

Also, fix a nit from https://github.com/bitcoin/bitcoin/pull/30395#discussion_r1670972864

faa5c86dbf

maflcko force-pushed on Jul 25, 2024

DrahtBot removed the label Needs rebase on Jul 25, 2024

maflcko commented at 12:33 pm on July 25, 2024: member

Rebased. Should be trivial to re-ACK with git range-diff bitcoin-core/master fa5c253b4e fa91404c68

danielabrozzoni commented at 4:12 pm on July 25, 2024: contributor

re-ACK fa91404c68b4d503e6568f49aaf74a5746887185

in src/validation.cpp:5912 in fa91404c68 outdated

5908@@ -5915,7 +5909,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
5909                 // means <5MB of memory imprecision.
5910                 if (coins_processed % 120000 == 0) {
5911                     if (m_interrupt) {
5912-                        return false;
5913+                        return util::Error{};

ryanofsky commented at 6:38 pm on July 25, 2024:

In commit “rpc: Return precise loadtxoutset error messages” (fa91404c68b4d503e6568f49aaf74a5746887185)

This is ok, but IMO it would be better to change result type from Result<void> to Result<InterruptResult> and return Interrupted{} here. If a function exits early because the user requested a shutdown, it may or may not be an error, and bubbling up InterruptResult lets the top level caller decide what to do based on context.

For example, in this case because the call is being made by RPC it would probably be good for the RPC call to return an “Snapshot loading was aborted” error. But in the GUI case, if a user tries to load snapshot and then shuts down before it is loaded, it would probably be better to just shut down cleanly and maybe log something, than try to present an error message.

In either case, returning something like return util::Error{Untranslated("Processing snapshot aborted")} or return Interrupted{} would be better than returning an error with no message.

maflcko commented at 12:11 pm on July 26, 2024:

it would probably be better to just shut down cleanly and maybe log something, than try to present an error message.

An alternative may be to ask Really shutdown and discard the loadutxoset progress? [Cancel] [Shutdown].

But the GUI design can be done later, so I’ll just clarify the error message here for the RPC.

ryanofsky commented at 12:54 pm on July 26, 2024:

An alternative may be to ask Really shutdown and discard the loadutxoset progress? [Cancel] [Shutdown].

Yes exactly, that is the point of returning Interrupted status, because there are many ways caller may want to handle an interruption, and it should be up to the caller to decide. The behavior shouldn’t be hardcoded into the function that was interrupted.

But the GUI design can be done later, so I’ll just clarify the error message here for the RPC.

That would be better than current version, but I wonder why you don’t want to return interrupted status to make intent of the code clear and avoid needing to revisit this and change it later?

maflcko commented at 1:38 pm on July 26, 2024:

An alternative may be to ask Really shutdown and discard the loadutxoset progress? [Cancel] [Shutdown].

Yes exactly, that is the point of returning Interrupted status, because there are many ways caller may want to handle an interruption, and it should be up to the caller to decide. The behavior shouldn’t be hardcoded into the function that was interrupted.

Your suggestion doesn’t allow my suggestion. They are completely separate, because one is about cancelling the shutdown, before it happens, and the other is about returning a shutdown happened in loadtxoutset (after it happened).

I wonder why you don’t want to return interrupted status to make intent of the code clear and avoid needing to revisit this and change it later?

I think the intent of the code is fully clear with return util::Error{Untranslated("Aborting after an interrupt was requested")}; (If not, happy to push an alternative)

In any case, I think Result<InterruptResult> would be wrong and would have to be revisited later on anyway:

Wrapping one result into another is fragile, because call-sites may easily forget to unwrap the inner result, especially if the inner result is otherwise void. I’ve shared a diff in #30516 (review) to make the inner result non-void. However, I don’t want to block or wait this pull on other stuff.
It is not clear how soon the feature will be implemented in the GUI. Clearly, a 8-line patch should not be an additional hurdle to implement it in the GUI later on. However, if it is never implemented in the GUI, I don’t consider the patch worth to do (even if it is small).
Personally, I could also see how the interrupt state could be a failure value (https://github.com/bitcoin/bitcoin/pull/25665), to avoid the fragility mentioned in the first point. But again, I don’t want to block or wait this here on other stuff.

Overall I think it is not clear (at least to me) that Result<InterruptResult> is the way to go. Given the uncertainty it seems better to hold back on it for now. But that may just be me.

ryanofsky commented at 2:22 pm on July 26, 2024:

re: #30497 (review)

Overall I think it is not clear (at least to me) that Result<InterruptResult> is the way to go. Given the uncertainty it seems better to hold back on it for now. But that may just be me.

Great, thanks for explaining. You may want to add feedback on #29700 which makes extensive use of InterruptResult.

I think Result would be wrong

It sounds like maybe you would want to this to Result<void, InterruptResult> instead of Result<InterruptResult, void>. I think that would be reasonable and could have advantages. I didn’t opt for that approach because return Interrupted{} is shorter and seems more appropriate than return {Error{}, Interrupted{}}, because I wouldn’t consider a successful interruption to be an error. But maybe it makes sense to support a util::Result constructor for failures that are not necessarily errors and allow return Interrupted{} syntax for Result<void, InterruptResult> types. I could experiment with that.

ryanofsky commented at 2:41 pm on July 26, 2024:

Your suggestion doesn’t allow my suggestion. They are completely separate, because one is about cancelling the shutdown, before it happens, and the other is about returning a shutdown happened in loadtxoutset (after it happened).

Forgot to respond to this part. We would still want to return Interrupted in this case. To implement your suggestion the GUI would make an asynchronous ActivateSnapshot call when the user loaded a snapshot file, and show the [Cancel] [Shutdown] dialog if there was an attempt to shut down while the call was progress. The question is just what should ActivateSnapshot return after the user clicked [Shutdown], and Interupted{} makes more than util::Error{Untranslated("Aborting after an interrupt was requested")} to avoid triggering an error log or dialog when no error happened and the software did exactly what the user requested.

in src/validation.cpp:5975 in fa91404c68 outdated

5971@@ -5980,18 +5972,16 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
5972         maybe_stats = ComputeUTXOStats(
5973             CoinStatsHashType::HASH_SERIALIZED, snapshot_coinsdb, m_blockman, [&interrupt = m_interrupt] { SnapshotUTXOHashBreakpoint(interrupt); });
5974     } catch (StopHashingException const&) {
5975-        return false;
5976+        return util::Error{};

ryanofsky commented at 6:41 pm on July 25, 2024:

In commit “rpc: Return precise loadtxoutset error messages” (fa91404c68b4d503e6568f49aaf74a5746887185)

Same comment about InterruptResult here

in src/validation.cpp:5858 in fa91404c68 outdated

5854@@ -5857,8 +5855,7 @@ bool ChainstateManager::PopulateAndValidateSnapshot(
5855     // ActivateSnapshot(), but is done so that we avoid doing the long work of staging
5856     // a snapshot that isn't actually usable.
5857     if (WITH_LOCK(::cs_main, return !CBlockIndexWorkComparator()(ActiveTip(), snapshot_start_block))) {
5858-        LogPrintf("[snapshot] activation failed - work does not exceed active chainstate\n");
5859-        return false;
5860+        return util::Error{Untranslated("Activation failed - work does not exceed active chainstate")};

ryanofsky commented at 6:44 pm on July 25, 2024:

In commit “rpc: Return precise loadtxoutset error messages” (fa91404c68b4d503e6568f49aaf74a5746887185)

The error message seems inconsistent with other ones because it is prefixed with “Activation failed -”. Maybe drop that prefix or also include it in the other messages?

ryanofsky approved

ryanofsky commented at 6:47 pm on July 25, 2024: contributor

Code review ACK fa91404c68b4d503e6568f49aaf74a5746887185. Nice change which should make RPC errors easier to understand.

rpc: Return precise loadtxoutset error messages

The error messages should never happen in normal operation. However, if
they do, they are helpful to return to the user to debug the issue. For
example, to notice a truncated file.

fa530ec543

maflcko force-pushed on Jul 26, 2024

maflcko commented at 1:39 pm on July 26, 2024: member

Force pushed to adjust three error messages slightly. Should be easy to re-ACK

ryanofsky approved

ryanofsky commented at 2:26 pm on July 26, 2024: contributor

Code review ACK fa530ec54386a3fd56b51e50699b424cc8647821, just adjusting error messages a little since last review. (Thanks!)

DrahtBot requested review from danielabrozzoni on Jul 26, 2024

fjahr commented at 4:45 pm on July 30, 2024: contributor

Code review ACK fa530ec54386a3fd56b51e50699b424cc8647821

maflcko commented at 2:04 pm on August 5, 2024: member

rfm, or does it need more review?

ryanofsky commented at 4:38 pm on August 5, 2024: contributor

rfm, or does it need more review?

Looks good to me. I’d tend to wait for 3 current acks on a change to validation.cpp, but in this case there are two current acks and only string changes since that last stale ack so this seem close enough. Will merge soon.

ryanofsky merged this on Aug 5, 2024

ryanofsky closed this on Aug 5, 2024

maflcko deleted the branch on Aug 7, 2024

rpc: Return errors in loadtxoutset that currently go to logs #30497

Code Coverage

Reviews

Conflicts