Add public Boost headers explicitly #27783

Why are they in the header when they can be in the cpp file?

(Edit: NVM. They are included via txmempool.h either way, so while they can be moved to the cpp file, it doesn't matter)

$ git grep -l boost::multi_index::tag
src/node/miner.h
src/txmempool.h
src/txrequest.cpp

The symbol is used in the header. IWYU agree :)

I meant moving everything to the cpp file, but they are included via txmempool.h either way, so while everything can be moved to the cpp file, it doesn't matter.

what is the point in adding it and using it over just assert or Assert or BOOST_REQUIRE?

I see no point at all. Is it OK to put this change into this PR ?

Or I can just revert the recent change?

Feel free to take:

diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index a5394d8816..b1d67c1432 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -432,7 +432,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         CAmount selection_target = 16 * CENT;
         const auto& no_res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs*/true),
                                             selection_target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT);
-        BOOST_ASSERT(!no_res);
+        BOOST_REQUIRE(!no_res);
         BOOST_CHECK(util::ErrorString(no_res).original.find("The inputs size exceeds the maximum weight") != std::string::npos);
 
         // Now add same coin value with a good size and check that it gets selected
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 194c8663db..a376d4e5df 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -955,11 +955,10 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup)
     }
 
     // Add tx to wallet
-    const auto& op_dest = wallet.GetNewDestination(OutputType::BECH32M, "");
-    BOOST_ASSERT(op_dest);
+    const auto op_dest{*Assert(wallet.GetNewDestination(OutputType::BECH32M, ""))};
 
     CMutableTransaction mtx;
-    mtx.vout.push_back({COIN, GetScriptForDestination(*op_dest)});
+    mtx.vout.push_back({COIN, GetScriptForDestination(op_dest)});
     mtx.vin.push_back(CTxIn(g_insecure_rand_ctx.rand256(), 0));
     const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash();
 
diff --git a/src/wallet/test/walletdb_tests.cpp b/src/wallet/test/walletdb_tests.cpp
index 00b2f47e14..17b6c4f7ed 100644
--- a/src/wallet/test/walletdb_tests.cpp
+++ b/src/wallet/test/walletdb_tests.cpp
@@ -39,7 +39,7 @@ BOOST_AUTO_TEST_CASE(walletdb_read_write_deadlock)
         DatabaseStatus status;
         bilingual_str error_string;
         std::unique_ptr<WalletDatabase> db = MakeDatabase(m_path_root / strprintf("wallet_%d_.dat", db_format).c_str(), options, status, error_string);
-        BOOST_ASSERT(status == DatabaseStatus::SUCCESS);
+        BOOST_CHECK_EQUAL(status, DatabaseStatus::SUCCESS);
 
         std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", std::move(db)));
         wallet->m_keypool_size = 4;
diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp
index 6823eafdfa..ab1184556c 100644
--- a/src/wallet/test/walletload_tests.cpp
+++ b/src/wallet/test/walletload_tests.cpp
@@ -169,7 +169,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_ckey, TestingSetup)
         // Fourth test case:
         // Verify that loading up a 'ckey' with an invalid pubkey throws an error
         CPubKey invalid_key;
-        BOOST_ASSERT(!invalid_key.IsValid());
+        BOOST_CHECK(!invalid_key.IsValid());
         SerializeData key = MakeSerializeData(DBKeys::CRYPTED_KEY, invalid_key);
         records[key] = ckey_record_value;
 

Feel free to take:

Taken. Thank you!

I would expect to see boost/multi_index_container.hpp removed from this list as part of this PR, but it looks like it's still used:

 $ git grep multi_index_container.hpp origin/pr/27783
origin/pr/27783:node/miner.h:#include <boost/multi_index_container.hpp>
origin/pr/27783:txmempool.h:#include <boost/multi_index_container.hpp>
origin/pr/27783:txrequest.cpp:#include <boost/multi_index_container.hpp>

Is there some complication with those 3 that prevents us from using the specific headers rather than the umbrella one?

Nevermind, I misremembered how multi_index_container.hpp worked. I was thinking it was just a dumb list of other includes, but it's more than that.

Still, does using multi_index_container_fwd.hpp help at all?

Still, does using multi_index_container_fwd.hpp help at all?

No, it doesn't:

./txmempool.h:406:29: error: field ‘mapTx’ has incomplete type ‘CTxMemPool::indexed_transaction_set’ {aka ‘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> > >’}
  406 |     indexed_transaction_set mapTx GUARDED_BY(cs);
      |