mempool: apply rule of 5 to epochguard.h, fix compiler warnings #22481

jonatack commented at 12:06 AM on July 18, 2021: member

Apply the rule of five to src/util/epochguard.h::{Epoch, Marker} for safety, which also nicely fixes the -Wdeprecated-copy compiler warnings with Clang 13.

References:

<details><summary>Compiler warnings fixed</summary><p>

In file included from policy/rbf.cpp:5:
In file included from ./policy/rbf.h:8:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
        Marker& operator=(const Marker&) = delete;
                ^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
      ^
policy/rbf.cpp:29:29: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
    CTxMemPoolEntry entry = *pool.mapTx.find(tx.GetHash());

In file included from txmempool.cpp:6:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
        Marker& operator=(const Marker&) = delete;
                ^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit copy constructor for 'CTxMemPoolEntry' first required here
        { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
                             ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
          __a.construct(__p, std::forward<_Args>(__args)...);
              ^
/usr/include/boost/multi_index_container.hpp:655:24: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::hashed_index_node<boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::ordered_index_node<boost::multi_index::detail::null_augment_policy, boost::multi_index::detail::index_node_base<CTxMemPoolEntry, std::allocator<CTxMemPoolEntry>>>>>>>>>::construct<CTxMemPoolEntry, const CTxMemPoolEntry &>' requested here
    node_alloc_traits::construct(
                       ^
/usr/include/boost/multi_index/detail/index_base.hpp:108:15: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::construct_value' requested here
      final().construct_value(x,v);
              ^
/usr/include/boost/multi_index/detail/ord_index_impl.hpp:770:33: note: (skipping 5 contexts in backtrace; use -ftemplate-backtrace-limit=0 to see all)
    final_node_type* res=super::insert_(v,x,variant);
                                ^
/usr/include/boost/multi_index_container.hpp:693:33: note: in instantiation of function template specialization 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
    final_node_type* res=super::insert_(v,x,variant);
                                ^
/usr/include/boost/multi_index_container.hpp:705:12: note: in instantiation of function template specialization 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_<boost::multi_index::detail::lvalue_tag>' requested here
    return insert_(v,detail::lvalue_tag());
           ^
/usr/include/boost/multi_index/detail/index_base.hpp:213:21: note: in instantiation of member function 'boost::multi_index::multi_index_container<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>>::insert_' requested here
    {return final().insert_(x);}
                    ^
/usr/include/boost/multi_index/hashed_index.hpp:284:46: note: in instantiation of member function 'boost::multi_index::detail::index_base<CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>::final_insert_' requested here
    std::pair<final_node_type*,bool> p=this->final_insert_(x);
                                             ^
txmempool.cpp:363:53: note: in instantiation of member function 'boost::multi_index::detail::hashed_index<mempoolentry_txid, SaltedTxidHasher, std::equal_to<uint256>, boost::multi_index::detail::nth_layer<1, CTxMemPoolEntry, boost::multi_index::indexed_by<boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>, boost::multi_index::hashed_unique<boost::multi_index::tag<index_by_wtxid>, mempoolentry_wtxid, SaltedTxidHasher>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<descendant_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByDescendantScore>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<entry_time>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByEntryTime>, boost::multi_index::ordered_non_unique<boost::multi_index::tag<ancestor_score>, boost::multi_index::identity<CTxMemPoolEntry>, CompareTxMemPoolEntryByAncestorFee>>, std::allocator<CTxMemPoolEntry>>, boost::mpl::vector0<>, boost::multi_index::detail::hashed_unique_tag>::insert' requested here
    indexed_transaction_set::iterator newit = mapTx.insert(entry).first;

In file included from test/fuzz/policy_estimator.cpp:9:
In file included from ./test/fuzz/util.h:27:
In file included from ./txmempool.h:24:
./util/epochguard.h:53:17: warning: definition of implicit copy constructor for 'Marker' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
        Marker& operator=(const Marker&) = delete;
                ^
./txmempool.h:81:7: note: in implicit copy constructor for 'Epoch::Marker' first required here
class CTxMemPoolEntry
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/ext/new_allocator.h:150:23: note: in implicit move constructor for 'CTxMemPoolEntry' first required here
        { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
                             ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/alloc_traits.h:512:8: note: in instantiation of function template specialization '__gnu_cxx::new_allocator<CTxMemPoolEntry>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
          __a.construct(__p, std::forward<_Args>(__args)...);
              ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/vector.tcc:115:21: note: in instantiation of function template specialization 'std::allocator_traits<std::allocator<CTxMemPoolEntry>>::construct<CTxMemPoolEntry, CTxMemPoolEntry>' requested here
            _Alloc_traits::construct(this->_M_impl, this->_M_impl._M_finish,
                           ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/stl_vector.h:1204:9: note: in instantiation of function template specialization 'std::vector<CTxMemPoolEntry>::emplace_back<CTxMemPoolEntry>' requested here
      { emplace_back(std::move(__x)); }
        ^
test/fuzz/policy_estimator.cpp:49:37: note: in instantiation of member function 'std::vector<CTxMemPoolEntry>::push_back' requested here
                    mempool_entries.push_back(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));

</p></details>

DrahtBot added the label Mempool on Jul 18, 2021

DrahtBot added the label Utils/log/libs on Jul 18, 2021

JeremyRubin commented at 4:46 AM on July 18, 2021: contributor

concept ack; i think you missed one of the defs you were planning to do.

jonatack commented at 7:35 AM on July 18, 2021: member

Thanks @JeremyRubin. I defined the copy constructor and the default constructor; the copy assignment operator is already defined. The move constructor and the move assignment operator (rule of 5) aren't needed for now, though they can be added.

vasild approved

vasild commented at 10:22 AM on July 20, 2021: member

ACK 8fc52af31199496d86a1b300f25826e89322d779

Thanks for fixing this! Now I can remove -Wno-deprecated-copy from my build environment.

I would go with the rule of 5 for both Epoch and Marker. If nobody is using the move assignment operator (constructor), then now is a good time to = delete; it since the intention seems to be // only allow modification via Epoch member functions. Why virtual Epoch destructor in the OP?

cc @ajtowns the code originated in https://github.com/bitcoin/bitcoin/pull/18017

mempool: apply rule of 5 to epochguard.h, fix compiler warnings 7b3a20b260

jonatack force-pushed on Jul 20, 2021

jonatack renamed this:
~~mempool: apply rule of 3 to epochguard.h, fix compiler warnings~~
mempool: apply rule of 5 to epochguard.h, fix compiler warnings
on Jul 20, 2021

jonatack commented at 12:25 PM on July 20, 2021: member

@vasild thanks! Done; upgraded from the minimum rule of 3 change needed to quiet the compiler, to rule of five.

vasild approved

vasild commented at 9:49 AM on July 22, 2021: member

ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc

laanwj commented at 12:53 PM on July 22, 2021: member

i think we should at least implement the rule of 7 here !!!

Code review ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc

laanwj merged this on Jul 22, 2021

laanwj closed this on Jul 22, 2021

jonatack deleted the branch on Jul 22, 2021

in src/util/epochguard.h:44 in 7b3a20b260

  39 | @@ -40,6 +40,9 @@ class LOCKABLE Epoch
  40 |      Epoch() = default;
  41 |      Epoch(const Epoch&) = delete;
  42 |      Epoch& operator=(const Epoch&) = delete;
  43 | +    Epoch(Epoch&&) = delete;
  44 | +    Epoch& operator=(Epoch&&) = delete;

MarcoFalke commented at 3:09 PM on July 22, 2021:

The && constructors should already be deleted, no?

the presence of a user-defined destructor, copy-constructor, or copy-assignment operator prevents implicit definition of the move constructor and the move assignment operator

From the page you linked (https://en.cppreference.com/w/cpp/language/rule_of_three#Rule_of_five)

And quoting from your error message this should be the case: "... has a user-declared copy assignment operator".

MarcoFalke commented at 3:18 PM on July 22, 2021:

Ah ok, the other pages you linked to seem to always prefer all 5 (or zero)

in src/util/epochguard.h:62 in 7b3a20b260

  53 | @@ -51,6 +54,13 @@ class LOCKABLE Epoch
  54 |          // only allow modification via Epoch member functions
  55 |          friend class Epoch;
  56 |          Marker& operator=(const Marker&) = delete;
  57 | +
  58 | +    public:
  59 | +        Marker() = default;
  60 | +        Marker(const Marker&) = default;
  61 | +        Marker(Marker&&) = delete;
  62 | +        Marker& operator=(Marker&&) = delete;

MarcoFalke commented at 3:09 PM on July 22, 2021:

Same

sidhujag referenced this in commit 50d5494282 on Jul 23, 2021

practicalswift commented at 9:09 PM on July 24, 2021: contributor

Suggested follow-up: could perhaps add a developer note about "rule of […]" to make expectations clear for new code and new contributors? :)

vasild commented at 7:59 AM on July 26, 2021: member

That is just one of many C++ good practices.

Fabcien referenced this in commit 118a1c1fee on Apr 8, 2022

DrahtBot locked this on Aug 18, 2022