While the loadtxoutset default 10 minute timeout is convenient when it is sufficient, it may cause hassle where it is not. For example:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
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.
No conflicts as of last run.
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).
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.
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.
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.
ACK faa30a4c56
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/
I wrote a test for this (and also removed a
sync_blockcall 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
tACK faa30a4c566c5b720c7994c55f276352a119129f
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 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.
0diff --git a/contrib/devtools/test_utxo_snapshots.sh b/contrib/devtools/test_utxo_snapshots.sh
1index 93a4cd1683..cd34197cf5 100755
2--- a/contrib/devtools/test_utxo_snapshots.sh
3+++ b/contrib/devtools/test_utxo_snapshots.sh
4@@ -196,7 +196,12 @@ echo " Press CTRL+C after you're satisfied to exit the demo"
5 echo
6 read -p "Press [enter] to continue"
7
8-client_sleep_til_boot
9+# shellcheck disable=SC2086
10+./src/bitcoind $SERVER_PORTS -logthreadnames=1 -debug=net -datadir="$SERVER_DATADIR" \
11+ $EARLY_IBD_FLAGS -connect=0 -listen=1 >/dev/null &
12+SERVER_PID="$!"
13+server_sleep_til_boot
14+
15 # shellcheck disable=SC2086
16 ./src/bitcoind $CLIENT_PORTS $ALL_INDEXES -logthreadnames=1 -datadir="$CLIENT_DATADIR" -connect=0 \
17 -addnode=127.0.0.1:$SERVER_PORT "$EARLY_IBD_FLAGS" >/dev/null &
Testing the script on both
masterand 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?
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.