log: Don't log cache rebalancing in absense of a snapshot chainstate

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.

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: member

Missing 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: member

If 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: 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?

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.

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: member

Ok. 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: contributor

Concept ACK

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.

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.

glozow commented at 12:25 pm on October 20, 2023: member

concept 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: contributor

utACK ec84f999f1408b7f1ff4498f78c33b34c30e934c

DrahtBot requested review from glozow on Oct 20, 2023

glozow commented at 1:14 pm on October 20, 2023: member

concept 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: contributor

utACK 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: member

Post merge lgtm

bitcoin locked this on Oct 22, 2024

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

Code Coverage

Reviews

Conflicts