Add logging of CValidationInterface callbacks using a new VALIDATION log flag (see #12994). A separate flag is desirable as the logging can be noisy and thus may need to be disabled without affecting other logging.
This could help debug issues where there may be race conditions at play, such as #12978.
4@@ -5,9 +5,13 @@
5
6 #include <validationinterface.h>
7
8+#include <chain.h>
9+#include <logging.h>
10 #include <primitives/block.h>
11+#include <primitives/transaction.h>
From the Developer Notes - Source code organization:
- Every
.cppand.hfile should#includeevery header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.
- Rationale: Excluding headers because they are already indirectly included results in compilation failures when those indirect dependencies change. Furthermore, it obscures what the real code dependencies are.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
136@@ -133,47 +137,62 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
137 // the chain actually updates. One way to ensure this is for the caller to invoke this signal
138 // in the same critical section where the chain is updated
139
140- m_internals->m_schedulerClient.AddToProcessQueue([pindexNew, pindexFork, fInitialDownload, this] {
141+ auto func = __func__;
Using it directly will give the name of the lambda, i.e. operator(), which is not desirable.
Ideally, I could use __func__ in the lambda capture. However, this is not allowed because it has static storage duration. With C++14, we could use capture initializers instead.
136@@ -133,47 +137,62 @@ void CMainSignals::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockInd
137 // the chain actually updates. One way to ensure this is for the caller to invoke this signal
138 // in the same critical section where the chain is updated
139
140- m_internals->m_schedulerClient.AddToProcessQueue([pindexNew, pindexFork, fInitialDownload, this] {
141+ auto func = __func__;
142+ m_internals->m_schedulerClient.AddToProcessQueue([func, pindexNew, pindexFork, fInitialDownload, this] {
143+ if (pindexFork) LogPrint(BCLog::VALIDATIONINTERFACE, "%s: fork block hash=%s\n", func, pindexFork->GetBlockHash().ToString());
168
169 void CMainSignals::BlockConnected(const std::shared_ptr<const CBlock> &pblock, const CBlockIndex *pindex, const std::shared_ptr<const std::vector<CTransactionRef>>& pvtxConflicted) {
170- m_internals->m_schedulerClient.AddToProcessQueue([pblock, pindex, pvtxConflicted, this] {
171+ auto func = __func__;
172+ m_internals->m_schedulerClient.AddToProcessQueue([func, pblock, pindex, pvtxConflicted, this] {
173+ LogPrint(BCLog::VALIDATIONINTERFACE, "%s: block hash=%s\n", func, pblock->GetHash().ToString());
186
187 void CMainSignals::ChainStateFlushed(const CBlockLocator &locator) {
188- m_internals->m_schedulerClient.AddToProcessQueue([locator, this] {
189+ auto func = __func__;
190+ m_internals->m_schedulerClient.AddToProcessQueue([func, locator, this] {
191+ LogPrint(BCLog::VALIDATIONINTERFACE, "%s: block hash=%s\n", func, locator.IsNull() ? "null" : locator.vHave.front().ToString());
158 void CMainSignals::TransactionRemovedFromMempool(CTransactionRef ptx, MemPoolRemovalReason reason) {
159 if (reason != MemPoolRemovalReason::BLOCK && reason != MemPoolRemovalReason::CONFLICT) {
160- m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] {
161+ auto func = __func__;
162+ m_internals->m_schedulerClient.AddToProcessQueue([func, ptx, this] {
163+ LogPrint(BCLog::VALIDATIONINTERFACE, "%s: tx hash=%s\n", func, ptx->GetHash().ToString());
Tested validationinterface log flag with both cli option and logging RPC. Worked as expected.
Nice to have more logging around the validation callbacks, order of events may be so confusing!
Addressed all comments and add correct formatting for valid CValidationState in b303865.
Also updated CValidationState accessors in 3ff8755 to return strings by constant reference to avoid copying. Although this means the references are only valid for the lifetime of the object, which should be okay.
146 // in the same critical section where the chain is updated
147
148- m_internals->m_schedulerClient.AddToProcessQueue([pindexNew, pindexFork, fInitialDownload, this] {
149+ auto func = __func__;
150+ m_internals->m_schedulerClient.AddToProcessQueue([func, pindexNew, pindexFork, fInitialDownload, this] {
151+ LogPrint(BCLog::VALIDATIONINTERFACE, "%s: new block hash=%s fork block hash=%s initial download=%d\n", func,
nit: Refer to this as “IBD” or “Initial Block Download” rather than “initial download”. I suggest: "%s: new block hash=%s fork block hash=%s (In IBD=%d)\n" or similar.
(These are low-level logs. Anyone reading them should understand what IBD means.
158
159 void CMainSignals::TransactionAddedToMempool(const CTransactionRef &ptx) {
160- m_internals->m_schedulerClient.AddToProcessQueue([ptx, this] {
161+ auto func = __func__;
162+ m_internals->m_schedulerClient.AddToProcessQueue([func, ptx, this] {
163+ LogPrint(BCLog::VALIDATIONINTERFACE, "%s: tx hash=%s\n", func, ptx->GetHash().ToString());
GetWitnessHash())
1099@@ -1098,3 +1100,22 @@ void CTxMemPool::SetIsLoaded(bool loaded)
1100 }
1101
1102 SaltedTxidHasher::SaltedTxidHasher() : k0(GetRand(std::numeric_limits<uint64_t>::max())), k1(GetRand(std::numeric_limits<uint64_t>::max())) {}
1103+
1104+std::string MemPoolRemovalReasonString(const MemPoolRemovalReason reason)
1105+{
1106+ switch (reason) {
1107+ case MemPoolRemovalReason::EXPIRY:
1108+ return "Expired from mempool";
nit: I think these strings could just be EXPIRY, SIZELIMIT, etc. These strings are for logs only, so will only be read by engineers.
Trying to make these strings more generally accessible obscures some of the nuance. For example MemPoolRemovalReason::REORG means that a transaction was removed during a re-org because it spent a coinbase transaction that is no longer mature, or because it is no longer final, or because mempool policy changed since it first entered the mempool so it can no longer be added or because the re-org was too large and we dropped transactions to avoid memory exhaustion.
145 // the chain actually updates. One way to ensure this is for the caller to invoke this signal
146 // in the same critical section where the chain is updated
147
148- m_internals->m_schedulerClient.AddToProcessQueue([pindexNew, pindexFork, fInitialDownload, this] {
149+ auto func = __func__;
150+ m_internals->m_schedulerClient.AddToProcessQueue([func, pindexNew, pindexFork, fInitialDownload, this] {
The scheduler client guarantees these callbacks are executed serially in the order in which they were enqueued. Unless there is a need to correlate the enqueuing with the execution of BlockChecked and NewPowValidBlock, I’m hesitant to add more complexity to the code.
Is there a specific case where you think the additional logging would be useful? If so, would correlation using data from the log message be sufficient rather than adding a counter?
I think this: #12978 (comment) would have been easier to spot with logging when the callbacks were enqueued, and it would have been easier to correlate those enqueue events with the thread that was running.
You’re probably right that there’s no need for a counter and that the data from the log message is enough to correlate the enqueue/dequeue events.
MainSignalsInstance into the header.
FYI, I had to make some minor changes to placate the linter. It was giving the following failures:
0src/validationinterface.cpp: Expected 0 argument(s) after format string but found 1 argument(s): LogPrint(BCLog::VALIDATIONINTERFACE, fmt, args...)
1^---- failure generated from test/lint/lint-format-strings.sh
2All calls to LogPrintf() and LogPrint() should be terminated with \n
3src/validationinterface.cpp: LogPrint(BCLog::VALIDATIONINTERFACE, fmt, args...);
4^---- failure generated from test/lint/lint-logs.sh
In order to avoid duplicating the logging code, I added some private template methods declared in the header and defined in the implementation file. The linker seems ok with this(…)
This could lead to portability issues, see e.g. https://stackoverflow.com/questions/495021/why-can-templates-only-be-implemented-in-the-header-file. I tried something similar recently in another PR, and the linker in one of the Travis runs did complain, so I am also interested to know if there are better solutions than moving stuff into the header.
Turns out changing EnqueueAndLogEvent into a macro has some implications that are making this infeasible. Consider:
0#define ENQUEUE_AND_LOG_EVENT(event, fmt, ...) \
1 do { \
2 LOG_EVENT("Enqueuing " fmt, __VA_ARGS__); \
3 m_internals->m_schedulerClient.AddToProcessQueue([event, fmt, __VA_ARGS__, this] { \
4 LOG_EVENT(fmt, __VA_ARGS__); \
5 event(); \
6 }); \
7 } while (0)
8
9#define LOG_EVENT(fmt, ...) \
10 LogPrint(BCLog::VALIDATIONINTERFACE, fmt "\n", __VA_ARGS__);
The problem is that the capture list is expecting variables but the call sites are providing expressions. So unfortunately this won’t compile. Presumably, if this did work, the expressions in the lambda would be evaluated thus defeating the purpose.
Unless there is a creative solution that I’m unaware of, the only alternative I can think of is to forgo the macro and duplicate the LogPrint for the enqueuing in each method. Thoughts?
__VA_ARGS__ will do what I need!
I did some light testing and this looks good. If you wanted to add automated tests, take a look at assert_debug_log() in the TestNode object. That allows you to test that specific strings have been written to the debug log.
updated CValidationState accessors in 3ff8755 to return strings by constant reference to avoid copying.
Was this required for the PR? Those accessors aren’t used very much, so performance isn’t critical. I’d suggest not changing them in this PR.
I did some light testing and this looks good. If you wanted to add automated tests, take a look at
assert_debug_log()in theTestNodeobject. That allows you to test that specific strings have been written to the debug log.
I would be hesitant to add a functional test only to check for informational logging. Is there an existing a test where all these events are exercised? Not sure if it is worth adding regardless.
updated CValidationState accessors in 3ff8755 to return strings by constant reference to avoid copying.
Was this required for the PR? Those accessors aren’t used very much, so performance isn’t critical. I’d suggest not changing them in this PR.
Yeah, commit 72c6e50 calls those methods in such a way that not returning references would have incurred excessive copies. Would it be better if the two commits were merged?
@jnewbery I have some concerns about this additional logging. Since #14209 was merged, all log function arguments are evaluated regardless of whether logging is enabled for the given category. This was not the case prior to that PR since a macro was used for logging.
Given the heavy amount of logging added in this PR, should the function vs macro approach for logging be revisited? Typically, the use of macros in logging libraries is to avoid such argument evaluation if I understand correctly.
Since #14209 was merged, all log function arguments are evaluated regardless of whether logging is enabled for the given category.
Huh? They were passed to strprintf before and after that change, but strprintf was only called when logging was enabled. I don’t see how that pull changed anything.
Since #14209 was merged, all log function arguments are evaluated regardless of whether logging is enabled for the given category.
Huh? They were passed to
strprintfbefore and after that change, butstrprintfwas only called when logging was enabled. I don’t see how that pull changed anything.
By evaluation I mean at the call site of LogPrint. While this doesn’t make a difference when passing variables, it does matter when the arguments are calls to other functions. Prior to #14209, the latter would be textually included when used and thus not evaluated if logging was not enabled for the given category.
136@@ -137,8 +137,8 @@ class CValidationState {
137 }
138 ValidationInvalidReason GetReason() const { return m_reason; }
139 unsigned int GetRejectCode() const { return chRejectCode; }
140- std::string GetRejectReason() const { return strRejectReason; }
141- std::string GetDebugMessage() const { return strDebugMessage; }
142+ const std::string& GetRejectReason() const { return strRejectReason; }
143+ const std::string& GetDebugMessage() const { return strDebugMessage; }
What is the object lifetime of strRejectReason and strDebugMessage?
Are they guaranteed to outlive the callers of GetRejectReason or GetDebugMessage?
CValidationState as mentioned in #16688 (comment). All current uses are guaranteed to outlive the callers as they either make a copy or are passed by const reference to other functions that do something similar. There’s no such guarantee for future callers though.
I don’t think 6c730c3926d1871d8ce4c63eb6752808d7c3484a is a good idea: it introduces a very sharp edge in consensus critical code which is not offset by any measurable gain AFAICT.
We should always err on the side of caution when touching consensus critical code: if there is a trade-off between safety (current or future) and speed, then safety is generally much more important. In the cases where speed is critical for some reasons we should always measure before optimising :)
Somewhat related: https://www.youtube.com/watch?v=3MB2iiCkGxg&t=870
1114+ return "BLOCK";
1115+ case MemPoolRemovalReason::CONFLICT:
1116+ return "CONFLICT";
1117+ case MemPoolRemovalReason::REPLACED:
1118+ return "REPLACED";
1119+ }
0 } // no default case, so the compiler can warn about missing cas
See: Developer Notes - switch statement on an enumeration example
215+ LogEvent("%s: block hash=%s", __func__, block->GetHash().ToString());
216 m_internals->NewPoWValidBlock(pindex, block);
217 }
218+
219+template <typename... Args>
220+void CMainSignals::EnqueueAndLogEvent(std::function<void ()> event, const char* fmt, const Args&... args) {
event parameter be a const reference?
string (i.e., basic_string<char>) offers move semantics; so it can be efficiently return by value.
IMO, 6c730c3 commit is outside the original PR goal. Also
string(i.e.,basic_string<char>) offers move semantics; so it can be efficiently return by value.
I removed the commit. But I don’t believe move semantics apply here as the function is returning a member variable.
I removed the commit. (https://github.com/bitcoin/bitcoin/commit/6c730c3926d1871d8ce4c63eb6752808d7c3484a)
Thanks!
I’ve tested this and it works great. I think we need to resolve this question: #16688 (comment) before merging this PR. It seems reasonable to me to change logging back to using a macro for the reasons you suggest. Adding a GetHash() call to every validation interface call, even when logging is disabled, seems a shame. I’d be happy to review a PR that does that.
Rename CMainSignals::MempoolEntryRemoved to TransactionRemovedFromMempool and Format CValidationState properly in all cases could be pulled out into their own PR while we’re waiting for that, although note that Format CValidationState properly in all cases conflicts with https://github.com/bitcoin/bitcoin/pull/17004/commits/0053e16714323c1694c834fdca74f064a1a33529.
I’ve tested this and it works great. I think we need to resolve this question: #16688 (comment) before merging this PR. It seems reasonable to me to change logging back to using a macro for the reasons you suggest. Adding a
GetHash()call to every validation interface call, even when logging is disabled, seems a shame. I’d be happy to review a PR that does that. @jnewbery Will do. @MarcoFalke I’m not sure I follow the part about “unnecessary branches are not analyzed” in #14209. If I were to makeLogPrint(but notLogPrintf) a macro again, will I need to define it conditionally based onUSE_COVERAGEas was done in c8914b9?
I would be hesitant to add a functional test only to check for informational logging. Is there an existing a test where all these events are exercised? Not sure if it is worth adding regardless.
Yeah, I agree that this isn’t necessary.
Rename CMainSignals::MempoolEntryRemoved to TransactionRemovedFromMempool and Format CValidationState properly in all cases could be pulled out into their own PR while we’re waiting for that, although note that Format CValidationState properly in all cases conflicts with 0053e16.
I’ve rebased and resolved the conflict now that your change has been merged. The PR should be ready for any last comments. Also, will need reviews of #17218. :)
It appears that the function name isn’t being captured properly:
02019-11-01T15:06:44.834237Z [loadblk] Enqueuing BlockConnected: block hash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 block height=0
12019-11-01T15:06:44.834255Z [loadblk] Enqueuing UpdatedBlockTip: new block hash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 fork block hash=null (in IBD=1)
22019-11-01T15:06:44.834276Z [init] block tree size = 1
32019-11-01T15:06:44.834283Z [init] nBestHeight = 0
42019-11-01T15:06:44.834338Z [init] Bound to 127.0.0.1:11625
52019-11-01T15:06:44.834350Z [init] init message: Loading P2P addresses...
62019-11-01T15:06:44.834367Z [init] ERROR: DeserializeFileDB: Failed to open file /tmp/user/1000/bitcoin_func_test_if8trhon/node0/regtest/peers.dat
72019-11-01T15:06:44.834438Z [init] Invalid or missing peers.dat; recreating
82019-11-01T15:06:44.834758Z [scheduler] operator(): block hash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 block height=0
92019-11-01T15:06:44.834919Z [scheduler] operator(): new block hash=0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206 fork block hash=null (in IBD=1)
Those last two lines should show BlockConnected and UpdatedBlockTip
It appears that the function name isn’t being captured properly:
Fixed in 9eb373c.
142+// evaluating arguments when logging is not enabled.
143+//
144+// NOTE: The lambda captures all local variables by value.
145+#define ENQUEUE_AND_LOG_EVENT(event, fmt, name, ...) \
146+ do { \
147+ auto local_name = (name); \
The downside is that the logging can go out of sync if the method is renamed (like in dd2855d) and that the format string is longer.
Note this implementation was used in an earlier version of the PR: #16688#pullrequestreview-278747465 (see second outdated comment). It was no longer needed when I added a helper method but needed again once I switched to a macro.
166- });
167+ };
168+ ENQUEUE_AND_LOG_EVENT(event, "%s: new block hash=%s fork block hash=%s (in IBD=%d)", __func__,
169+ pindexNew->GetBlockHash().ToString(),
170+ pindexFork ? pindexFork->GetBlockHash().ToString() : "null",
171+ fInitialDownload);
fInitialDownload ? 'true' : 'false'
Done in f871e27 by changing %d to %s, as tinyformat does the conversion if using that specifier.
https://github.com/c42f/tinyformat#special-cases-for-p-c-and-s
210+ auto event = [pblock, this] {
211 m_internals->BlockDisconnected(pblock);
212- });
213+ };
214+ ENQUEUE_AND_LOG_EVENT(event, "%s: block hash=%s", __func__,
215+ pblock->GetHash().ToString());
Tested ACK 9eb373c07ad78448ea78eee97d1ee9b9b409b4b9
One personal-style-preference nit, which you should feel free to ignore.
I’ve run several functional tests with this and manually tested it with a regtest node. It’s really illuminating to see where the CValidationInterface callbacks are being enqueued and then called.
Really great stuff @jkczyz . Thanks for persisting with this!
355@@ -356,6 +356,8 @@ enum class MemPoolRemovalReason {
356 REPLACED, //!< Removed for replacement
357 };
358
359+std::string MemPoolRemovalReasonString(const MemPoolRemovalReason reason);
130@@ -126,52 +131,90 @@ void SyncWithValidationInterfaceQueue() {
131 promise.get_future().wait();
132 }
133
134+// Use a macro instead of a function for conditional logging to prevent
compare the macro expansion:
0if (debug_foo) {
1 LogPrintf("Foobar %i", ExpensiveToComputeFunction());
2}
with
0LogPrint(BCLog::FOO, "Foobar %i", ExpensiveToComputeFunction());
In the first example, ExpensiveToComputeFunction is not invoked if debug_foo is false. In the second one it’s invoked, but then disregarded by LogPrint.
LogPrint since it was simply partially reverting a commit. It would have been a much larger change to update all the call sites, and that’s something I’d be hesitant in undertaking without enough support for the change.
-debug=validationinterface set, print messages as expected. Holding my ACK on answer on the macro question.
BlockDisconnected here: https://github.com/jnewbery/bitcoin/tree/pr16688.1 . Feel free to take it!
Tested ACK 741cab0a7dcfe07860db8ad73678fbfde7eb6ada
Your latest push has different emails for the Author and Committer in the commits. I’m not sure if that was intentional or you want to change it back.
BlockDisconnected log.
@jnewbery asked to have the reason added in this PR
It’s not essential but I think logging the reason is definitely a nice-to-have. None of the clients need access to the reason (yet) so it could just be logged and dropped in the validationinterface.
It’s not essential but I think logging the reason is definitely a nice-to-have. None of the clients need access to the reason (yet) so it could just be logged and dropped in the validationinterface.
Done in 34d6486.
The linter says 34d6486 introduced a circular dependency:
0A new circular dependency in the form of "txmempool -> validationinterface -> txmempool" appears to have been introduced.
To break this, I’d have to move both MemPoolRemovalReason and MemPoolRemovalReasonString elsewhere. Thoughts?
To break this, I’d have to move both MemPoolRemovalReason and MemPoolRemovalReasonString elsewhere. Thoughts?
Just remove that commit. No need to make this PR more complex just to log the removal reason. It can be done in a follow-up.
There are some code organization concerns.
I believe that headers and forward class declaration should not be touched in the “Format CValidationState properly in all cases” commit (ee4d66a38c5c09161f8a3e63047db6b6908687f4).
Instead, <consensus/validation.h> should be included to validationinterface.cpp in the “Add logging for CValidationInterface events” commit (0cadb123bad37a2c224834a3d98bed474c661c21).
Tested on Linux Mint 19.3. It works as expected.
The only suggestion: #16688 (comment)
161@@ -162,6 +162,7 @@ const CLogCategoryDesc LogCategories[] =
162 {BCLog::COINDB, "coindb"},
163 {BCLog::QT, "qt"},
164 {BCLog::LEVELDB, "leveldb"},
165+ {BCLog::VALIDATIONINTERFACE, "validationinterface"},
In commit “Add VALIDATIONINTERFACE to BCLog::LogFlags” (389d423a252e43d0b08871f8ae5b66a84a8a217c)
Would change names to VALIDATION, "validation". These are just validation events. Fact that they are printed in the validationinterface source file instead of the validation one is an implementation detail that could change and would be arcane to someone just trying to configure logging categories.
5@@ -6,9 +6,9 @@
6 #ifndef BITCOIN_UTIL_VALIDATION_H
7 #define BITCOIN_UTIL_VALIDATION_H
8
9-#include <string>
10+#include <consensus/validation.h>
11
12-class ValidationState;
In commit “Format CValidationState properly in all cases” (ee4d66a38c5c09161f8a3e63047db6b6908687f4)
Replacing forward declaration with a full include seems unnecessary here, just making the diff bigger and perhaps unnecessarily slowing down compilation.
19+ }
20+
21+ const std::string debug_message = state.GetDebugMessage();
22+ if (!debug_message.empty()) {
23+ return strprintf("%s, %s", state.GetRejectReason(), debug_message);
24+ }
In commit “Format CValidationState properly in all cases” (ee4d66a38c5c09161f8a3e63047db6b6908687f4)
I’m a little confused by all the changes in this commit. As far as I can tell the only change that affects behavior is adding if (state.IsValid()) return "Valid"; at the top of this function, and everything else is a rewrite / style cleanup. Not a big deal, but in the future it might be good to fully motivate changes in the commit description or do style changes in separate commits.
181 }
182
183-void CMainSignals::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex)
184-{
185- m_internals->m_schedulerClient.AddToProcessQueue([pblock, pindex, this] {
186+void CMainSignals::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex) {
In commit “Add logging for CValidationInterface events” (0cadb123bad37a2c224834a3d98bed474c661c21)
Whitespace is changing unnecessarily (and format is nonstandard, brace should go on new line)
I was largely trying to avoid style changes and conform with the local conventions in the file. However, in the interim the brace was moved in https://github.com/bitcoin/bitcoin/commit/10b4729e33f76092bd8cfa06d1a5e0a066436f76#diff-e8d9e22d9683f73a9fb8399be0dab640R159 to use the standard conventions.
Not sure what is preferred, but I don’t mind changing it back to reduce the diff size.
This flag is for logging from within CValidationInterface (see #12994).
A separate flag is desirable as the logging can be noisy and thus may
need to be disabled without affecting other logging.
FormatStateMessage does not properly handle the case where
CValidationState::IsValid() returns true. Use "Valid" for the state in
this case.
All cases of CValidationState were condensed into one strprintf call.
This is no longer suitable as more cases are added (e.g., IsValid).
This could help debug issues where there may be race conditions at play,
such as #12978.
Fixes #12994.
VALIDATIONINTERFACE by VALIDATION and avoiding a forward declaration with a new include
ACK e7f84503571c171a7e6728cd2d77dd4103bd7a6f posthumous code review and manual tests.
Usage listed here for visitor convenience (the PR description was not updated from validationinterface to validation):
bitcoind -help | grep -B6 ", validation"0 -debug=<category>
1 Output debugging information (default: -nodebug, supplying <category> is
2 optional). If <category> is not supplied or if <category> = 1,
3 output all debugging information. <category> can be: net, tor,
4 mempool, http, bench, zmq, walletdb, rpc, estimatefee, addrman,
5 selectcoins, reindex, cmpctblock, rand, prune, proxy, mempoolrej,
6 libevent, coindb, qt, leveldb, validation.
bitcoind -debug=validation (launch bitcoind with validation logging)bitcoin-cli help loggingbitcoin-cli logging [] '["validation"]' (toggle validation logging off)bitcoin-cli logging '["validation"]' (toggle validation logging on)