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
  1. 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)
  2. Move validation option logging to LoadChainstate() fa3358b668
  3. DrahtBot added the label Utils/log/libs on Aug 29, 2022
  4. MarcoFalke commented at 10:55 am on August 29, 2022: member

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

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

    Command to test:

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

    Output diff:

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

  6. ryanofsky approved
  7. 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

  8. jonatack commented at 1:21 pm on August 31, 2022: contributor
    Concept ACK
  9. fanquake requested review from theuni on Aug 31, 2022
  10. 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

    02022-08-31T20:05:35Z Bitcoin Core version v23.99.0-7c7c71500403-dirty (debug build)
    12022-08-31T20:05:35Z Assuming ancestors of block 000000000000000000052d314a259755ca65944e68df6b12a067ea8f1f5a7091 have valid signatures.
    22022-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

     0...
     12022-09-01T12:08:23Z Using BerkeleyDB version ...
     22022-09-01T12:08:23Z Using wallet ...
     32022-09-01T12:08:23Z BerkeleyEnvironment::Open: LogDir=
     42022-09-01T12:08:23Z Opened asmap file (1219959 bytes) from disk
     52022-09-01T12:08:25Z Using asmap version  for IP bucketing
     62022-09-01T12:08:25Z init message: Loading P2P addresses…
     72022-09-01T12:08:27Z Loaded 75260 addresses from peers.dat  2853ms
     82022-09-01T12:08:27Z init message: Loading banlist…
     92022-09-01T12:08:27Z SetNetworkActive: true
    102022-09-01T12:08:27Z Cache configuration:
    11...
    122022-09-01T12:08:27Z init message: Loading block index…
    132022-09-01T12:08:27Z Assuming ancestors of block 000000000000000000052d314a259755ca65944e68df6b12a067ea8f1f5a7091 have valid signatures.
    142022-09-01T12:08:27Z Setting nMinimumChainWork=00000000000000000000000000000000000000002927cdceccbd5209e81e80db
    
  11. Move blockstorage option logging to LoadChainstate() fa4c59d65b
  12. ryanofsky approved
  13. 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
  14. jonatack commented at 4:49 pm on September 1, 2022: contributor
    Review ACK fa4c59d65bdf1c64056b18a58f8aaa2800995aa6
  15. MarcoFalke merged this on Sep 1, 2022
  16. MarcoFalke closed this on Sep 1, 2022

  17. sidhujag referenced this in commit e5ead4fe3d on Sep 1, 2022
  18. MarcoFalke deleted the branch on Sep 2, 2022
  19. 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?

  20. 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.
  21. 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.

  22. bitcoin locked this on Sep 6, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-31 03:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me