log: Move validation option logging to LoadChainstate() #25951

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2208-log-val-🚣 changing 2 files +15 −10

MarcoFalke commented at 10:03 AM on August 29, 2022: member

This would allow libbitcoinkernel users to see the options logged as well. Currently they would only be logged for bitcoind. Behavior change suggested in the refactoring pull #25704 (review)
Move validation option logging to LoadChainstate() fa3358b668
DrahtBot added the label Utils/log/libs on Aug 29, 2022

MarcoFalke commented at 10:55 AM on August 29, 2022: member

One way to test this is to modify bitcoin-chainstate a bit:

diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index 7312ae45d4..4ec71ca3fb 100644
--- a/src/bitcoin-chainstate.cpp
+++ b/src/bitcoin-chainstate.cpp
@@ -53,6 +53,10 @@ int main(int argc, char* argv[])
     // SETUP: Misc Globals
     SelectParams(CBaseChainParams::MAIN);
     const CChainParams& chainparams = Params();
+LogInstance().m_print_to_console=true;
+LogInstance().m_file_path = AbsPathForConfigVal("log_debug");
+LogInstance().m_log_timestamps = false;
+Assert(LogInstance().StartLogging());
 
     kernel::Context kernel_context{};
     // We can't use a goto here, but we can use an assert since none of the

Command to test:

make && ./src/bitcoin-chainstate /tmp/temp_dd

Output diff:

@@ -3,6 +3,9 @@ Using RdSeed as an additional entropy source
 Using RdRand as an additional entropy source
 Using 16 MiB out of 16 MiB requested for signature cache, able to store 524288 elements
 Using 16 MiB out of 16 MiB requested for script execution cache, able to store 524288 elements
+Validating signatures for all blocks.
+Setting nMinimumChainWork=0000000000000000000000000000000000000000000000000000000000000000
+Warning: nMinimumChainWork set below default value of 00000000000000000000000000000000000000002927cdceccbd5209e81e80db
 Switching active chainstate to Chainstate [ibd] @ height -1 (null)
 Opening LevelDB in /tmp/temp_dd/blocks/index
 scheduler thread start

DrahtBot commented at 6:24 PM on August 29, 2022: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #25704 (refactor: Remove almost all validation option globals by MarcoFalke)
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.
ryanofsky approved
ryanofsky commented at 6:18 PM on August 30, 2022: contributor

Code review ACK fa3358b668d8415adc1b5586caa382fe4140ad42. This should simplify #25704 a little.

One suggestion: It might also be good to move the pruning log prints immediately below to simplify #25781
jonatack commented at 1:21 PM on August 31, 2022: contributor

Concept ACK
fanquake requested review from theuni on Aug 31, 2022

jonatack commented at 12:27 PM on September 1, 2022: contributor

ACK fa3358b668d8415adc1b5586caa382fe4140ad42

Tested and it works with bitcoin-chainstate as described in #25951 (comment).

Note that this pull also changes the placement of this logging from the top

2022-08-31T20:05:35Z Bitcoin Core version v23.99.0-7c7c71500403-dirty (debug build)
2022-08-31T20:05:35Z Assuming ancestors of block 000000000000000000052d314a259755ca65944e68df6b12a067ea8f1f5a7091 have valid signatures.
2022-08-31T20:05:35Z Setting nMinimumChainWork=00000000000000000000000000000000000000002927cdceccbd5209e81e80db

to a bit further down after the asmap, "Loading P2P addresses…" and "Cache configuration" sections into the "init message: Loading block index…" one, which seems ok

...
2022-09-01T12:08:23Z Using BerkeleyDB version ...
2022-09-01T12:08:23Z Using wallet ...
2022-09-01T12:08:23Z BerkeleyEnvironment::Open: LogDir=
2022-09-01T12:08:23Z Opened asmap file (1219959 bytes) from disk
2022-09-01T12:08:25Z Using asmap version  for IP bucketing
2022-09-01T12:08:25Z init message: Loading P2P addresses…
2022-09-01T12:08:27Z Loaded 75260 addresses from peers.dat  2853ms
2022-09-01T12:08:27Z init message: Loading banlist…
2022-09-01T12:08:27Z SetNetworkActive: true
2022-09-01T12:08:27Z Cache configuration:
...
2022-09-01T12:08:27Z init message: Loading block index…
2022-09-01T12:08:27Z Assuming ancestors of block 000000000000000000052d314a259755ca65944e68df6b12a067ea8f1f5a7091 have valid signatures.
2022-09-01T12:08:27Z Setting nMinimumChainWork=00000000000000000000000000000000000000002927cdceccbd5209e81e80db

Move blockstorage option logging to LoadChainstate() fa4c59d65b
ryanofsky approved
ryanofsky commented at 3:46 PM on September 1, 2022: contributor

Code review ACK fa4c59d65bdf1c64056b18a58f8aaa2800995aa6. Only change since last review is moving pruning logprints out of AppInitParameterInteraction as suggested
jonatack commented at 4:49 PM on September 1, 2022: contributor

Review ACK fa4c59d65bdf1c64056b18a58f8aaa2800995aa6
MarcoFalke merged this on Sep 1, 2022
MarcoFalke closed this on Sep 1, 2022
sidhujag referenced this in commit e5ead4fe3d on Sep 1, 2022
MarcoFalke deleted the branch on Sep 2, 2022
theuni commented at 2:30 PM on September 6, 2022: member

I kinda would've expected the opposite here, where statuses are returned and the caller (bitcoin-chainstate) can choose to report them as necessary.

I've probably missed discussions about logging though. Is the plan to have an instantiated logger in the future so that the caller can control the output?
MarcoFalke commented at 3:16 PM on September 6, 2022: member

InitializeChainstate already calls LogPrintf, so all LogPrintf (old and new) can be switched to something else once it is clear what that is. I am not sure myself what the plan here is.
ryanofsky commented at 4:05 PM on September 6, 2022: contributor

I think log prints are useful whether you are calling a library or running a daemon, so it shouldn't be a goal to strip them out.

I lost track of the different branches Carl sent (https://github.com/dongcarl/bitcoin/commits/2022-03-kirby-p2.5, etc) but I think all of them kept a global logger object. The global object could be fully or partially replaced with locals if it is a problem.
bitcoin locked this on Sep 6, 2023

Contributors

Review Requested
theuni

Labels

Utils/log/libs

Linked (view graph)

#25704 refactor: Remove almost all validation option globals

log: Move validation option logging to LoadChainstate() #25951

Conflicts