coins: Add move operations to Coin and CCoinsCacheEntry

l0rinc commented at 9:14 pm on August 12, 2024: contributor

Related to #28280 (review), the CI build had a Sonar warning, namely:

“std::move” should not be called on an object of a type with neither move constructor nor move-assignment operator.

Added defaulted move constructor and move assignment operator to CCoinsCacheEntry. And to make sure the mentioned Sonar warnings aren’t triggered by CI anymore, I’ve modernized the call-site minimally.

Based on comments I’ve also added explicit special member functions to Coin.

DrahtBot commented at 9:14 pm on August 12, 2024: 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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30906 (refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests 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.

DrahtBot added the label UTXO Db and Indexes on Aug 12, 2024

andrewtoth commented at 9:22 pm on August 12, 2024: contributor

Don’t we also need to add move constructor to Coin? Its prevector is the only thing that really benefits from being moved I believe.

maflcko commented at 6:09 am on August 13, 2024: member

Sonar

They are mostly just a clang-tidy wrapper, so I wonder if there is a clang-tidy warning that can be used here instead.

l0rinc commented at 6:34 am on August 13, 2024: contributor

I wonder if there is a clang-tidy warning

I have installed Sonar locally, reproduced the warning first, it doesn’t appear anymore afer the change.

Don’t we also need to add move constructor to Coin?

It seems to me that the Coin class already has an explicit move constructor that moves the CTxOut member - and the rest are trivially movable (i.e. bit-copy of unsigned int and uint32_t).

0    Coin(CTxOut&& outIn, int nHeightIn, bool fCoinBaseIn) : out(std::move(outIn)), fCoinBase(fCoinBaseIn), nHeight(nHeightIn) {}

The CTxOut class itself does not have an explicitly defined move constructor or move assignment operator, though it can rely on the compiler-generated defaults (since CAmount = int64_t, which is trivially movable). CScript itself does not explicitly declare any additional members beyond what it inherits from CScriptBase (and prevector does have move a constructor), so I think the compiler can automatically generate a default move constructor for CScript.

If you suspect I’m wrong here, let me know where you’d like me to investigate further.

l0rinc marked this as ready for review on Aug 13, 2024

fanquake commented at 8:29 am on August 13, 2024: member

reproduded the warning first, it doesn’t appear anymore afer the change.

Can you add any relevant output to the PR description.

andrewtoth commented at 2:42 pm on August 13, 2024: contributor

I wonder if there is a clang-tidy warning that can be used here instead.

I have installed Sonar locally, reproduced the warning first, it doesn’t appear anymore afer the change.

I think what is being suggested here is to configure clang-tidy to produce this warning in our own CI instead of Sonar, which can then be shown to be fixed in this PR. I think that would be a more valuable change.

It seems to me that the Coin class already has an explicit move constructor that moves the CTxOut member

That would not be an explicit move constructor, right? That is a constructor that has one of its members moved in. An explicit move constructor would be Coin(Coin&& coin).

so I think the compiler can automatically generate a default move constructor for CScript.

How can we be sure that this happens?

sipa commented at 2:48 pm on August 13, 2024: member

How can we be sure that this happens?

A class has an implicitly-declared move constructor whenever:

There are no user-declared copy constructors.
There are no user-declared copy assignment operators.
There are no user-declared move constructors.
There is no user-declared destructor

(from https://en.cppreference.com/w/cpp/language/move_constructor)

Personally, I find these rules too inscrutable to judge from a quick glance at a class, so if I want a class that is guaranteed to have a move constructor, I just add one explicitly:

0class Coin
1{
2    ...
3    Coin(Coin&&) noexcept = default;
4    ...
5};

l0rinc force-pushed on Aug 14, 2024

l0rinc renamed this:
~~coins: Add move operations to CCoinsCacheEntry~~
coins: Add move operations to Coin and CCoinsCacheEntry
on Aug 14, 2024

l0rinc force-pushed on Aug 14, 2024

l0rinc commented at 3:09 pm on August 14, 2024: contributor

configure clang-tidy to produce this warning in our own CI instead of Sonar

The [“coverage report”]( #30643 (comment)) already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck CI Sonar warnings. Or do you mean we should be able to reproduce this locally as well via clang-tidy?

if I want a class that is guaranteed to have a move constructor, I just add one explicitly

Done for Coin as well, thanks.

maflcko commented at 3:15 pm on August 14, 2024: member

configure clang-tidy to produce this warning in our own CI instead of Sonar

The “coverage report” already has the Sonarcloud section - I have opened this issue because of the #28280 corecheck CI Sonar warnings. Or do you mean we should be able to reproduce this locally as well via clang-tidy?

if I want a class that is guaranteed to have a move constructor, I just add one explicitly

Done dor Coin as well, thanks.

For testing locally, one can add a static assert, similar to src/validation.h:static_assert(std::is_nothrow_move_constructible_v<CScriptCheck>);. (This is also an alternative to adding move constructors explicitly).

I was mostly wondering about other cases, which would be nice to be able to find by clang-tidy, but that is probably only tangentially related to this pull.

sipa commented at 3:17 pm on August 14, 2024: member

@maflcko etd:is_nothrow_move_constructible_v will also return true if a nothrow copy constructor is available (because a copy constructor is eligible in any context where a move constructor is called), so that isn’t enough to verify that an efficient version exists.

in src/coins.h:49 in 945256d549 outdated

42@@ -43,7 +43,12 @@ class Coin
43 
44     //! construct a Coin from a CTxOut and height/coinbase information.
45     Coin(CTxOut&& outIn, int nHeightIn, bool fCoinBaseIn) : out(std::move(outIn)), fCoinBase(fCoinBaseIn), nHeight(nHeightIn) {}
46-    Coin(const CTxOut& outIn, int nHeightIn, bool fCoinBaseIn) : out(outIn), fCoinBase(fCoinBaseIn),nHeight(nHeightIn) {}
47+    Coin(const CTxOut& outIn, int nHeightIn, bool fCoinBaseIn) : out(outIn), fCoinBase(fCoinBaseIn), nHeight(nHeightIn) {}
48+
49+    Coin(Coin&& other) noexcept = default;
50+    Coin& operator=(Coin&&) = default;

maflcko commented at 2:42 pm on August 15, 2024:

Missing noexcept?

l0rinc commented at 4:44 pm on August 15, 2024:

Fixed, thanks!

l0rinc force-pushed on Aug 15, 2024

DrahtBot added the label CI failed on Sep 7, 2024

DrahtBot removed the label CI failed on Sep 9, 2024

achow101 requested review from theuni on Oct 15, 2024

achow101 commented at 3:21 pm on October 15, 2024: member

cc @davidgumberg @andrewtoth

in src/coins.h:51 in 56515256d3 outdated

47+    Coin(const CTxOut& outIn, int nHeightIn, bool fCoinBaseIn) : out(outIn), fCoinBase(fCoinBaseIn), nHeight(nHeightIn) {}
48+
49+    Coin(Coin&& other) noexcept = default;
50+    Coin& operator=(Coin&&) noexcept = default;
51+    Coin(const Coin&) noexcept = default;
52+    Coin& operator=(const Coin&) noexcept = default;

davidgumberg commented at 2:24 pm on October 21, 2024:

Is the reason that all of these default constructors can be declared noexcept that all of Coin’s members’ respective default constructors that get invoked are noexcept?

sipa commented at 2:31 pm on October 21, 2024:

According to https://en.cppreference.com/w/cpp/language/noexcept_spec:

default constructors, copy constructors, move constructors that are implicitly-declared or defaulted on their first declaration unless

a constructor for a base or member that the implicit definition of the constructor would call is potentially-throwing (see below)

a subexpression of such an initialization, such as a default argument expression, is potentially-throwing (see below)

a default member initializer (for default constructor only) is potentially-throwing (see below)

So these noexcept specifiers here aren’t technically necessary. Doesn’t hurt to be explicit though, IMO.

in src/coins.h:166 in 56515256d3 outdated

157@@ -153,6 +158,13 @@ struct CCoinsCacheEntry
158     };
159 
160     CCoinsCacheEntry() noexcept = default;
161+
162+    // No implicit copying, only moves.
163+    CCoinsCacheEntry(CCoinsCacheEntry&&) noexcept = default;
164+    CCoinsCacheEntry& operator=(CCoinsCacheEntry&&) noexcept = default;
165+    CCoinsCacheEntry(const CCoinsCacheEntry&) noexcept = delete;
166+    CCoinsCacheEntry& operator=(const CCoinsCacheEntry&) noexcept = delete;

davidgumberg commented at 6:55 pm on October 23, 2024:

nit: deleted functions don’t need noexcept

l0rinc commented at 7:03 pm on October 23, 2024:

Pushed, thank you

l0rinc force-pushed on Oct 23, 2024

coins: Add move operations to CCoinsCacheEntry

Add defaulted move constructor and move assignment operator to CCoinsCacheEntry.
This explicitly enables move semantics, satisfying static analysis checks.
Also disabled copy constructor since copying doesn't make sense in current setup.

f1b0607aeb

coins: Use try_emplace in InsertCoinsMapEntry

Replace map.emplace with map.try_emplace and simplify related code block.

127458e84b

coins: Add explicit move operations to Coin db91623c52

l0rinc force-pushed on Oct 23, 2024

davidgumberg commented at 7:16 pm on October 23, 2024: contributor

Am I right in understanding that this PR’s expected changes are:

Deletion of the CCoinsCacheEntry reference copy constructors
Update CoinsMapEntry in the coins unit tests to try_emplace over emplace.

And we are otherwise just explicitly declaring the move constructors that we expect to already be used implicitly in order to silence a spurious CI warning?

Also, could you point me in the direction of reproducing this CI warning locally? I can’t find the warning on corecheck (https://corecheck.dev/bitcoin/bitcoin/pulls/28280)

l0rinc commented at 7:25 pm on October 23, 2024: contributor

Thanks for checking @davidgumberg!

Also, could you point me in the direction of reproducing this CI warning locally? I can’t find the warning on corecheck (corecheck.dev/bitcoin/bitcoin/pulls/28280)

I also had a hard time reproducing it on CI (sonarcould seems to have expired), but installed Sonar locally which gave the same warning before the change. But the before state is not as important as the after state: we need to move Coin and CCoinsCacheEntry so an explicit constructor is better.

Deletion of the CCoinsCacheEntry reference copy constructors

This is just a safety measure

explicitly declaring the move constructors that we expect to already be used implicitly

Either that or adding a move constructor, since it’s needed (regardless of whether it was there implicitly or not)

Update CoinsMapEntry in the coins unit tests to try_emplaceoveremplace`

This was added to have an actual user of the (new?) move constructor

coins: Add move operations to Coin and CCoinsCacheEntry #30643

Code Coverage

Reviews

Conflicts