Add public Boost headers explicitly

hebasto commented at 5:14 pm on May 30, 2023: member

To check symbols in the code base, run:

0git grep boost::multi_index::identity
1git grep boost::multi_index::indexed_by
2git grep boost::multi_index::tag
3git grep boost::make_tuple

Hoping on the absence of conflicts with top-prio PRs :)

DrahtBot commented at 5:14 pm on May 30, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke, TheCharlatan

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:

#27425 (test: move remaining rand code from util/setup_common to util/random by jonatack)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)

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.

in src/node/miner.h:21 in 374a18c565 outdated

13@@ -14,7 +14,10 @@
14 #include <optional>
15 #include <stdint.h>
16 
17+#include <boost/multi_index/identity.hpp>
18+#include <boost/multi_index/indexed_by.hpp>
19 #include <boost/multi_index/ordered_index.hpp>
20+#include <boost/multi_index/tag.hpp>
21 #include <boost/multi_index_container.hpp>

maflcko commented at 5:24 pm on May 30, 2023:

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)

hebasto commented at 5:40 pm on May 30, 2023:

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

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

maflcko commented at 5:45 pm on May 30, 2023:

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.

hebasto force-pushed on May 30, 2023

DrahtBot added the label CI failed on May 30, 2023

in test/lint/lint-includes.py:24 in 36338b6cc5 outdated

20@@ -21,17 +21,23 @@
21                  "src/minisketch/",
22                 ]
23 
24-EXPECTED_BOOST_INCLUDES = ["boost/date_time/posix_time/posix_time.hpp",
25+EXPECTED_BOOST_INCLUDES = ["boost/assert.hpp",

maflcko commented at 5:42 pm on May 30, 2023:

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

hebasto commented at 5:43 pm on May 30, 2023:

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

hebasto commented at 5:44 pm on May 30, 2023:

Or I can just revert the recent change?

maflcko commented at 6:53 am on May 31, 2023:

Feel free to take:

 0diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
 1index a5394d8816..b1d67c1432 100644
 2--- a/src/wallet/test/coinselector_tests.cpp
 3+++ b/src/wallet/test/coinselector_tests.cpp
 4@@ -432,7 +432,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
 5         CAmount selection_target = 16 * CENT;
 6         const auto& no_res = SelectCoinsBnB(GroupCoins(available_coins.All(), /*subtract_fee_outputs*/true),
 7                                             selection_target, /*cost_of_change=*/0, MAX_STANDARD_TX_WEIGHT);
 8-        BOOST_ASSERT(!no_res);
 9+        BOOST_REQUIRE(!no_res);
10         BOOST_CHECK(util::ErrorString(no_res).original.find("The inputs size exceeds the maximum weight") != std::string::npos);
11 
12         // Now add same coin value with a good size and check that it gets selected
13diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
14index 194c8663db..a376d4e5df 100644
15--- a/src/wallet/test/wallet_tests.cpp
16+++ b/src/wallet/test/wallet_tests.cpp
17@@ -955,11 +955,10 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup)
18     }
19 
20     // Add tx to wallet
21-    const auto& op_dest = wallet.GetNewDestination(OutputType::BECH32M, "");
22-    BOOST_ASSERT(op_dest);
23+    const auto op_dest{*Assert(wallet.GetNewDestination(OutputType::BECH32M, ""))};
24 
25     CMutableTransaction mtx;
26-    mtx.vout.push_back({COIN, GetScriptForDestination(*op_dest)});
27+    mtx.vout.push_back({COIN, GetScriptForDestination(op_dest)});
28     mtx.vin.push_back(CTxIn(g_insecure_rand_ctx.rand256(), 0));
29     const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash();
30 
31diff --git a/src/wallet/test/walletdb_tests.cpp b/src/wallet/test/walletdb_tests.cpp
32index 00b2f47e14..17b6c4f7ed 100644
33--- a/src/wallet/test/walletdb_tests.cpp
34+++ b/src/wallet/test/walletdb_tests.cpp
35@@ -39,7 +39,7 @@ BOOST_AUTO_TEST_CASE(walletdb_read_write_deadlock)
36         DatabaseStatus status;
37         bilingual_str error_string;
38         std::unique_ptr<WalletDatabase> db = MakeDatabase(m_path_root / strprintf("wallet_%d_.dat", db_format).c_str(), options, status, error_string);
39-        BOOST_ASSERT(status == DatabaseStatus::SUCCESS);
40+        BOOST_CHECK_EQUAL(status, DatabaseStatus::SUCCESS);
41 
42         std::shared_ptr<CWallet> wallet(new CWallet(m_node.chain.get(), "", std::move(db)));
43         wallet->m_keypool_size = 4;
44diff --git a/src/wallet/test/walletload_tests.cpp b/src/wallet/test/walletload_tests.cpp
45index 6823eafdfa..ab1184556c 100644
46--- a/src/wallet/test/walletload_tests.cpp
47+++ b/src/wallet/test/walletload_tests.cpp
48@@ -169,7 +169,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_load_ckey, TestingSetup)
49         // Fourth test case:
50         // Verify that loading up a 'ckey' with an invalid pubkey throws an error
51         CPubKey invalid_key;
52-        BOOST_ASSERT(!invalid_key.IsValid());
53+        BOOST_CHECK(!invalid_key.IsValid());
54         SerializeData key = MakeSerializeData(DBKeys::CRYPTED_KEY, invalid_key);
55         records[key] = ckey_record_value;
56

hebasto commented at 2:44 pm on May 31, 2023:

Feel free to take:

Taken. Thank you!

in test/lint/lint-includes.py:30 in 36338b6cc5 outdated

27                            "boost/multi_index/hashed_index.hpp",
28+                           "boost/multi_index/identity.hpp",
29+                           "boost/multi_index/indexed_by.hpp",
30                            "boost/multi_index/ordered_index.hpp",
31                            "boost/multi_index/sequenced_index.hpp",
32+                           "boost/multi_index/tag.hpp",

theuni commented at 9:14 pm on May 30, 2023:

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:

0 $ git grep multi_index_container.hpp origin/pr/27783
1origin/pr/27783:node/miner.h:#include <boost/multi_index_container.hpp>
2origin/pr/27783:txmempool.h:#include <boost/multi_index_container.hpp>
3origin/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?

theuni commented at 9:18 pm on May 30, 2023:

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?

hebasto commented at 2:24 pm on May 31, 2023:

Still, does using multi_index_container_fwd.hpp help at all?

No, it doesn’t:

0./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> > >’}
1  406 |     indexed_transaction_set mapTx GUARDED_BY(cs);
2      |

DrahtBot removed the label CI failed on May 31, 2023

test: Avoid `BOOST_ASSERT` macro

The `BOOST_ASSERT` macro is defined in the `boost/assert.hpp` header.
This change allows to skip `#include <boost/assert.hpp>`.

fade2adb5b

Add public Boost headers explicitly 2484cacb7a

hebasto force-pushed on May 31, 2023

hebasto commented at 2:44 pm on May 31, 2023: member

Taken @MarcoFalke’s suggestion.

maflcko commented at 6:32 pm on May 31, 2023: member

lgtm ACK 2484cacb7a6367b24e924dba0825c843b1dfc1c3

TheCharlatan approved

TheCharlatan commented at 7:28 am on June 5, 2023: contributor

ACK 2484cacb7a6367b24e924dba0825c843b1dfc1c3

fanquake merged this on Jun 12, 2023

fanquake closed this on Jun 12, 2023

hebasto deleted the branch on Jun 12, 2023

sidhujag referenced this in commit c951edbdf0 on Jun 15, 2023

fanquake referenced this in commit 2880bb588a on Jun 22, 2023

sidhujag referenced this in commit 1d1ed98bef on Jun 22, 2023

bitcoin locked this on Jun 11, 2024

Add public Boost headers explicitly #27783

Reviews

Conflicts