assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset #28670

pablomartin4btc · 2023-10-18T03:59:10Z

Improve error messaging in loadtxoutset to provide a more user-friendly error message when the utxo snapshot file size is insufficient to contain the necessary metadata structure (40 bytes).

Error message output before the change (current master):

./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset ${AU_DATADIR}/utxo-111.dat
error code: -1
error message:
AutoFile::read: end of file: iostream error

Error message output after the change:

./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset ${AU_DATADIR}/utxo-111.dat
error code: -32603
error message:
Unable to load UTXO snapshot, couldn't read snapshot metadata from file /tmp/.test_utxo_3/utxo-111.dat.
The file may be corrupted or it was crafted in an incompatible format.

Original strategy was to check the size of the file before de-serialisation and the current strategy was presented at that time as an alternative which has been preferred by reviewers and it looks simpler since streams source code files are no longer touched.

This PR needs to be backported to 26.x

DrahtBot · 2023-10-18T03:59:13Z

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	furszy
Approach ACK	hernanmarino
Stale 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:

#29612 (rpc: Optimize serialization and enhance metadata of dumptxoutset output by fjahr)
#27432 (contrib: add tool to convert compact-serialized UTXO set to SQLite database by theStack)

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.

src/rpc/blockchain.cpp

pablomartin4btc · 2024-01-08T23:38:20Z

Rebased + fix correct usage of utf8string() instead of u8string().

furszy

Would be good to add test coverage for this error.

Also, the "file size being too small" error message isn't really descriptive and it does not provide further guidance to solve the problem. The file could be corrupt or could had been crafted erroneously.
I would go simpler for now and change "file size being too small" for "Unable to load UTXO snapshot, the file %s cannot be parsed."

I see your alternative solution better. More comprehensive.

pablomartin4btc · 2024-01-17T18:21:28Z

Updates:

Switched the logic strategy to the previously presented alternative due to preference of reviewers @kevkevinpal & @furszy (thanks!);
Added test coverage

src/rpc/blockchain.cpp

pablomartin4btc · 2024-01-17T23:47:02Z

Addressed @furszy's feedback.

DrahtBot · 2024-01-18T00:30:34Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

_{Debug: https://github.com/bitcoin/bitcoin/runs/20595100031}

src/rpc/blockchain.cpp

pablomartin4btc · 2024-01-18T17:23:09Z

Addressed @hebasto's suggestion in order to fix the MSVC CI job.

hernanmarino

Approach ACK

fjahr · 2024-03-06T23:33:58Z

Code review ACK 53fbf19

DrahtBot · 2024-03-13T20:12:48Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

_{Debug: https://github.com/bitcoin/bitcoin/runs/20626357772}

pablomartin4btc · 2024-04-16T00:23:02Z

Updates:

I think lint CI failure is unrelated with the code change, it was running successful before started failing with no code change, run ./test/lint/lint-python.py locally with no failure.

furszy

The CI failure is due a merge conflict. Need to rebase the PR. Some of the code you are using here is not in master anymore.

pablomartin4btc · 2024-04-16T02:58:07Z

Rebased.

fjahr

re-ACK 521de52

test/functional/feature_assumeutxo.py

furszy

utACK 521de52

src/rpc/blockchain.cpp

In the RPC command `loadtxoutset`, catch possible errors while reading snapshot metadata, most commonly due to the structure size being shorter than the expected one (40 bytes) or the snapshot file may be corrupted or crafted in an incompatible format.

Add a test to cover scenarios where an EOF error is raised during the reading of invalid snapshot metadata (40 bytes file size min) or a corrupted file with an invalid format.

pablomartin4btc · 2024-05-17T02:44:01Z

Updates:

Addressed feedback from @fjahr by amending the logging info description.
Addressed feedback from @furszy by using a better RPC error code (RPC_DESERIALIZATION_ERROR instead of RPC_INTERNAL_ERROR).

furszy

utACK 4a2df05

furszy · 2024-05-20T13:02:19Z

src/rpc/blockchain.cpp

+        // Handle any exception, which includes reaching the end of the file e.g. file size < sizeOf(metadata)
+        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("Unable to load UTXO snapshot, "
+            "couldn't read snapshot metadata from file %s.\nThe file may be corrupted "
+            "or it was crafted in an incompatible format.", path.utf8string()));


nit: should use fs::PathToString(path)

PathToString

That is only for internal use. However, the result here will be returned on RPC, so the dev-notes apply:

- Use `fs::path::u8string()`/`fs::path::utf8string()` and `fs::u8path()` functions when converting path to JSON strings, not `fs::PathToString` and `fs::PathFromString` - *Rationale*: JSON strings are Unicode strings, not byte strings, and RFC8259 requires JSON to be encoded as UTF-8.

fjahr · 2024-05-21T12:29:19Z

Hm, unfortunately, I think this PR was made redundant by #29612 recently. After the latest feedback there which I addressed just 3 weeks ago, we are enhancing the assumeutxo metadata there significantly and this means all of the lines changed here are touched there as well and I don't see much of an overlap either that would let me use the code here.

pablomartin4btc · 2024-05-21T14:15:42Z

Hm, unfortunately, I think this PR was made redundant by #29612 recently. After the latest feedback there which I addressed just 3 weeks ago, we are enhancing the assumeutxo metadata there significantly and this means all of the lines changed here are touched there as well and I don't see much of an overlap either that would let me use the code here.

No prob, thanks for letting me know, I'll review that PR.

pablomartin4btc force-pushed the assumeutxo-improve-EOF-error-when-reading-snapshot-also-on-autofile branch from 65ef0d8 to acb4d35 Compare October 18, 2023 04:01

kevkevinpal reviewed Oct 20, 2023

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

DrahtBot added the CI failed label Dec 17, 2023

pablomartin4btc force-pushed the assumeutxo-improve-EOF-error-when-reading-snapshot-also-on-autofile branch 2 times, most recently from 3e013e2 to 9a56b40 Compare January 8, 2024 23:13

DrahtBot removed the CI failed label Jan 8, 2024

furszy reviewed Jan 9, 2024

View reviewed changes

DrahtBot added the CI failed label Jan 17, 2024

pablomartin4btc force-pushed the assumeutxo-improve-EOF-error-when-reading-snapshot-also-on-autofile branch 3 times, most recently from 45b005c to 2b6d44a Compare January 17, 2024 18:04

pablomartin4btc force-pushed the assumeutxo-improve-EOF-error-when-reading-snapshot-also-on-autofile branch from 54e0121 to 4d175c1 Compare January 17, 2024 18:30

DrahtBot removed the CI failed label Jan 17, 2024

furszy reviewed Jan 17, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

pablomartin4btc force-pushed the assumeutxo-improve-EOF-error-when-reading-snapshot-also-on-autofile branch from 4d175c1 to 22e4442 Compare January 17, 2024 23:44

DrahtBot added the CI failed label Jan 18, 2024

pablomartin4btc force-pushed the assumeutxo-improve-EOF-error-when-reading-snapshot-also-on-autofile branch from 22e4442 to 50c7a76 Compare January 18, 2024 02:10

DrahtBot removed the CI failed label Jan 18, 2024

hebasto reviewed Jan 18, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

pablomartin4btc force-pushed the assumeutxo-improve-EOF-error-when-reading-snapshot-also-on-autofile branch from 50c7a76 to 53fbf19 Compare January 18, 2024 17:21

willcl-ark added Docs RPC/REST/ZMQ labels Jan 24, 2024

hernanmarino approved these changes Feb 13, 2024

View reviewed changes

DrahtBot requested a review from hernanmarino March 6, 2024 23:34

fjahr mentioned this pull request Mar 10, 2024

AssumeUTXO Mainnet Readiness Tracking #29616

Open

22 tasks

DrahtBot added the CI failed label Mar 13, 2024

furszy reviewed Apr 16, 2024

View reviewed changes

pablomartin4btc force-pushed the assumeutxo-improve-EOF-error-when-reading-snapshot-also-on-autofile branch 3 times, most recently from de514b9 to 521de52 Compare April 16, 2024 02:38

DrahtBot removed the CI failed label Apr 16, 2024

fjahr reviewed Apr 16, 2024

View reviewed changes

test/functional/feature_assumeutxo.py Outdated Show resolved Hide resolved

This was referenced Apr 16, 2024

rpc: Optimize serialization and enhance metadata of dumptxoutset output #29612

Merged

contrib: add tool to convert compact-serialized UTXO set to SQLite database #27432

Open

furszy reviewed May 10, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

pablomartin4btc added 2 commits May 16, 2024 22:20

test, assumeutxo: add coverage for loadtxoutset EOF error

4a2df05

Add a test to cover scenarios where an EOF error is raised during the reading of invalid snapshot metadata (40 bytes file size min) or a corrupted file with an invalid format.

pablomartin4btc force-pushed the assumeutxo-improve-EOF-error-when-reading-snapshot-also-on-autofile branch from 521de52 to 4a2df05 Compare May 17, 2024 01:20

furszy reviewed May 20, 2024

View reviewed changes

DrahtBot requested a review from fjahr May 20, 2024 13:03

pablomartin4btc closed this May 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset #28670

assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset #28670

pablomartin4btc commented Oct 18, 2023 •

edited

DrahtBot commented Oct 18, 2023 •

edited

pablomartin4btc commented Jan 8, 2024

furszy left a comment

pablomartin4btc commented Jan 17, 2024 •

edited

pablomartin4btc commented Jan 17, 2024

DrahtBot commented Jan 18, 2024

pablomartin4btc commented Jan 18, 2024

hernanmarino left a comment

fjahr commented Mar 6, 2024

DrahtBot commented Mar 13, 2024

pablomartin4btc commented Apr 16, 2024

furszy left a comment •

edited

pablomartin4btc commented Apr 16, 2024

fjahr left a comment

furszy left a comment

pablomartin4btc commented May 17, 2024

furszy left a comment

furszy May 20, 2024

maflcko May 20, 2024

fjahr commented May 21, 2024 •

edited

pablomartin4btc commented May 21, 2024

assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset #28670

assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset #28670

Conversation

pablomartin4btc commented Oct 18, 2023 • edited

DrahtBot commented Oct 18, 2023 • edited

Code Coverage

Reviews

Conflicts

pablomartin4btc commented Jan 8, 2024

furszy left a comment

Choose a reason for hiding this comment

pablomartin4btc commented Jan 17, 2024 • edited

pablomartin4btc commented Jan 17, 2024

DrahtBot commented Jan 18, 2024

pablomartin4btc commented Jan 18, 2024

hernanmarino left a comment

Choose a reason for hiding this comment

fjahr commented Mar 6, 2024

DrahtBot commented Mar 13, 2024

pablomartin4btc commented Apr 16, 2024

furszy left a comment • edited

Choose a reason for hiding this comment

pablomartin4btc commented Apr 16, 2024

fjahr left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

pablomartin4btc commented May 17, 2024

furszy left a comment

Choose a reason for hiding this comment

furszy May 20, 2024

Choose a reason for hiding this comment

maflcko May 20, 2024

Choose a reason for hiding this comment

fjahr commented May 21, 2024 • edited

pablomartin4btc commented May 21, 2024

pablomartin4btc commented Oct 18, 2023 •

edited

DrahtBot commented Oct 18, 2023 •

edited

pablomartin4btc commented Jan 17, 2024 •

edited

furszy left a comment •

edited

fjahr commented May 21, 2024 •

edited