rpc: Use untranslated error strings in loadtxoutset #30395

maflcko · 2024-07-05T08:17:11Z

Motivation:

Some are not translated at all, anyway. See assumeutxo: Check snapshot base block is not in invalid chain #30267 (comment)
For others translation is not yet needed, because they are not called by the GUI (yet)
For others translations will never be needed, because they are RPC code. See assumeutxo: Check snapshot base block is not in invalid chain #30267 (comment)

Also, while touching this:

Remove the trailing \n. See assumeutxo: Check snapshot base block is not in invalid chain #30267 (comment)
Add back the path. See assumeutxo: Check snapshot base block is not in invalid chain #30267 (comment)
Use named args to get the path.

DrahtBot · 2024-07-05T08:17: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	tdb3, fjahr, ryanofsky
Concept ACK	TheCharlatan

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:

#30383 (util: Catch translation string errors at compile time 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.

TheCharlatan · 2024-07-05T08:20:39Z

Concept ACK

DrahtBot · 2024-07-05T09:00:35Z

🚧 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/27074813459}

fjahr · 2024-07-05T13:12:55Z

utACK fa22edc

tdb3

Approach ACK

tdb3 · 2024-07-05T15:49:07Z

src/rpc/blockchain.cpp

@@ -2833,7 +2833,7 @@ static RPCHelpMan loadtxoutset()

    auto activation_result{chainman.ActivateSnapshot(afile, metadata, false)};
    if (!activation_result) {
-        throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf(_("Unable to load UTXO snapshot: %s\n"), util::ErrorString(activation_result)).original);
+        throw JSONRPCError(RPC_INTERNAL_ERROR, strprintf("Unable to load UTXO snapshot: %s. (%s)", util::ErrorString(activation_result).original, path.utf8string()));


nit: It looks like there might be an extraneous period for at least some error messages (between the error message and the parenthesis-wrapped path).

Created a snapshot with dumptxoutset.
Modified a byte in the snapshot to induce failure during load.

$ src/bitcoin-cli loadtxoutset mytxoutset.dat error code: -32603 error message: Unable to load UTXO snapshot: assumeutxo block hash in snapshot metadata not recognized (hash: 767fccc851802c0180a3ebf1631ab978d4d6a5ae1d452dc26e9c177850708e6c, height: 101). The following snapshot heights are available: 110, 299.. (/home/dev/.bitcoin/regtest/mytxoutset.dat)

Thx, removed ..

tdb3

ACK fa5b892

Re-ran check in #30395 (comment) . No extra period seen.
Ran unit/functional tests locally, including feature_assumeutxo (all passed).

fjahr · 2024-07-05T19:04:46Z

re-ACK fa5b892

ryanofsky

Code review ACK fa5b892

ryanofsky · 2024-07-09T18:16:40Z

src/validation.cpp

@@ -5732,7 +5732,7 @@ util::Result<void> ChainstateManager::ActivateSnapshot(
            static_cast<size_t>(current_coinstip_cache_size * SNAPSHOT_CACHE_PERC));
    }

-    auto cleanup_bad_snapshot = [&](const char* reason) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {
+    auto cleanup_bad_snapshot = [&](bilingual_str&& reason) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) {


In commit "rpc: Use untranslated error strings in loadtxoutset" (fa5b892)

Not important, but you could change this from bilingual_str&& to just bilingual_str so it is simpler, and function can be called naturally with both lvalues and rvalues, not just rvalues. Rvalues will be moved in any case.

hebasto

Post-merge ACK fa5b892.

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)

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/bitcoin#30395 (comment)

Summary: > rpc: Fix race in loadtxoutset > > The tip may have advanced, also if it did not, there is no reason to > have two variables point to the same block. bitcoin/bitcoin@fa91089 The potential race just means that tip_hash in the RPC response could be different from "the hash of the base of the snapshot" (as documented in the RPC doc). Note that this race cannot really happen in the functional test, as there are no blocks coming over the network during the loadtxoutset call (blocks are mined locally). It could have happened in the wild, with very limited impact. The node would keep working fine, but some users may have been confused by the RPC response. > refactor: Move early loadtxoutset checks into ActiveSnapshot > > Also changes the return type of ActiveSnapshot to allow returning the > error message to the user of the loadtxoutset RPC. bitcoin/bitcoin@80315c0 > refactor: Use named arguments to get path arg in loadtxoutset > > rpc: Use untranslated error strings in loadtxoutset [[bitcoin/bitcoin#30395 | core#30395]] This is a partial backport of [[bitcoin/bitcoin#29262 | core#29262]], [[bitcoin/bitcoin#30267 | core#30267]] and [[bitcoin/bitcoin#30395 | core#30395]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18055

DrahtBot changed the title ~~rpc: Use untranslated error strings in loadtxoutset~~ rpc: Use untranslated error strings in loadtxoutset Jul 5, 2024

DrahtBot added the RPC/REST/ZMQ label Jul 5, 2024

maflcko force-pushed the 2407-rpc-loadtxoutset-msgs branch from fa7f58e to fa55ada Compare July 5, 2024 08:23

bitcoin deleted a comment from HerWayBit Jul 5, 2024

maflcko force-pushed the 2407-rpc-loadtxoutset-msgs branch from fa55ada to fa79edb Compare July 5, 2024 09:00

DrahtBot added the CI failed label Jul 5, 2024

refactor: Use named arguments to get path arg in loadtxoutset

fa45865

maflcko force-pushed the 2407-rpc-loadtxoutset-msgs branch from fa79edb to fa22edc Compare July 5, 2024 09:07

DrahtBot removed the CI failed label Jul 5, 2024

DrahtBot requested a review from TheCharlatan July 5, 2024 13:12

tdb3 reviewed Jul 5, 2024

View reviewed changes

rpc: Use untranslated error strings in loadtxoutset

fa5b892

maflcko force-pushed the 2407-rpc-loadtxoutset-msgs branch from fa22edc to fa5b892 Compare July 5, 2024 15:56

DrahtBot mentioned this pull request Jul 5, 2024

util: Catch translation string errors at compile time #30383

Merged

tdb3 approved these changes Jul 5, 2024

View reviewed changes

DrahtBot requested a review from fjahr July 5, 2024 16:12

fanquake mentioned this pull request Jul 9, 2024

assumeutxo: Remove translated strings added in 30267 #30417

Closed

ryanofsky approved these changes Jul 9, 2024

View reviewed changes

ryanofsky merged commit c06b376 into bitcoin:master Jul 9, 2024

maflcko deleted the 2407-rpc-loadtxoutset-msgs branch July 10, 2024 07:38

hebasto reviewed Jul 10, 2024

View reviewed changes

ryanofsky mentioned this pull request Jul 19, 2024

refactor: Improve assumeutxo state representation #30214

Open

bitcoin locked and limited conversation to collaborators Jul 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rpc: Use untranslated error strings in loadtxoutset #30395

rpc: Use untranslated error strings in loadtxoutset #30395

Uh oh!

maflcko commented Jul 5, 2024

Uh oh!

DrahtBot commented Jul 5, 2024 •

edited

Loading

Uh oh!

TheCharlatan commented Jul 5, 2024

Uh oh!

DrahtBot commented Jul 5, 2024

Uh oh!

fjahr commented Jul 5, 2024

Uh oh!

tdb3 left a comment

Uh oh!

tdb3 Jul 5, 2024

Uh oh!

maflcko Jul 5, 2024

Uh oh!

tdb3 left a comment

Uh oh!

fjahr commented Jul 5, 2024

Uh oh!

ryanofsky left a comment

Uh oh!

ryanofsky Jul 9, 2024

Uh oh!

hebasto left a comment

Uh oh!

Uh oh!

rpc: Use untranslated error strings in loadtxoutset #30395

rpc: Use untranslated error strings in loadtxoutset #30395

Uh oh!

Conversation

maflcko commented Jul 5, 2024

Uh oh!

DrahtBot commented Jul 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

TheCharlatan commented Jul 5, 2024

Uh oh!

DrahtBot commented Jul 5, 2024

Uh oh!

fjahr commented Jul 5, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Jul 5, 2024

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jul 5, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 9, 2024

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jul 5, 2024 •

edited

Loading