faca73000fa8975c28f6be8be01957c1ae94ea62 ci: Install fixed version of clang-format for linters (MarcoFalke)
fa4695da4c69646b58a8fa0b6b30146bb234deb8 build: Sort Makefile.am after renaming file (MarcoFalke)
cccc2784a3bb10fa8e43be7e68207cafb12bd915 scripted-diff: Move ui_interface to the node lib (MarcoFalke)
fa72ca6a9d90d66012765b0043fd819698b94ba8 qt: Remove unused includes (MarcoFalke)
fac96e6450d595fe67168cb7afa7692da6cc9973 wallet: Do not include server symbols (MarcoFalke)
fa0f6c58c1c6d10f04c4e65a424cc51ebca50a8c Revert "Fix link error with --enable-debug" (MarcoFalke)
Pull request description:
This reverts a hacky workaround from commit b83cc0f, which only happens to work due to compiler optimizations. Then, it actually fixes the linker error.
The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures. Though, in this specific case the linker failures have not been observed (unless `-O0`) because our compilers were smart enough to strip unused symbols.
Fix the underlying problem by creating a new header-only with the needed symbol and move ui_interface to node to clarify that this is part of libbitcoin_server.
ACKs for top commit:
Sjors:
ACK faca730
laanwj:
ACK faca73000fa8975c28f6be8be01957c1ae94ea62
hebasto:
re-ACK faca73000fa8975c28f6be8be01957c1ae94ea62, since the [previous](https://github.com/bitcoin/bitcoin/pull/19331#pullrequestreview-434420539) review:
Tree-SHA512: e9731f249425aaea50b6db5fc7622e10078cf006721bb87989cac190a2ff224412f6f8a7dd83efd018835302337611f5839e29e15bef366047ed591cef58dfb4
68537275bd91d1dc14a69609ae443f955bfdbd64 build: Enable -Werror=sign-compare (Ben Woosley)
eac6a3080d38cfd4eb7204ecd327df213958e51a refactor: Rework asmap Interpret to avoid ptrdiff_t (Ben Woosley)
df37377e30678ac9b8338ea920e50b7296da6bd5 test: Fix outstanding -Wsign-compare errors (Ben Woosley)
Pull request description:
Disallowing sign-comparison mismatches can help to prevent the introduction of overflow and interpretation bugs.
In this case, ~all~ most existing violations are in the tests, and most simply required annotating the literal as unsigned for comparison.
This was previously prevented by violations in leveldb which were fixed upstream and merged in #17398. You can test that by building this branch against: 22d11187ee3c7abfe9d43c9eb68f102498cc2b9a vs 75fb37ce68289eb7e00e2ccdd2ef7f9271332545
ACKs for top commit:
fjahr:
re-ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
practicalswift:
ACK 68537275bd91d1dc14a69609ae443f955bfdbd64
Tree-SHA512: 14b5daa38c496fb51548feb30fb4dd179e6f76a8d355f52bc8e2a18f2f9340f0bc98dcf36d8b3d6521045d013891c3103749a4eda88ceef00202a6a0cf93f73c
e8fc236da70085d30c90cdade06edfa1da855a6c refactor: add missing std:: includes to threadnames.cpp (fanquake)
87f3c04cc539c34d32af5fd59abef2c0b5faee26 doc: remove incorrect mention of PR_GET_NAME (fanquake)
Pull request description:
By removing the whole comment. These `#include // For` comments are near impossible
to maintain, pollute diffs, and generally don't add a lot of value.
While here, also add the missing `std::` includes.
ACKs for top commit:
junderw:
LGTM ACK e8fc236
Tree-SHA512: d29aff40c94f59c42f295a5738bc5ff2f4a2f2e6d270cc505f27d56d07d272597e2f8403d72fe45775661e1a1fc2af9fc52aeaeb41263bd3e9dfe255332383c8
faa7d8a3f7cba02eca7e247108a6b98ea9daf373 util: Add SaturatingAdd helper (MarcoFalke)
Pull request description:
Seems good to have this in the repo, as it might be needed to write cleaner code. For example:
* https://github.com/bitcoin/bitcoin/pull/24090#issuecomment-1019948200
* https://github.com/bitcoin/bitcoin/pull/23418#discussion_r744953272
* ...
ACKs for top commit:
MarcoFalke:
Added a test. Should be trivial to re-ACK with `git range-diff bitcoin-core/master fa90189cbf faa7d8a3f7`
klementtan:
reACK faa7d8a3f7
vasild:
ACK faa7d8a3f7cba02eca7e247108a6b98ea9daf373
Tree-SHA512: d0e6efdba7dfcbdd16ab4539a7f5e45a97d17792e42586c3c52caaae3fc70612dc9e364359658de5de5718fb8c2a765a59ceb2230098355394fa067a9732bc2a
317442525586ba9ff8b9af6c506b48f87cd8cd87 Cleanup headers after #20788 (Hennadii Stepanov)
Pull request description:
This is a header cleanup after #20788.
ACKs for top commit:
vasild:
ACK 317442525586ba9ff8b9af6c506b48f87cd8cd87
Tree-SHA512: 1c21b1ba43841880625289174f10e5b333f6eb857f448e1e4114b1ecdf32a6044ec91c5987c1d66806c1d408a4e3d46569eb41d69a0acb8296601d7c203d9f1d
7b3a20b2602f902c344a615f23f8f0280b6f6bcc mempool: apply rule of 5 to epochguard.h, fix compiler warnings (Jon Atack)
Pull request description:
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:
- https://en.cppreference.com/w/cpp/language/rule_of_three
- https://www.stroustrup.com/C++11FAQ.html#default
- https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-five
<details><summary>Compiler warnings fixed</summary><p>
```bash
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());
```
```bash
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;
```
```bash
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>
ACKs for top commit:
laanwj:
Code review ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc
vasild:
ACK 7b3a20b2602f902c344a615f23f8f0280b6f6bcc
Tree-SHA512: 0406dfcec180152d4f9ed07cbc2f406ad739b41f9c9cb38f8c75159c15d9d8a9a5c7526765966e40d695d265c178f6a80152e7edf82da344a65e55938dddb63d
da9caa1cedd69702aea44cb44b2fd0a2d6d56916 Replace fs::absolute calls with AbsPathJoin calls (Kiminuo)
66576c4fd532ac18b8b355ea93d25581a2c15654 test: Clear forced -walletdir setting after wallet init_tests (Kiminuo)
Pull request description:
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
This PR doesn't change behavior aside from adding an assert and fixing a test bug.
ACKs for top commit:
jonatack:
Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 only doxygen improvements since my last review per `git diff d867d7a da9caa1`
MarcoFalke:
review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 📯
ryanofsky:
Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916. Just comment and test tweaks since previous review.
Tree-SHA512: c940ee60f3ba374d4927cf34cf12d27c4c735c94af591fbc0ca408c641b30f8f8fbcfe521d66bfbddf9877a1fc8cd99bd8a47ebcd2fa59789de6bd87a7b9cf4d
fd6580e405699ccb051fd2a34525e48d3253673d [refactor] txmempool: split epoch logic into class (Anthony Towns)
Pull request description:
Splits the epoch logic introduced in #17925 into a separate class.
Uses clang's thread safety annotations and encapsulates the data more strongly to reduce chances of bugs from API misuse.
ACKs for top commit:
jonatack:
ACK fd6580e405699ccb051fd2a34525e48d3253673d using clang thread safety annotations looks like a very good idea, and the encapsulation this change adds should improve robustness (and possible unit test-ability) of the code. Verified that changing some of the locking duly provoked build-time warnings with Clang 9 on Debian and that small changes in the new `Epoch` class were covered by failing functional test assertions in `mempool_updatefromblock.py`, `mempool_resurrect.py`, and `mempool_reorg.py`
hebasto:
re-ACK fd6580e405699ccb051fd2a34525e48d3253673d, since my [previous](https://github.com/bitcoin/bitcoin/pull/18017#pullrequestreview-569619362) review:
Tree-SHA512: 7004623faa02b56639aa05ab7a078320a6d8d54ec62d8022876221e33f350f47df51ddff056c0de5be798f8eb39b5c03c2d3f035698555d70abc218e950f2f8c
3ddbf22ed179a2db733af4b521bec5d2b13ebf4b util: Disallow negative mocktime (MarcoFalke)
f5f2f9716885e7548809e77f46b493c896a019bf net: Avoid UBSan warning in ProcessMessage(...) (practicalswift)
Pull request description:
Avoid UBSan warning in `ProcessMessage(...)`.
Context: https://github.com/bitcoin/bitcoin/pull/20380#issuecomment-770427182 (thanks Crypt-iQ!)
ACKs for top commit:
MarcoFalke:
re-ACK 3ddbf22ed179a2db733af4b521bec5d2b13ebf4b only change is adding patch written by me
ajtowns:
ACK 3ddbf22ed179a2db733af4b521bec5d2b13ebf4b -- code review only
Tree-SHA512: e8d7af0457ca86872b75a4e406c0a93aafd841c2962e244e147e748cc7ca118c56be0fdafe53765f4b291410030b2c3cc8f76f733b37a955d34fc885ab6037b9
aaaaad6ac95b402fe18d019d67897ced6b316ee0 scripted-diff: Bump copyright of files changed in 2019 (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK aaaaad6ac95b402fe18d019d67897ced6b316ee0
promag:
ACK aaaaad6ac95b402fe18d019d67897ced6b316ee0 🎉
fanquake:
ACK aaaaad6ac95b402fe18d019d67897ced6b316ee0 - going to merge this now because the year is over and conflicts are minimal.
Tree-SHA512: 58cb1f53bc4c1395b2766f36fabc7e2332e213780a802762fff0afd59468dad0c3265f553714d761c7a2c44ff90f7dc250f04458f4b2eb8eef8b94f8c9891321
e286cd0d7b4e12c8efe5e7ac3066a100e0ba2c0a net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)
Pull request description:
Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in https://github.com/bitcoin/bitcoin/pull/21631.
ACKs for top commit:
practicalswift:
cr ACK e286cd0d7b4e12c8efe5e7ac3066a100e0ba2c0a: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
laanwj:
Code review ACK e286cd0d7b4e12c8efe5e7ac3066a100e0ba2c0a
Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
## Issue being fixed or feature implemented
Some headers include other heavy headers, such as `logging.h`,
`tinyformat.h`, `iostream`. These headers are heavy and increase
compilation time on scale of whole project drastically because can be
used in many other headers.
## What was done?
Moved many heavy includes from headers to cpp files to optimize
compilation time.
In some places added forward declarations if it is reasonable.
As side effect removed 2 circular dependencies:
```
"llmq/debug -> llmq/dkgsessionhandler -> llmq/debug"
"llmq/debug -> llmq/dkgsessionhandler -> llmq/dkgsession -> llmq/debug"
```
## How Has This Been Tested?
Run build 2 times before refactoring and after refactoring: `make clean
&& sleep 10s; time make -j18`
Before refactoring:
```
real 5m37,826s
user 77m12,075s
sys 6m20,547s
real 5m32,626s
user 76m51,143s
sys 6m24,511s
```
After refactoring:
```
real 5m18,509s
user 73m32,133s
sys 6m21,590s
real 5m14,466s
user 73m20,942s
sys 6m17,868s
```
~5% of improvement for compilation time. That's not huge, but that's
worth to get merged
There're several more refactorings TODO but better to do them later by
backports:
- bitcoin/bitcoin#27636
- bitcoin/bitcoin#26286
- bitcoin/bitcoin#27238
- and maybe this one: bitcoin/bitcoin#28200
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
fea75ad3caa29972db32d3ce7e0fe125ec77a0eb refactor: Drop `boost/algorithm/string/replace.hpp` dependency (Hennadii Stepanov)
857526e8cbb0847a865e9c2509425960d458f535 test: Add test case for `ReplaceAll()` function (Hennadii Stepanov)
Pull request description:
A new implementation of the `ReplaceAll()` seems enough for all of our purposes.
ACKs for top commit:
adam2k:
ACK Tested fea75ad3caa29972db32d3ce7e0fe125ec77a0eb
theStack:
Code-review ACK fea75ad3caa29972db32d3ce7e0fe125ec77a0eb
Tree-SHA512: dacfffc9d2bd1fb9f034baf8c045b1e8657b766db2f0a7f8ef7e25ee6cd888f315b0124c54aba7a29ae59186b176ef9868a8b709dc995ea215c6b4ce58e174d9
includes:
- 2a2182c387f607cd8284f33890bd285a81077b7f
- 77c507358bda9bd6c496f33e0f4418c0603bb08d
completion of partial merge done in dash#4164 as 56f1b2d (blocker
bitcoin#17938 was merged in dash#5246 as 00802bb)
## Issue being fixed or feature implemented
Some headers or modules are used objects from STL without including it
directly, it cause compilation failures on some platforms for some
specific compilers such as #5554
## What was done?
Added missing includes and removed obsolete includes for `optional`,
`deque`, `tuple`, `unordered_set`, `unordered_map`, `set` and `atomic`.
Please, note, that this PR doesn't cover all cases, only cases when it
is obviously missing or obviously obsolete.
Also most of changes belongs to to dash specific code; but for cases of
original bitcoin code I keep it untouched, such as missing <map> in
`src/psbt.h`
I used this script to get a list of files/headers which looks suspicious
`./headers-scanner.sh std::optional optional`:
```bash
#!/bin/bash
set -e
function check_includes() {
obj=$1
header=$2
file=$3
used=0
included=0
grep "$obj" "$file" >/dev/null 2>/dev/null && used=1
grep "include <$header>" $file >/dev/null 2>/dev/null && included=1
if [ $used == 1 ] && [ $included == 0 ]
then echo "missing <$header> in $file"
fi
if [ $used == 0 ] && [ $included == 1 ]
then echo "obsolete <$header> in $file"
fi
}
export -f check_includes
obj=$1
header=$2
find src \( -name '*.h' -or -name '*.cpp' -or -name '*.hpp' \) -exec bash -c 'check_includes "$0" "$1" "$2"' "$obj" "$header" {} \;
```
## How Has This Been Tested?
Built code locally
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
fa4632c41714dfaa699bacc6a947d72668a4deef test: Move boost/stdlib includes last (MarcoFalke)
fa488f131fd4f5bab0d01376c5a5013306f1abcd scripted-diff: Bump copyright headers (MarcoFalke)
fac5c373006a9e4bcbb56843bb85f1aca4d87599 scripted-diff: Sort test includes (MarcoFalke)
Pull request description:
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have `clang-format` installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.
This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is **NOT** a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.
ACKs for top commit:
practicalswift:
ACK fa4632c41714dfaa699bacc6a947d72668a4deef
jonatack:
ACK fa4632c41714dfaa, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
01a3392b1b778fa4fcf568013326d6ea1de4fb3b Drop bitcoin-wallet dependency on libevent (Russell Yanofsky)
0660119ac372c2863d14060ac1bc9bc243771f94 Drop unintended bitcoin-tx dependency on libevent (Russell Yanofsky)
Pull request description:
This fixes compile errors trying to build bitcoin-tx and bitcoin-wallet without libevent, which were reported by Luke Dashjr in https://github.com/bitcoin/bitcoin/issues/18465
The fix avoiding `bitcoin-tx` dependency on libevent just adds a conditional build rule. This is implemented in the first commit (more details in commit description).
The fix avoiding `bitcoin-wallet` dependency on libevent requires minor code changes, because `bitcoin-wallet` (unlike `bitcoin-tx`) links against code that calls `urlDecode` / `evhttp_uridecode`. This fix is implemented in the second commit (again details in the commit description).
ACKs for top commit:
jonasschnelli:
utACK 01a3392b1b778fa4fcf568013326d6ea1de4fb3b.
Tree-SHA512: d2245e912ab494cccceeb427a1eca8e55b01a0006ff93eebcfb5461ae7cecd1083ac2de443d9db036b18bdc6f0fb615546caaa20c585046f66d234937f74870a
ef712298c3f8bc2afdad783f05080443b72b3f77 util: Check for file being NULL in DirectoryCommit (Luke Dashjr)
457490403853321d308c6ca6aaa90d6f8f29b4cf Fix possible data race when committing block files (Evan Klitzke)
220bb16cbee5b91d0bc0fcc6c71560d631295fa5 util: Introduce DirectoryCommit commit function to sync a directory (Evan Klitzke)
ce5cbaea63ad4ea78e533bdb14f47f414061ae7f util.h: Document FileCommit function (Evan Klitzke)
844d650eea3bd809884cc5dd996a388bdc58314e util: Prefer Mac-specific F_FULLSYNC over fdatasync in FileCommit (Evan Klitzke)
f6cec0bcaf560fa310853ad3fe17022602b63d5f util: Refactor FileCommit from an #if sequence nested in #else, to a sequence of #elif (Evan Klitzke)
Pull request description:
Reviving #12696
ACKs for top commit:
laanwj:
Code review ACK ef712298c3f8bc2afdad783f05080443b72b3f77
Tree-SHA512: 07d650990ef4c18d645dee3f9a199a940683ad17557d79d93979a76c4e710d8d70e6eae01d1a5991494a24a7654eb7db868be0c34a31e70b2509945d95bc9cce
This data structure provide efficient storage for numbers if amount of gaps between them is not too big
It works similarly to CSkipSet but amount of gaps now in unlimited
1e62350ca20898189904a88dfef9ea11ddcd8626 refactor: Improve use of explicit keyword (Fabian Jahr)
c502a6dbfb854ca827a5a3925394f9e09d29b898 lint: Use c++17 std in cppcheck linter (Fabian Jahr)
Pull request description:
I found the `extended-lint-cppcheck` linter still uses `std=c++11` when reviewing #20471. The only difference in the output after this change is one line is missing:
```
src/script/descriptor.cpp:159:5: warning: Struct 'PubkeyProvider' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
```
After some digging, I am still not sure why this one is ignored with c++17 when 40 other`noExplicitConstructor` warnings were still appearing.
In the second commit, I fix these warnings, adding `explicit` where appropriate and adding fixes to ignore otherwise.
ACKs for top commit:
practicalswift:
cr ACK 1e62350ca20898189904a88dfef9ea11ddcd8626: patch looks correct!
MarcoFalke:
review ACK 1e62350ca20898189904a88dfef9ea11ddcd8626
Tree-SHA512: dff7b324429a57160e217cf38d9ddbb6e70c6cb3d3e3e0bd4013d88e07afc2292c3df94d0acf7122e9d486322821682ecf15c8f2724a78667764c05d47f89a12
## Issue being fixed or feature implemented
Fixes issue #5497.
## What was done?
Checks if settings file is empty, and deletes it if that's the case.
It will will be generated with default value `{}` afterwards.
## How Has This Been Tested?
Running Dash Qt on regtest masternode with `--nocleanup` and
`./src/qt/dash-qt --regtest --datadir=`
## Breaking Changes
No
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
This is an implementation of DIP0027 "Credit Asset Locks".
It's a mechanism to fluidly exchange between Dash and credits.
## What was done?
This pull request includes:
- Asset Lock transaction
- Asset Unlock transaction (withdrawal)
- Credit Pool in coinbase
- Unit tests for Asset Lock/Unlock tx
- New functional test `feature_asset_locks.py`
RPC: currently locked amount (credit pool) is available through rpc call
`getblock`.
## How Has This Been Tested?
There added new unit tests for basic checks of transaction validity
(asset lock/unlock).
Also added new functional test "feature_asset_locks.py" that cover
typical cases, but not all corner cases yet.
## Breaking Changes
This feature should be activated as hard-fork because:
- It adds 2 new special transaction and one of them [asset unlock tx]
requires update consensus rulels
- It adds new data in coinbase tx (credit pool)
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
**To release DIP 0027**
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
25dac9fa65243ca8db02df22f484039c08114401 doc: add release notes for explicit fee estimators and bumpfee change (Karl-Johan Alm)
05227a35545d7656450874b3668bf418c73813fb tests for bumpfee / estimate_modes (Karl-Johan Alm)
3404c1b753432c4859a4ca245f01c240610a00cb policy: optional FeeEstimateMode param to CFeeRate::ToString (Karl-Johan Alm)
6fcf4484302d13bd7739b617470d8c8e31974908 rpc/wallet: add two explicit modes to estimate_mode (Karl-Johan Alm)
b188d80c2de9ebb114da5ceea78baa46bde7dff6 MOVEONLY: Make FeeEstimateMode available to CFeeRate (Karl-Johan Alm)
5d1a411eb12fc700804ffe5d6e205234d30edd5f fees: add FeeModes doc helper function (Karl-Johan Alm)
91f6d2bc8ff4d4cd1b86daa370ec9d2d9662394d rpc/wallet: add conf_target as alias to confTarget in bumpfee (Karl-Johan Alm)
69158b41fc488e4f220559da17a475eff5923a95 added CURRENCY_ATOM to express minimum indivisible unit (Karl-Johan Alm)
Pull request description:
This lets users pick their own fees when using `sendtoaddress`/`sendmany` if they prefer this over the estimators.
ACKs for top commit:
Sjors:
re-utACK 25dac9fa65: rebased, more fancy C++,
jonatack:
ACK 25dac9fa65243ca8db02df2 I think this should be merged after all this time, even though it looks to me like there are needed follow-ups, fixes and test coverage to be added (see further down), which I don't mind helping out with, if wanted.
fjahr:
Code review ACK 25dac9fa65243ca8db02df22f484039c08114401
Tree-SHA512: f31177e6cabf3187a43cdfe93477144f8e8385c7344613743cbbd16e8490d53ff5144aec7b9de6c9a65eb855b55e0f99d7f164dee4b6bf3cfea4dce51cf11d33
fad0867d6ab9430070aa7d60bf7617a6508e0586 Cleanup -includeconf error message (MarcoFalke)
fa9f711c3746ca3962f15224285a453744cd45b3 Fix crash when parsing command line with -noincludeconf=0 (MarcoFalke)
Pull request description:
The error message has several issues:
* It may crash instead of cleanly shutting down, when `-noincludeconf=0` is passed
* It doesn't quote the value
* It includes an erroneous trailing `\n`
* It is redundantly mentioning `"-includeconf cannot be used from commandline;"` several times, when once should be more than sufficient
Fix all issues by:
* Replacing `get_str()` with `write()` to fix the crash and quoting issue
* Remove the `\n` and only print the first value to fix the other issues
Before:
```
$ ./src/bitcoind -noincludeconf=0
terminate called after throwing an instance of 'std::runtime_error'
what(): JSON value is not a string as expected
Aborted (core dumped)
$ ./src/bitcoind -includeconf='a b' -includeconf=c
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=a b
-includeconf cannot be used from commandline; -includeconf=c
```
After:
```
$ ./src/bitcoind -noincludeconf=0
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=true
$ ./src/bitcoind -includeconf='a b' -includeconf=c
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf="a b"
```
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34493
Testcase: https://github.com/bitcoin/bitcoin/files/6515429/clusterfuzz-testcase-minimized-system-6328535926046720.log
```
FUZZ=system ./src/test/fuzz/fuzz ./clusterfuzz-testcase-minimized-system-6328535926046720.log
```
See https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md
ACKs for top commit:
sipa:
utACK fad0867d6ab9430070aa7d60bf7617a6508e0586
Tree-SHA512: b44af93be6bf71b43669058c1449c4c6999f03b5b01b429851b149b12d77733408cb207e9a3edc6f0bffd6030c4c52165e8e23a1c2718ff5082a6ba254cc94a4
10efc0487c442bccb0e4a9ac29452af1592a3cf2 Templatize ValidationState instead of subclassing (Jeffrey Czyz)
10e85d4adc9b7dbbda63e00195e0a962f51e4d2c Remove ValidationState's constructor (Jeffrey Czyz)
0aed17ef2892478c28cd660e53223c6dd1dc0187 Refactor FormatStateMessage into ValidationState (Jeffrey Czyz)
Pull request description:
This removes boilerplate code in the subclasses which otherwise only
differ by the result type.
The subclassing was introduced in a27a295.
ACKs for top commit:
MarcoFalke:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 🐱
ajtowns:
ACK 10efc0487c442bccb0e4a9ac29452af1592a3cf2 -- looks good to me
jonatack:
ACK 10efc048 code review, build/tests green, nice cleanup
Tree-SHA512: 765dd52dde7d49b9a5c6d99d97c96f4492673e2aed0b0604faa88db0308fa4500a26bf755cca0b896be283874096c215932e1110a2d01dc012cd36a5fce58a42
f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f Add logging for CValidationInterface events (Jeffrey Czyz)
6edebacb2191373e76d79a4972d6192300976096 Refactor FormatStateMessage for clarity (Jeffrey Czyz)
72f3227c83810936e7a334304e5fd7c6dab8e91b Format CValidationState properly in all cases (Jeffrey Czyz)
428ac70095253225f64462ee15c595644747f376 Add VALIDATION to BCLog::LogFlags (Jeffrey Czyz)
Pull request description:
Add logging of `CValidationInterface` callbacks using a new `VALIDATIONINTERFACE` log flag (see #12994). A separate flag is desirable as the logging can be noisy and thus may need to be disabled without affecting other logging.
This could help debug issues where there may be race conditions at play, such as #12978.
ACKs for top commit:
jnewbery:
ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
hebasto:
ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
ariard:
ACK f9abf4a, only changes since 0cadb12 are replacing log indication `VALIDATIONINTERFACE` by `VALIDATION` and avoiding a forward declaration with a new include
ryanofsky:
Code review ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f. Just suggested changes since last review (thanks!)
Tree-SHA512: 3e0f6e2c8951cf46fbad3ff440971d95d526df2a52a2e4d6452a82785c63d53accfdabae66b0b30e2fe0b00737f8d5cb717edbad1460b63acb11a72c8f5d4236
e09c701e0110350f78366fb837308c086b6503c0 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe6209646db8914b87bf6edbc18c6031a16f1e scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
3bf9d8cac09fc88727ef2f2a2bea33b90b625e50 Testchains: Qt: Simplify network/chain styles (Jorge Timón)
052c54ecb02695e5d2694e8e0cbf5ccc89de86e8 Testchains: Generic selection with -chain=<str> in addition of -testnet and -regtest (Jorge Timón)
Pull request description:
Separated from #8994 as suggested by MarcoFalke and Sjors in https://github.com/bitcoin/bitcoin/pull/8994#issuecomment-522555390
You can't really test the qt changes on their own, so to test them, use #8994 .
ACKs for top commit:
MarcoFalke:
ACK 3bf9d8cac09fc88727ef2f2a2bea33b90b625e50
Tree-SHA512: 5b5e6083ebc0a44505a507fac633e7af18037c85e5e73f5d1e6f7e730575d3297ba8a31d1c2441df623b273f061c32d8fa324f4aa6bead01d23e88582029b568
4bdd68f301a9cee3360deafc7531c638e923226b Add missing typeinfo includes (Wladimir J. van der Laan)
4d88c3dcb61e7c075ed3dd442044e0eff4e3c8de net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)
Pull request description:
Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.
ACKs for top commit:
MarcoFalke:
re-ACK 4bdd68f301a9cee3360deafc7531c638e923226b (only change is adding includes) 🕕
promag:
ACK 4bdd68f301a9cee3360deafc7531c638e923226b, could squash.
Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366
e9fd366044e271632dc0e4f96e1c14f8e87213ae refactor: Remove null setting check in GetSetting() (Russell Yanofsky)
cba2710220d76bbe790b04088839cbbd410436de scripted-diff: Remove unused ArgsManager type flags in tests (Russell Yanofsky)
425bb307252cf4dec9b3ef6426e6548b2be7a303 refactor: Add util_CheckValue test (Russell Yanofsky)
0fa54358b06b58f4d17073bcc8a959eb9498aadc refactor: Add ArgsManager::GetSettingsList method (Russell Yanofsky)
3e185522ace1678e0a25b9cf8a5553a4bc279bea refactor: Get rid of ArgsManagerHelper class (Russell Yanofsky)
dc0f1480746b34aa3ca2d9c0f1ec764083026b40 refactor: Replace FlagsOfKnownArg with GetArgFlags (Russell Yanofsky)
57e8b7a7273567aa4a4aee87cce18e9bff8f3196 refactor: Clean up includeconf comments (Russell Yanofsky)
3f7dc9b808316c1e5d677af8d9a99112568c8ccb refactor: Clean up long lines in settings code (Russell Yanofsky)
Pull request description:
This PR doesn't change behavior. It just implements some suggestions from #15934 and #16545 and few other small cleanups.
ACKs for top commit:
jnewbery:
Code review ACK e9fd366044e271632dc0e4f96e1c14f8e87213ae
MarcoFalke:
ACK e9fd366044 🚟
Tree-SHA512: 6e100d92c72f72bc39567187ab97a3547b3c06e5fcf1a1b74023358b8bca552124ca6a53c0ab53179b7f1329c03d9a73faaef6d73d2cd1a2321568a0286525e2
