refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests

[l0rinc]

Similarly to #30849, this cleanup is intended to de-risk #30673 (comment) by simplifying the coin cache public interface.

CCoinsCacheEntry provided general access to its internal flags state, even though, in reality, it could only be clean, fresh, dirty, or fresh|dirty (in the follow-up, we will remove fresh without dirty).

Once it was marked as dirty, we couldn’t set the state back to clean with AddFlags(0)—tests explicitly checked against that.

This PR refines the public interface to make this distinction clearer and to make invalid behavior impossible, rather than just checked by tests. We don't need extensive access to the internals of CCoinsCacheEntry, as many tests were simply validating invalid combinations in this way.

The last few commits contain significant test refactorings to make coins_tests easier to change in follow-ups.

[DrahtBot]

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/30906.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	laanwj, ryanofsky, andrewtoth
Concept ACK	davidgumberg, hodlinator

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:

• #31308 (ci, iwyu: Treat warnings as errors for specific targets by hebasto)
• #31306 (ci: Update Clang in "tidy" job by hebasto)
• #31132 (validation: fetch block inputs on parallel threads 10% faster IBD by andrewtoth)
• #30643 (coins: Add move operations to Coin and CCoinsCacheEntry by l0rinc)

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.

[andrewtoth]

Concept ACK

Can we remove any other tests that are trying to add flags that are not FRESH or DIRTY and so are now useless?

[l0rinc]

Can we remove any other tests that are trying to add flags that are not FRESH or DIRTY and so are now useless?

I've merged NO_ENTRY and char(0) values - this simplifies the cases somewhat.
Do you think we can remove any of the lines? There aren't any duplicates, even after the above change...

Edit: moved the booleans closer to the edges in later commits.

[davidgumberg]

Concept ACK on dropping the bitfield interface for CCoinsCacheEntry flags

[hodlinator]

Concept ACK.

Really like the move to put restrictions on how flags are used.

[andrewtoth]

I think the commit title test: Migrate GetCoinsMapEntry to return std::optional<CoinState> is no longer accurate.

I didn't find splitting the changes in coins_tests up into that many commits to be helpful for review. I ended up reviewing the combined diff for that file. Will review in more depth later.

[hodlinator]

Using const for value type parameters (CAmount is a typedef of int64_t), when a function doesn't contain any logic is noisy to me and not used in the codebase at large.

Suggested change

	constexpr CoinEntry(const CAmount v, const State s) : value{v}, state{s} {}
	constexpr CoinEntry(CAmount v, State s) : value{v}, state{s} {}

It is useful in cases like this where the function is actually doing something:

bitcoin/src/uint256.cpp

Lines 20 to 44 in ebe4cac

	template <unsigned int BITS>
	void base_blob<BITS>::SetHexDeprecated(const std::string_view str)
	{
	std::fill(m_data.begin(), m_data.end(), 0);
	const auto trimmed = util::RemovePrefixView(util::TrimStringView(str), "0x");
	// Note: if we are passed a greater number of digits than would fit as bytes
	// in m_data, we will be discarding the leftmost ones.
	// str="12bc" in a WIDTH=1 m_data => m_data[] == "\0xbc", not "0x12".
	size_t digits = 0;
	for (const char c : trimmed) {
	if (::HexDigit(c) == -1) break;
	++digits;
	}
	unsigned char* p1 = m_data.data();
	unsigned char* pend = p1 + WIDTH;
	while (digits > 0 && p1 < pend) {
	*p1 = ::HexDigit(trimmed[--digits]);
	if (digits > 0) {
	*p1 |= ((unsigned char)::HexDigit(trimmed[--digits]) << 4);
	p1++;
	}
	}
	}

(Edit: I realize that string_view isn't strictly a value-type. If one greps for "(const uint64_t " one gets 5 hits, while "(uint64_t " gives 212).

Same goes for ToState, SetCoinsValue, SingleEntryCacheTest, CheckAccessCoin etc.

Otherwise you might as well add const to AddFlags(uint8_t flags, ... etc?

[l0rinc]

I prefer const here, my IDE was also asking for them, I don't mind adding them here.
Being noisy when a function doesn't contain any logic doesn't sound like a blocker to me :)

[hodlinator]

This issue is a non-nit for me as a general pattern as it will affect diffs & reviews going forward.

We've got a ratio of 5/212 or roughly 1:40 for this codebase. I'm surprised that the IDE is encouraging that.

What is you rationale for not applying it to AddFlags? Did the specific version of your IDE not tell you to do so? Does your IDE have a documented rationale?

If there is a clear rationale, ideally with a statistical sample of % bugs discovered from changing to const value type parameters, I'm open to that. But the 1:40 ratio weighs heavily.

[l0rinc]

We can unify these in a separate PR, I don't mind, I added these since the linters were complaining - and I was fine with either option since it doesn't affect the body of the function signal-to-noise-ratio-wise.

[hodlinator]

9edbe5a48fe9a2c0d364882fcb7d44ae6aac6d24 before the latest push didn't have these const params. I think it was a mistake to add them in the latest push and would prefer them rolled back.

Another alternative is to discuss my samples of ratios of const value type params or why you didn't apply it to AddFlags.

Sorry to be annoying about this but for some reason it really annoys me. :)
Will take a timeout and see if I can let go of this, if you still insist.

[hodlinator]

<string> is still needlessly added in c0b4b2c.

[ryanofsky]

Code review ACK 50cce20. Looks good! Thanks for the followups.

Since last review, coins.h commits were reorganized to minimize diffs, but only overall change was to drop redundant inline keywords. In coins_tests.cpp a lot of smaller changes were made like adding const to amount parameters (which is not great but ok), using more brace initialization, changing CheckAddCoin parameter order, and fixing up some comments.

[andrewtoth]

Code Review ACK 50cce20

Only behavior change from last review seems to be the new check in CheckAccessCoin.

I'm not sure why inlines needed to be dropped.

[ryanofsky]

Will merge this soon since it has 3 ACKs and mostly consists of test changes and a few straightforward cleanups in coins.h/coins.cpp.

I think the only part of this that might deserve some extra scrutiny is commit 6b73369 which sets no longer used pointers to null and adds Assume calls to check that they are null. Reviewers also seemed to disagree about const and inline changes, but these are minor changes and clang-tidy is able to check for them so they could be revisited again in a broader context:

https://clang.llvm.org/extra/clang-tidy/checks/readability/avoid-const-params-in-decls.html
https://clang.llvm.org/extra/clang-tidy/checks/readability/redundant-inline-specifier.html

[davidgumberg]

post-merge ACK 50cce20

This refactor reduces the risk of CCoinsCacheEntry's interfaces, and making CCoinsCacheEntry::AddFlags() static is a big improvement for readability (and safety).

I did not thoroughly review the refactors to unit tests, but they seem correct to me.

More broadly, I think the changes made here to SetClean() are good, but SetClean() being public (only used externally by NextAndMaybeErase()) seems dangerous to me. The invariant that the code currently respects is that a coin's state has been written to the parent/backing view before NextAndMaybeErase()->SetClean(), so removing DIRTY and FRESH flags are fine, but is it risky to have an interface that doesn't enforce that?

[hodlinator]

I stand by my refusal to fully ACK based off the unrequested addition of const parameter noise in the last push, but aside from that the PR is good.

Follow-up idea: Noticed CCoinsCacheEntry::Flags can be made private now that CoinEntry in the test has its own enum.

[l0rinc]

the changes made here to SetClean() are good, but SetClean() being public (only used externally by NextAndMaybeErase()) seems dangerous to me.

Can you provide a PR for it? We need to clean up this area in tiny steps, if you can objectively make it better, we'll review it.

Follow-up idea: Noticed CCoinsCacheEntry::Flags can be made private now that CoinEntry in the test has its own enum.

Seems you had strong preferences in other areas as well, follow-up PRs are welcome.

[hodlinator]

Follow-up idea: Noticed CCoinsCacheEntry::Flags can be made private now that CoinEntry in the test has its own enum.

..., follow-up PRs are welcome.

#31496