rpc: Do not wait for headers inside loadtxoutset

[maflcko]

While the loadtxoutset default 10 minute timeout is convenient when it is sufficient, it may cause hassle where it is not. For example:

• When P2P connections are missing, it seems better to abort early than wait for the timeout.
• When the 10 minute timeout is not sufficient, the RPC will have to be called again, so a check or loop is needed outside the RPC either way. So might as well remove the loop inside the RPC.

[DrahtBot]

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	fjahr, theStack, pablomartin4btc, TheCharlatan

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

[theStack]

Concept ACK

I tend to agree that it's best to remove the waiting loop and have instant feedback about missing headers instead, as I think that avoids frustration and/or confusion for users. Curious about reasonable arguments to keep it though (maybe I'm missing something).

[pablomartin4btc]

Concept ACK

I agree with this change and @theStack's comment; one case I can think of would be a "ready to go" installation package or something like that? But as @maflcko said in the description, the procedure/ RPC call would need to be re-executed again, so not too sure about it really.

edit: I found here in the original PR #27596 a discussion about the waiting/ timeout.

[maflcko]

Curious about reasonable arguments to keep it though

The only reason I can see to keep it, is when it is made infinite, and the RPC is made async.

[pablomartin4btc]

Curious about reasonable arguments to keep it though

The only reason I can see to keep it, is when it is made infinite, and the RPC is made async.

You might consider this coming then #28620 (make loadtxoutset async), the linked fix could be updated soon I think.

[jamesob]

Looks fine to me.

[fjahr]

ACK faa30a4

Thanks, as @pablomartin4btc found, I had a slight preference for this simpler solution when reviewing the original PR already.

I wrote a test for this (and also removed a sync_block call that seemed unnecessary in the process), can be pulled in here or I can open it as a follow-up: https://github.com/fjahr/bitcoin/commits/pr29345/

[theStack]

Code-review ACK faa30a4

[fjahr]

I wrote a test for this (and also removed a sync_block call that seemed unnecessary in the process), can be pulled in here or I can open it as a follow-up: https://github.com/fjahr/bitcoin/commits/pr29345/

I have opened a separate PR since this already has multiple ACKs and I might forget to open it later: #29478

[pablomartin4btc]

tACK faa30a4

Testing the script on both master and this PR, it seems the execution gets stuck (at least for me) at the last validation - snapshot chain reloads -, independently from this change, and the following line would need to be removed, so the client node can be started:

bitcoin/contrib/devtools/test_utxo_snapshots.sh

Line 199 in 1ac627c

client_sleep_til_boot

I think when the watch command sub-process gets stopped with CTRL+C (as instructed to the user when validation gets completed), both bitcoind server and client get shutdown (I can see in their debug logs on the other 2 terminals I open following the script instructions during its execution).

I tested the script removing the above line #L199 and validated that works fine. Also, the finish() function would not kill the server node which got shutdown during the CTRL+C, unless we still need to provide blocks to the client node, we need to start it up again as well.

diff --git a/contrib/devtools/test_utxo_snapshots.sh b/contrib/devtools/test_utxo_snapshots.sh
index 93a4cd1683..cd34197cf5 100755
--- a/contrib/devtools/test_utxo_snapshots.sh
+++ b/contrib/devtools/test_utxo_snapshots.sh
@@ -196,7 +196,12 @@ echo "   Press CTRL+C after you're satisfied to exit the demo"
 echo
 read -p "Press [enter] to continue"
 
-client_sleep_til_boot
+# shellcheck disable=SC2086
+./src/bitcoind $SERVER_PORTS -logthreadnames=1 -debug=net -datadir="$SERVER_DATADIR" \
+    $EARLY_IBD_FLAGS -connect=0 -listen=1 >/dev/null &
+SERVER_PID="$!"
+server_sleep_til_boot
+
 # shellcheck disable=SC2086
 ./src/bitcoind $CLIENT_PORTS $ALL_INDEXES -logthreadnames=1 -datadir="$CLIENT_DATADIR" -connect=0 \
     -addnode=127.0.0.1:$SERVER_PORT "$EARLY_IBD_FLAGS" >/dev/null &

[TheCharlatan]

ACK faa30a4

[maflcko]

Testing the script on both master and this PR, it seems the execution gets stuck (at least for me) at the last validation - snapshot chain reloads -, independently from this change, and the following line would need to be removed, so the client node can be started:

I wonder if there is a need to have a bash script to serve as documentation on how to create and load snapshots. Maybe the existing python tests are enough, or a shorter markdown document can be written, with the important key settings and steps highlighted?

[pablomartin4btc]

I wonder if there is a need to have a bash script to serve as documentation on how to create and load snapshots.

Well, IMPO since it's already there I would keep it updated, it's a different exercise, you need to touch de code and re-compile in order the snapshot to work, you could hit different issues perhaps.

or a shorter markdown document can be written, with the important key settings and steps highlighted?

I think, and looking at the current state of the doc, it could be improved adding some command output references and perhaps to some parts of this bash script.