log: Fix log message for -par=1 #17325

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:20191030-fix-par-log changing 1 files +1 −1

hebasto commented at 8:30 PM on October 30, 2019: member

Fix #17139
practicalswift commented at 8:43 PM on October 30, 2019: contributor

Concept ACK
in src/init.cpp:1264 in 3ee5d33775 outdated
```
1260 |      if (nScriptCheckThreads) {
1261 | +        LogPrintf("Using %u threads concurrently for script verification\n", nScriptCheckThreads);
1262 |          for (int i=0; i<nScriptCheckThreads-1; i++)
1263 |              threadGroup.create_thread([i]() { return ThreadScriptCheck(i); });
1264 | +    } else {
1265 | +        LogPrintf("Using the only thread for script verification\n");
```
laanwj commented at 8:44 PM on October 30, 2019:

Concept ACK, my only comment would be ~~that -par=0 and -par=1 do the same thing (right?)~~, but log different messages.

hebasto commented at 8:50 PM on October 30, 2019:

Not exactly: https://github.com/bitcoin/bitcoin/blob/08e29473126d5cc4df6d2b3f368c6f6f641c0bd8/src/init.cpp#L380

laanwj commented at 8:19 AM on October 31, 2019:

Wait, I'm not really talking about the input argument, but the already-processed value we end up with here.

It's doing the same thing in case of nScriptCheckThreads==0 and nScriptCheckThreads==1 so should log the same thing.

hebasto commented at 9:58 AM on October 31, 2019:

After this code https://github.com/bitcoin/bitcoin/blob/08e29473126d5cc4df6d2b3f368c6f6f641c0bd8/src/init.cpp#L1064-L1071

is executed in AppInitParameterInteraction(), nScriptCheckThreads==1 is an impossible value.

laanwj commented at 11:33 AM on October 31, 2019:

Ok, so it will never log Using 1 threads concurrently for script verification?

hebasto commented at 11:37 AM on October 31, 2019:

Sure.

practicalswift commented at 12:09 PM on October 31, 2019:

Something feels a bit off with the wording here. What about "Using one thread for script verification"?

laanwj commented at 12:15 PM on October 31, 2019:

It's not just any thread though, it's the main thread. No additional threads are spawned. I think that's what he's trying to convey.

practicalswift commented at 12:28 PM on October 31, 2019:

What about:

"Validation concurrency disabled: using the main thread for script validation."

hebasto commented at 12:38 PM on October 31, 2019:

Isn't this the thread: https://github.com/bitcoin/bitcoin/blob/feb1a8c03aff0d37730d80eee7a05cc7e4729850/src/init.cpp#L1267 ?

laanwj commented at 12:48 PM on October 31, 2019:

That spawns the scheduler thread, which is unrelated to script verification.
MarcoFalke commented at 2:33 PM on October 31, 2019:

On -par=1, it will inline the calls into the current thread. See also:

git grep '\-par=' test
MarcoFalke commented at 2:35 PM on October 31, 2019:

LogPrintf("Script verification is done in the main thread\n");
laanwj commented at 3:41 PM on October 31, 2019:

But script verification is always done in the main thread (as well), the other threads are only additional :smile:

practicalswift commented at 3:55 PM on October 31, 2019:

"Script verification is done in the main thread only (no validation concurrency)"

laanwj commented at 3:59 PM on October 31, 2019:

or "Script verification uses N additional threads" where N can be 0… it doesn't have to be a whole story

MarcoFalke commented at 4:07 PM on October 31, 2019:

Yeah, let's make it a one-line "fix"

hebasto commented at 4:17 PM on October 31, 2019:

Yeah, let's make it a one-line "fix"

Not sure about "one-line fix" as actual additional threads number is nScriptCheckThreads-1.
laanwj commented at 4:35 PM on October 31, 2019:

shortest would be

LogPrintf("Script verification uses %d additional threads\m", std::max(nScriptCheckThreads-1, 0));
hebasto commented at 4:52 PM on October 31, 2019:

True, it is shorter than conditional operator ;)
jnewbery commented at 9:42 PM on October 31, 2019:

~I don't think this should be nScriptCheckThreads-1. nScriptCheckThreads is the number of threads that are dedicated to script checking, so:~

~if nScriptCheckThreads >= 2, then log "Script verification uses {nScriptCheckThreads} additional threads"~

~if nScriptCheckThreads == 0, then script checking is done on the main thread so log "Script verification uses 0 additional threads"~

~nScriptCheckThreads cannot == 1 because of the logic in init.cpp~

~so you just want:~

~LogPrintf("Script verification uses %d dedicated threads\m", nScriptCheckThreads);~

~The log is already essentially correct. I think you just need to add the word 'dedicated' or 'additional' (I prefer 'dedicated').~ @sipa has corrected my misunderstanding below.
sipa commented at 10:16 PM on October 31, 2019:

That's not correct; it should be LogPrintf("Script verification uses %d dedicated threads\m", nScriptCheckThreads - 1); then, as nScriptCheckThreads includes the main thread.

laanwj commented at 9:48 AM on November 1, 2019:

@sipa The std::max(nScriptCheckThreads-1, 0) bound is there because nScriptCheckThreads might be zero at this point.
fanquake added the label Utils/log/libs on Oct 30, 2019
hebasto force-pushed on Oct 31, 2019

log: Fix log message for -par=1

Co-authored-by: Wladimir J. van der Laan <laanwj@protonmail.com>

8a0ca5e9de

hebasto force-pushed on Oct 31, 2019
hebasto commented at 5:01 PM on October 31, 2019: member

All comments have been addressed.
jnewbery commented at 10:45 PM on October 31, 2019: member

Tested ACK 8a0ca5e9de021ce2f722b321f27208a7bc7110f7

If we wanted to be completely explicit we could log "Script verification uses main thread and %d dedicated threads\n"
laanwj commented at 9:47 AM on November 1, 2019: member

ACK 8a0ca5e9de021ce2f722b321f27208a7bc7110f7

If we wanted to be completely explicit we could log "Script verification uses main thread and %d dedicated threads\n"

I know bitcoin core is lacking in user documentation, but I don't think the debug log is the place for it :smile: The value is now correct, which is good for troubleshooting.
laanwj referenced this in commit 462fbcd212 on Nov 1, 2019
laanwj merged this on Nov 1, 2019
laanwj closed this on Nov 1, 2019
hebasto deleted the branch on Nov 1, 2019
sidhujag referenced this in commit d304bb9664 on Nov 1, 2019
MarkLTZ referenced this in commit 8823f65197 on Apr 7, 2020
MarkLTZ referenced this in commit 0a1571d657 on Apr 7, 2020
sidhujag referenced this in commit 9b70d9e0ff on Nov 10, 2020
PastaPastaPasta referenced this in commit 213c668e2f on Jun 27, 2021
PastaPastaPasta referenced this in commit 7bc7dca5cb on Jun 28, 2021
PastaPastaPasta referenced this in commit ceffbae448 on Jun 29, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#17139 log, doc: Confusing log message