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-
fjahr commented at 11:41 am on October 3, 2023: contributorI 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.
-
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.
-
maflcko commented at 11:48 am on October 3, 2023: memberMissing
log:
prefix in title? -
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 -
DrahtBot added the label Utils/log/libs on Oct 3, 2023
-
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?
-
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.
-
fanquake added this to the milestone 26.0 on Oct 10, 2023
-
Sjors commented at 7:22 am on October 11, 2023: memberIf it can happen when loading an invalid snapshot then it’s probably fine to keep it. No strong feelings.
-
maflcko removed this from the milestone 26.0 on Oct 19, 2023
-
maflcko commented at 7:56 am on October 19, 2023: memberRemoving 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?
-
fanquake commented at 9:06 am on October 19, 2023: memberAs 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.
-
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.
-
fanquake commented at 9:08 pm on October 19, 2023: memberOk. Putting it back on the milestone.
-
fanquake added this to the milestone 26.0 on Oct 19, 2023
-
theStack commented at 9:23 pm on October 19, 2023: contributorConcept ACK
-
maflcko commented at 7:19 am on October 20, 2023: memberI 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.
-
fanquake commented at 7:32 am on October 20, 2023: memberI 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.
-
glozow commented at 12:25 pm on October 20, 2023: memberconcept ACK b39fec7379d069d138c0e60cb41c86c7e84686fb
-
DrahtBot requested review from theStack on Oct 20, 2023
-
DrahtBot requested review from Sjors on Oct 20, 2023
-
stickies-v approved
-
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).
-
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.
-
fjahr force-pushed on Oct 20, 2023
-
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.
-
log: Don't log cache rebalancing in absense of a snapshot chainstate ec84f999f1
-
fjahr force-pushed on Oct 20, 2023
-
stickies-v approved
-
stickies-v commented at 1:01 pm on October 20, 2023: contributorutACK ec84f999f1408b7f1ff4498f78c33b34c30e934c
-
DrahtBot requested review from glozow on Oct 20, 2023
-
glozow commented at 1:14 pm on October 20, 2023: memberconcept ACK ec84f999f1408b7f1ff4498f78c33b34c30e934c, don’t have opinions other than removing confusing log
-
glozow requested review from fanquake on Oct 20, 2023
-
theStack approved
-
theStack commented at 1:21 pm on October 20, 2023: contributorutACK ec84f999f1408b7f1ff4498f78c33b34c30e934c
-
fanquake merged this on Oct 20, 2023
-
fanquake closed this on Oct 20, 2023
-
Frank-GER referenced this in commit affbaff8d0 on Oct 21, 2023
-
Sjors commented at 5:22 am on October 23, 2023: memberPost merge lgtm
-
bitcoin locked this on Oct 22, 2024
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
More mirrored repositories can be found on mirror.b10c.me