Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
ryanofsky merged 2 commits into from
Aug 5, 2024
Merged

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

ryanofsky merged 2 commits into from
Aug 5, 2024

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jul 22, 2024

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
Copy link
Contributor

DrahtBot commented Jul 22, 2024 •
edited
Loading

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 DrahtBot changed the title rpc: Return precise loadtxoutset error messages rpc: Return precise loadtxoutset error messages Jul 22, 2024
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 22, 2024

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

Hints

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.

@fjahr fjahr mentioned this pull request Jul 22, 2024
Closed
32 tasks
@fjahr
Copy link
Contributor

fjahr commented Jul 22, 2024

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 maflcko changed the title rpc: Return precise loadtxoutset error messages rpc: Return errors in loadtxoutset that currently go to logs Jul 22, 2024
@DrahtBot DrahtBot changed the title rpc: Return errors in loadtxoutset that currently go to logs rpc: Return errors in loadtxoutset that currently go to logs Jul 22, 2024
@maflcko
Copy link
Member Author

maflcko commented Jul 22, 2024

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!

This was referenced Jul 23, 2024
Merged
Draft
Draft
Merged
danielabrozzoni
danielabrozzoni reviewed Jul 23, 2024
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DrahtBot DrahtBot requested a review from fjahr July 23, 2024 14:44
@maflcko maflcko requested a review from Sjors July 23, 2024 16:12
@Sjors
Copy link
Member

Sjors commented Jul 23, 2024

Concept ACK

refactor: Use untranslated error message in ActivateSnapshot
faa5c86 
The message is not exposed in the GUI or another translated context, so
translating it is useless for now.

Also, fix a nit from bitcoin#30395 (comment)
@maflcko maflcko force-pushed the 2407-loadtxoutset-rpc-err branch from fa5c253 to fa91404 Compare July 25, 2024 11:27
@maflcko
Copy link
Member Author

maflcko commented Jul 25, 2024

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

@danielabrozzoni
Copy link
Member

danielabrozzoni commented Jul 25, 2024

re-ACK fa91404c68b4d503e6568f49aaf74a5746887185

ryanofsky
ryanofsky approved these changes Jul 25, 2024
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

rpc: Return precise loadtxoutset error messages
fa530ec 
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.
@maflcko maflcko force-pushed the 2407-loadtxoutset-rpc-err branch from fa91404 to fa530ec Compare July 26, 2024 12:11
@maflcko
Copy link
Member Author

maflcko commented Jul 26, 2024

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

ryanofsky
ryanofsky approved these changes Jul 26, 2024
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DrahtBot DrahtBot requested a review from danielabrozzoni July 26, 2024 14:26
@fjahr
Copy link
Contributor

fjahr commented Jul 30, 2024

Code review ACK fa530ec

@maflcko
Copy link
Member Author

maflcko commented Aug 5, 2024

rfm, or does it need more review?

@ryanofsky
Copy link
Contributor

ryanofsky commented Aug 5, 2024

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 ryanofsky merged commit 69df012 into bitcoin:master Aug 5, 2024
@maflcko maflcko deleted the 2407-loadtxoutset-rpc-err branch August 7, 2024 09:27
@bitcoin bitcoin locked and limited conversation to collaborators Aug 7, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

@fjahr fjahr Awaiting requested review from fjahr

@Sjors Sjors Awaiting requested review from Sjors

@danielabrozzoni danielabrozzoni Awaiting requested review from danielabrozzoni

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rpc: loadtxoutset should return errors that currently go to logs

6 participants

@maflcko @DrahtBot @fjahr @Sjors @danielabrozzoni @ryanofsky