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

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2407-loadtxoutset-rpc-err changing 3 files +38 −49
  1. 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.

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

  3. DrahtBot renamed this:
    rpc: Return precise loadtxoutset error messages
    rpc: Return precise loadtxoutset error messages
    on Jul 22, 2024
  4. DrahtBot added the label RPC/REST/ZMQ on Jul 22, 2024
  5. 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.

  6. DrahtBot added the label CI failed on Jul 22, 2024
  7. DrahtBot removed the label CI failed on Jul 22, 2024
  8. 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.

  9. maflcko renamed this:
    rpc: Return precise loadtxoutset error messages
    rpc: Return errors in loadtxoutset that currently go to logs
    on Jul 22, 2024
  10. 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
  11. 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!

  12. 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.
  13. DrahtBot requested review from fjahr on Jul 23, 2024
  14. maflcko requested review from Sjors on Jul 23, 2024
  15. Sjors commented at 4:51 pm on July 23, 2024: member
    Concept ACK
  16. DrahtBot added the label Needs rebase on Jul 23, 2024
  17. 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
  18. maflcko force-pushed on Jul 25, 2024
  19. DrahtBot removed the label Needs rebase on Jul 25, 2024
  20. 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
  21. danielabrozzoni commented at 4:12 pm on July 25, 2024: contributor
    re-ACK fa91404c68b4d503e6568f49aaf74a5746887185
  22. 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.

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

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

  25. ryanofsky approved
  26. 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.
  27. 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
  28. maflcko force-pushed on Jul 26, 2024
  29. 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
  30. ryanofsky approved
  31. 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!)
  32. DrahtBot requested review from danielabrozzoni on Jul 26, 2024
  33. fjahr commented at 4:45 pm on July 30, 2024: contributor
    Code review ACK fa530ec54386a3fd56b51e50699b424cc8647821
  34. maflcko commented at 2:04 pm on August 5, 2024: member
    rfm, or does it need more review?
  35. 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.

  36. ryanofsky merged this on Aug 5, 2024
  37. ryanofsky closed this on Aug 5, 2024

  38. maflcko deleted the branch on Aug 7, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-18 04:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me