kernel, logging: Pass Logger instances to kernel objects #30342

pull ryanofsky wants to merge 9 commits into bitcoin:master from ryanofsky:pr/gklog changing 47 files +600 −296
  1. ryanofsky commented at 5:52 pm on June 26, 2024: contributor

    Pass Logger instances to BlockManager, CCoinsViewDB, CDBWrapper, Chainstate, ChainstateManager, CoinsViews, and CTxMemPool classes so libbitcoinkernel applications and tests have the option to control where log output goes instead of having all output sent to the global logger.

    This PR is an alternative to #30338. It is more limited because it only changes kernel code while leaving other parts of the codebase alone. It also takes the opportunity to migrate legacy LogPrint / LogPrintf calls to the new log macros introduced in #28318.


    This is based on #29256. The non-base commits are:

  2. DrahtBot commented at 5:52 pm on June 26, 2024: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30342.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK TheCharlatan

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32185 (coins: replace manual CDBBatch size estimation with LevelDB’s native ApproximateSize by l0rinc)
    • #31981 (Add checkBlock() to Mining interface by Sjors)
    • #31860 (init: Take lock on blocks directory in BlockManager ctor by TheCharlatan)
    • #31644 (leveldb: show non-default options during init by l0rinc)
    • #31576 (test: Move script_assets_tests into its own suite by hebasto)
    • #31533 (fuzz: Add fuzz target for block index tree and related validation events by mzumsande)
    • #31405 (validation: stricter internal handling of invalid blocks by mzumsande)
    • #31382 (kernel: Flush in ChainstateManager destructor by TheCharlatan)
    • #31144 ([IBD] multi-byte block obfuscation by l0rinc)
    • #31132 (validation: fetch block inputs on parallel threads 10% faster IBD by andrewtoth)
    • #30673 (coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries by andrewtoth)
    • #30610 (validation: do not wipe utxo cache for stats/scans/snapshots by sipa)
    • #30214 (refactor: Improve assumeutxo state representation by ryanofsky)
    • #30155 (validation: Make ReplayBlocks interruptible by mzumsande)
    • #30059 (Add option dbfilesize to control LevelDB target (“max”) file size by luke-jr)
    • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #29640 (Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties) by sr-gi)
    • #29491 ([EXPERIMENTAL] Schnorr batch verification for blocks by fjahr)
    • #27826 (validation: log which peer sent us a header by Sjors)
    • #26022 (Add util::ResultPtr class by ryanofsky)
    • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
    • #19463 (Prune locks by luke-jr)

    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.

  3. TheCharlatan commented at 8:21 pm on June 26, 2024: contributor
    Concept ACK.
  4. ryanofsky force-pushed on Jun 26, 2024
  5. DrahtBot added the label CI failed on Jun 26, 2024
  6. DrahtBot commented at 10:35 pm on June 26, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/26722091796

  7. ryanofsky commented at 10:36 pm on June 26, 2024: contributor
    Updated 45423355735770f01d2bac738cc8b0b41415e921 -> db1b9f7696af80036de739408cd9eae5d06481ec (pr/gklog.1 -> pr/gklog.2, compare) moving code to an earlier commit to fix a “test-each-commit” test failure https://github.com/bitcoin/bitcoin/actions/runs/9684398780/job/26722073921?pr=30342, and cleaning up code to fix a clang-tidy warning https://cirrus-ci.com/task/5893151045451776
  8. DrahtBot removed the label CI failed on Jun 27, 2024
  9. DrahtBot added the label Needs rebase on Jul 2, 2024
  10. ryanofsky referenced this in commit ecadc8f6e5 on Jul 3, 2024
  11. ryanofsky force-pushed on Jul 9, 2024
  12. ryanofsky commented at 10:23 pm on July 9, 2024: contributor
    Rebased db1b9f7696af80036de739408cd9eae5d06481ec -> 42d0e06d1a4343c2bf3257f26cf18b79072c6b0d (pr/gklog.2 -> pr/gklog.3, compare) to fix conflict with #30141. Also added a commit to let kernel applications direct low-level util output to custom log instances. Updated 42d0e06d1a4343c2bf3257f26cf18b79072c6b0d -> fcaed3faf0f12b3b322e65df1e81d455bbee7db5 (pr/gklog.3 -> pr/gklog.4, compare) to fix CI errors Updated fcaed3faf0f12b3b322e65df1e81d455bbee7db5 -> f005677f762754ae09417c2a418e27d28ef3c7e1 (pr/gklog.4 -> pr/gklog.5, compare) to fix more CI errors Rebased f005677f762754ae09417c2a418e27d28ef3c7e1 -> dcd055ecf9c20b84930c2ce45d922427cf0112ea (pr/gklog.5 -> pr/gklog.6, compare) due to conflicts with #30425 and #30407 Rebased dcd055ecf9c20b84930c2ce45d922427cf0112ea -> 53d7933727ebe0670dceb88ec9f1020bd7b0b280 (pr/gklog.6 -> pr/gklog.7, compare) due to conflict with #28052 Rebased 53d7933727ebe0670dceb88ec9f1020bd7b0b280 -> 10c275342dca365a6678da5fc016ca9d46d9dfca (pr/gklog.7 -> pr/gklog.8, compare) due to conflict with #30485 Rebased 10c275342dca365a6678da5fc016ca9d46d9dfca -> 493dd26b75d0e35422b3230be3c72a0f96eb5e8a (pr/gklog.8 -> pr/gklog.9, compare) due to various conflicts Rebased 493dd26b75d0e35422b3230be3c72a0f96eb5e8a -> e971028fa5e570acf585073494acd65f4831ab1c (pr/gklog.9 -> pr/gklog.10, compare) due to conflicts with #31393, #31490, #30965
  13. DrahtBot removed the label Needs rebase on Jul 10, 2024
  14. ryanofsky force-pushed on Jul 10, 2024
  15. ryanofsky force-pushed on Jul 10, 2024
  16. DrahtBot added the label CI failed on Jul 10, 2024
  17. DrahtBot commented at 6:22 pm on July 10, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/27284943547

  18. DrahtBot removed the label CI failed on Jul 10, 2024
  19. DrahtBot added the label Needs rebase on Jul 16, 2024
  20. ryanofsky force-pushed on Jul 18, 2024
  21. DrahtBot removed the label Needs rebase on Jul 18, 2024
  22. DrahtBot added the label CI failed on Jul 22, 2024
  23. DrahtBot commented at 11:07 am on July 22, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27590643116

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  24. DrahtBot removed the label CI failed on Jul 22, 2024
  25. DrahtBot added the label Needs rebase on Jul 26, 2024
  26. ryanofsky force-pushed on Aug 7, 2024
  27. ryanofsky force-pushed on Aug 7, 2024
  28. DrahtBot removed the label Needs rebase on Aug 7, 2024
  29. DrahtBot added the label CI failed on Aug 13, 2024
  30. DrahtBot commented at 7:50 pm on August 13, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28438476134

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  31. DrahtBot added the label Needs rebase on Aug 21, 2024
  32. ryanofsky force-pushed on Dec 9, 2024
  33. DrahtBot removed the label Needs rebase on Dec 9, 2024
  34. DrahtBot removed the label CI failed on Dec 9, 2024
  35. DrahtBot added the label CI failed on Dec 11, 2024
  36. DrahtBot removed the label CI failed on Dec 17, 2024
  37. DrahtBot added the label Needs rebase on Dec 18, 2024
  38. ryanofsky force-pushed on Mar 12, 2025
  39. DrahtBot removed the label Needs rebase on Mar 12, 2025
  40. DrahtBot added the label Needs rebase on Mar 14, 2025
  41. log, test: Add test for currently accepted logging arguments
    Test will be extended in next commit to cover support for context objects.
    f62272d811
  42. log, refactor: Allow log macros to accept context arguments
    Allow LogDebug(), LogTrace(), LogInfo(), LogWarning(), and LogError() macros to
    accept context arguments to provide more information in log messages and more
    control over logging to callers.
    
    This functionality is used in followup PRs:
    
    - https://github.com/bitcoin/bitcoin/pull/30342 - To let libbitcoinkernel send
      output to specfic `BCLog::Logger` instances instead of a global instance, so
      output can be disambiguated and applications can have more control over
      logging.
    
    - https://github.com/bitcoin/bitcoin/pull/30343 - To replace custom
      `WalletLogPrintf` calls with standard logging calls that automatically include
      wallet names and don't log everything at info level.
    
    This commit does not change behavior of current log prints or require them to
    be updated. It includes tests and documentation covering the new functionality.
    
    Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    eaac991552
  43. log, refactor: Add preprocessor workaround for MSVC
    Needed to fix errors like:
    
    const Context &GetContext(const Context &)': expects 1 arguments - 3 provided
    
    due to a compiler bug:
    
    https://stackoverflow.com/questions/5134523/msvc-doesnt-expand-va-args-correctly/5134656#5134656
    
    Example CI failure:
    
    https://github.com/bitcoin/bitcoin/actions/runs/8072891468/job/22055528830?pr=29256
    6e41dfa317
  44. log, refactor: Disallow category args to logging macros
    Disallow passing BCLog category constants to LogInfo(), LogWarning(), and
    LogError() macros, and disallow omitting BCLog categories when calling
    LogDebug() and LogTrace() macros.
    
    These restrictions have existed since the logging macros were added in #28318
    and not technically neccessary, but are believed to be useful to prevent log
    spam and prevent users from filtering out important messages based on category.
    
    Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    093d58e055
  45. Merge remote-tracking branch 'origin/pull/29256/head' e249f9d924
  46. refactor: Pass Logger instances to kernel objects
    Pass Logger instances to BlockManager, CCoinsViewDB, CDBWrapper,
    ChainstateManager, and CoinsViews instances so libbitcoinkernel applications
    and test code have the option to control where log output goes instead of
    having all output sent to the global logger.
    
    This commit just passes the logger objects without using them. The next commit
    updates log print statements to use the new objects.
    1c58fc2adc
  47. refactor: Log kernel output to local log instances
    This is a mechanical change updating kernel code that currently uses the global
    log instance to log to local instances instead.
    8517d317c2
  48. refactor: Pass Logger instance to CTxMemPool
    This allows libbitcoinkernel applications and test code to have the option to
    control where log output goes instead of having all output sent to the global
    logger.
    3e859af7a1
  49. kernel: Drop global Logger instance
    Change LogInstance() function to no longer allocate (and leak) a BCLog::Logger
    instance. Instead allow kernel applications to initialize their own logging
    instances that can be returned by LogInstance().
    
    The LogInstance() function is not actually used for the majority of logging in
    kernel code. Most kernel log output goes directly to BCLog::Logger instances
    specified when kernel objects like ChainstateManager and CTxMemPool are
    constructed, which allows applications to create multiple Logger instances and
    control which log output is sent to them.
    
    The only kernel code that does rely on LogInstance() is certain low level code in
    the util library, like the RNG seeder, that is not passed a specific instance
    and needs to rely on the global LogInstance() function.
    
    Other code outside the kernel library uses LogInstance() heavily, and may
    continue to do so. bitcoind and test code now just create a log instance before
    the first LogInstance() call, which it returns, so all behavior is the same as
    before.
    ec6d771d3b
  50. ryanofsky referenced this in commit 6d0745d22b on Apr 3, 2025
  51. ryanofsky force-pushed on Apr 3, 2025
  52. DrahtBot removed the label Needs rebase on Apr 3, 2025
  53. ryanofsky referenced this in commit 2be41ef85f on Apr 3, 2025
  54. ryanofsky force-pushed on Apr 4, 2025
  55. DrahtBot added the label CI failed on Apr 4, 2025
  56. DrahtBot commented at 3:37 am on April 4, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39937617956

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  57. DrahtBot removed the label CI failed on Apr 4, 2025
  58. DrahtBot added the label Needs rebase on Apr 8, 2025
  59. DrahtBot commented at 9:57 pm on April 8, 2025: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.


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: 2025-04-28 00:12 UTC

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