log: Don’t log cache rebalancing in absense of a snapshot chainstate #28569

pull fjahr wants to merge 1 commits into bitcoin:master from fjahr:2023-10-au-cache-log changing 1 files +2 −2
  1. fjahr commented at 11:41 am on October 3, 2023: contributor
    I have noticed that this log now is always printed, even if there is no snapshot chainstate present or even was present. I think this is confusing to users that have never even thought about using assumeutxo since in that case the rebalancing is just ensuring the normal environment with one chainstate. So I suggest we don’t log in absence of a snapshot chainstate. We could also think about rewording the message instead but I think this is simpler.
  2. DrahtBot commented at 11:41 am on October 3, 2023: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK stickies-v, glozow, theStack
    Concept ACK Sjors

    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:

    • #28608 (assumeutxo state and locking cleanup by ryanofsky)

    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. maflcko commented at 11:48 am on October 3, 2023: member
    Missing log: prefix in title?
  4. fanquake renamed this:
    Don't log cache rebalancing in absense of a snapshot chainstate
    log: Don't log cache rebalancing in absense of a snapshot chainstate
    on Oct 3, 2023
  5. DrahtBot added the label Utils/log/libs on Oct 3, 2023
  6. Sjors commented at 7:54 am on October 9, 2023: member

    Concept ACK on dropping this message when there’s no snapshot stuff going on.

    How do you even trigger this log message now? Loading an invalid snapshot?

  7. fjahr commented at 4:01 pm on October 10, 2023: contributor

    Concept ACK on dropping this message when there’s no snapshot stuff going on.

    How do you even trigger this log message now? Loading an invalid snapshot?

    Right, I would be also happy with dropping the message altogether if you prefer that approach.

  8. fanquake added this to the milestone 26.0 on Oct 10, 2023
  9. Sjors commented at 7:22 am on October 11, 2023: member
    If it can happen when loading an invalid snapshot then it’s probably fine to keep it. No strong feelings.
  10. maflcko removed this from the milestone 26.0 on Oct 19, 2023
  11. maflcko commented at 7:56 am on October 19, 2023: member
    Removing from milestone. AU is experimental and test-only, so a log issue, whose discussion is still ongoing, is not a blocker for the release. If needed, it can be backported to 26.1?
  12. fanquake commented at 9:06 am on October 19, 2023: member
    As long as this doesn’t/can’t appear under regular, non assumeutxo usage, then this isn’t a blocker. If it can appear under any other circumstances, then it should be fixed, as it’ll just be a pointless/confusing log message.
  13. fjahr commented at 9:07 pm on October 19, 2023: contributor

    Removing from milestone. AU is experimental and test-only, so a log issue, whose discussion is still ongoing, is not a blocker for the release. If needed, it can be backported to 26.1?

    The problem is that without the change the log message is always being printed for every node of every user that has never even heard of assumeutxo/snapshots. So this is not a log issue specific to AU but a general log issue caused by AU.

  14. fanquake commented at 9:08 pm on October 19, 2023: member
    Ok. Putting it back on the milestone.
  15. fanquake added this to the milestone 26.0 on Oct 19, 2023
  16. theStack commented at 9:23 pm on October 19, 2023: contributor
    Concept ACK
  17. maflcko commented at 7:19 am on October 20, 2023: member
    I still don’t think that a single harmless log line in the debug log is a blocker for a release, but also I don’t care too much.
  18. fanquake commented at 7:32 am on October 20, 2023: member
    I just don’t see the point of shipping confusing log output, (coming from a test only feature that the user isn’t even using) when we can simply delete this line.
  19. glozow commented at 12:25 pm on October 20, 2023: member
    concept ACK b39fec7379d069d138c0e60cb41c86c7e84686fb
  20. DrahtBot requested review from theStack on Oct 20, 2023
  21. DrahtBot requested review from Sjors on Oct 20, 2023
  22. stickies-v approved
  23. stickies-v commented at 12:41 pm on October 20, 2023: contributor

    ACK b39fec7379d069d138c0e60cb41c86c7e84686fb

    Since we’re not logging consistently (e.g. this path is not logged), I think I’d be in favour of just removing both log lines until we implement it properly, though:

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index d4b30e397c..ab0eb1a6aa 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -5701,17 +5701,11 @@ void ChainstateManager::MaybeRebalanceCaches()
     5     assert(ibd_usable || snapshot_usable);
     6 
     7     if (ibd_usable && !snapshot_usable) {
     8-        if (m_snapshot_chainstate) {
     9-            // In normal mode, without a snapshot chainstate present, this log
    10-            // message is confusing.
    11-            LogPrintf("[snapshot] allocating all cache to the IBD chainstate\n");
    12-        }
    13         // Allocate everything to the IBD chainstate.
    14         m_ibd_chainstate->ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache);
    15     }
    16     else if (snapshot_usable && !ibd_usable) {
    17         // If background validation has completed and snapshot is our active chain...
    18-        LogPrintf("[snapshot] allocating all cache to the snapshot chainstate\n");
    19         // Allocate everything to the snapshot chainstate.
    20         m_snapshot_chainstate->ResizeCoinsCaches(m_total_coinstip_cache, m_total_coinsdb_cache);
    21     }
    

    Regardless, this is not worth holding things up so I’d be comfortable with either approach (or making both these log statements DEBUG).

  24. fjahr commented at 12:43 pm on October 20, 2023: contributor

    I still don’t think that a single harmless log line in the debug log is a blocker for a release, but also I don’t care too much.

    It’s not blocking the release if we merge it quickly :) It’s a very simple change, the review probably takes just as long as arguing about it here.

    I just don’t see the point of shipping confusing log output, (coming from a test only feature that the user isn’t even using) when we can simply delete this line.

    As said above, I am also happy to remove the line instead of adding the conditional if people prefer that. I would just like to the log in every node.

  25. fjahr force-pushed on Oct 20, 2023
  26. fjahr commented at 12:53 pm on October 20, 2023: contributor

    I have removed the line since it sounds like the preferred approach from @stickies-v and @fanquake and otherwise people feel indifferent.

    removing both log lines @stickies-v I have kept the second one in for now since this one isn’t hurting IMO. In the context of the snapshot being present it may be valuable to log this though I agree this can be made more consistent again in a follow-up.

  27. log: Don't log cache rebalancing in absense of a snapshot chainstate ec84f999f1
  28. fjahr force-pushed on Oct 20, 2023
  29. stickies-v approved
  30. stickies-v commented at 1:01 pm on October 20, 2023: contributor
    utACK ec84f999f1408b7f1ff4498f78c33b34c30e934c
  31. DrahtBot requested review from glozow on Oct 20, 2023
  32. glozow commented at 1:14 pm on October 20, 2023: member
    concept ACK ec84f999f1408b7f1ff4498f78c33b34c30e934c, don’t have opinions other than removing confusing log
  33. glozow requested review from fanquake on Oct 20, 2023
  34. theStack approved
  35. theStack commented at 1:21 pm on October 20, 2023: contributor
    utACK ec84f999f1408b7f1ff4498f78c33b34c30e934c
  36. fanquake merged this on Oct 20, 2023
  37. fanquake closed this on Oct 20, 2023

  38. Frank-GER referenced this in commit affbaff8d0 on Oct 21, 2023
  39. Sjors commented at 5:22 am on October 23, 2023: member
    Post merge lgtm
  40. bitcoin locked this on Oct 22, 2024

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-12-26 12:12 UTC

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