1afc0e4aa1b910991d4f8a77d74e2197f370987c doc: remove potentially confusing ChainstateManager comment (James O'Beirne)
769a1ef9fdc9c372f5bbe91d1961cabd60bc1895 test: Add tests with maleated snapshot data (Fabian Jahr)
4d8de04f32736199e4b41a14a2d29b1a4d0a15d4 tests: add snapshot activation test (James O'Beirne)
31d225274ff1a4b245aea0a69f0e5224b0e64ca2 tests: add deterministic chain generation unittest fixture (James O'Beirne)
6606a4f8c616cf256537c3bfbdade9b43c51b4f5 move-onlyish: break out CreateUTXOSnapshot from dumptxoutset (James O'Beirne)
ad949ba449ff2115e3d22c71f5b6509f11112098 txdb: don't reset during in-memory cache resize (James O'Beirne)
f6e2da5fb7c6406c37612c838c998078ea8d2252 simplify ChainstateManager::SnapshotBlockhash() return semantics (James O'Beirne)
7a6c46b37edb8bfa0085d202aa7e9427d5e4fceb chainparams: add allowed assumeutxo values (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This change proposes logic for activating UTXO snapshots, which is unused at the moment aside from an included unittest. There are a few moveonyish/refactoring commits to allow for halfway decent unittests.
Basic structure is included for specifying and checking the assumeutxo hash values used to validate activated snapshots. Initially I had specified a few height/hash pairs for mainnet in this change, but because of the security-critical nature of those parameters, I figured it was better to leave their inclusion to a future PR that includes only that change - my intent being that reviewers will be more likely to verify those parameters firsthand in a dedicated PR.
Aside from that and the snapshot activation logic, there are a few related changes:
- ~~allow caching the `nChainTx` value in the CCoinsViewDB; this is set during snapshot activation. Because we don't necessarily have access to the full chain at the time of snapshot load, this value is communicated through the snapshot metadata and must be cached within the chainstate to survive restarts.~~
- break out `CreateUTXOSnapshot()` from dumptxoutset. This is essentially a move-only change to allow the reuse of snapshot creation logic from within unittests.
- ...and a few other misc. changes that are solely related to unittests.
The move-onlyish commit is most easily reviewed with `--color-moved=zebra`.
ACKs for top commit:
fjahr:
Code review ACK 1afc0e4aa1b910991d4f8a77d74e2197f370987c
laanwj:
Code review ACK 1afc0e4aa1b910991d4f8a77d74e2197f370987c
Tree-SHA512: a4e4f0698f00a53ec298b5e8b7ef1c9fdf0185f95139d1b1f63cfdf6cbbd6d17b8c6e51bbf1de2e5f1a946bf49f8466232698ef55acce5a012c80b067da366ea
ef21fb7313005a8a2d4f03fb4056f1f66c1b04f0 zmq test: speedup test by whitelisting peers (immediate tx relay) (Sebastian Falbesoner)
5c6546362dce8b468268578e345c37ed515a1855 zmq test: fix flakiness by using more robust sync method (Sebastian Falbesoner)
8666033630eeaf851ec69e018bb53eb23093f4b9 zmq test: accept arbitrary sequence start number in ZMQSubscriber (Sebastian Falbesoner)
6014d6e1b5a0dda6e20c2721f0bdb7e6a63ece81 zmq test: dedup message reception handling in ZMQSubscriber (Sebastian Falbesoner)
Pull request description:
Fixes#20934 by using the "sync up" method described in https://github.com/bitcoin/bitcoin/issues/20538#issuecomment-738791868.
After improving robustness with this approach (commits 1-3), it turned out that there were still some fails, but those were unrelated to zmq: Out of 500 runs, 3 times `sync_mempool()` or `sync_blocks()` timed out, which can happen because the trickle relay time has no upper bound -- hence in rare cases, it takes longer than 60s. This is fixed by enabling immediate tx relay on node1 (commit 4), which as a nice side-effect also gives us a rough 2x speedup for the test.
For further details, also see the explanations in the commit messages.
There is no guarantee that the test is still not flaky, but it would help if potential reviewers would run the following script locally and report how many runs failed (feel free to do less than 1000 runs, as this takes quite a long if ran with `--valgrind`):
```
#!/bin/sh
OUTPUT_FILE=./zmq_results
echo ===== repeated zmq test ===== > $OUTPUT_FILE
for i in `seq 1000`; do
echo ------------------------
echo ----- test run $i -----
echo ------------------------
echo --- $i --- >> $OUTPUT_FILE
./test/functional/interface_zmq.py --valgrind
if [ $? -ne 0 ]; then
echo "FAILED. /o\\" >> $OUTPUT_FILE
else
echo "PASSED. \\o/" >> $OUTPUT_FILE
fi
done
echo Failed test runs:
grep FAILED $OUTPUT_FILE | wc -l
```
ACKs for top commit:
jonatack:
Light ACK ef21fb7313005a8a2d4f03fb4056f1f66c1b04f0 with the caveat that I was unable to make the test fail with valgrind both here and on master, so I can't vouch that it actually fixes the CI flakiness. The test does run ~2x faster with this.
Tree-SHA512: 7a1e7592fbbd98e69e1e1294486b91253e589c72b3c6bbb7f587028ec07cca59b7d984e4ebf256c4bc3e8a529ec77d31842f3dd874038aea0b684abfea50306a
a5e15ae45ccae7948a6c5b95e6621f01afb88d55 scripted-diff: rename ping members (John Newbery)
45dcf2266125c65d7f546bdb211a278bd090a284 [net processing] Move ping data fields to net processing (John Newbery)
dd2646d12c172cb8899669af717c590483a17404 [net processing] Move ping timeout logic to net processing (John Newbery)
0b43b81f69ff13dbc1e893a80950f186690b4f62 [net processing] Move send ping message logic into function (John Newbery)
1a07600b4b0d08cffc7cd5c58af33fcd1ede558e [net] Add RunInactivityChecks() (John Newbery)
f8b3058992b507f3a6aac9d4e2db00102ae1b197 [net processing] Add Peer& arg to MaybeDiscourageAndDisconnect() (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing, by moving all ping data into the new Peer object added in #19607.
For motivation, see #19398.
ACKs for top commit:
glozow:
reACK a5e15ae45c
MarcoFalke:
review ACK a5e15ae45ccae7948a6c5b95e6621f01afb88d55 🥉
amitiuttarwar:
ACK a5e15ae45c
Tree-SHA512: fb84241613d6a6e1f2832fa5378030b5877a02e8308188f57ab545a6eaf2ab731a93abb7dcd3a7f7285bb66700f938096378a8e90cd6a3e6f3309f81d85a344e
ffff84a9cb659562b3f560d3a489d4a62c71f793 fuzz: Remove expensive and redundant muhash from crypto fuzz target (MarcoFalke)
Pull request description:
Remove because it is redundant with `src/test/fuzz/muhash.cpp` and incredibly expensive
ACKs for top commit:
practicalswift:
Tested ACK ffff84a9cb659562b3f560d3a489d4a62c71f793
Tree-SHA512: c91ea2406db857127c789b9cdeb714a719d88b54132e9cef74fffd229532d874b6c043353793ec687504b5784afc74995f8982243d41f976b63d57454a5ed339
Moves the logic to prevent running inactivity checks until
the peer has been connected for -peertimeout time into its
own function. This will be reused by net_processing later.
96635e61777add29b6a34d47767a63f43b2919af init: use GetNetworkNames() in -onlynet help (Jon Atack)
0dbde700a6e9894a8ead20f2eebd0ff6554ef2d9 rpc: use GetNetworkNames() in getnetworkinfo and getpeerinfo helps (Jon Atack)
1c3af37881adbbc37fb9924bcf69c403fcae1ae7 net: create GetNetworkNames() (Jon Atack)
b45eae4d5332f1da71ba9ec983fe7818fa4d32e9 net: update NET_UNROUTABLE to not_publicly_routable in GetNetworkName() (Jon Atack)
Pull request description:
per the IRC discussion today at http://www.erisian.com.au/bitcoin-core-dev/log-2021-01-19.html#l-87
- return a more helpful string name for `Network::NET_UNROUTABLE`: "not_publicly_routable" instead of "unroutable"
- update the RPC getpeerinfo "network" help, and automate it and the getnetworkinfo "network#name" and the -onlynet help doc generation
ACKs for top commit:
theStack:
re-ACK 96635e61777add29b6a34d47767a63f43b2919af 🌳
MarcoFalke:
review ACK 96635e61777add29b6a34d47767a63f43b2919af 🐗
Tree-SHA512: 511a7d987126b48a7a090739aa7c4964b6186a3ff8f5f7eec9233dd816c6b7a6dc91b3ea6b824aa68f218a8a3ebdc6ffd214e9a88af38f2bf23f3257c4284c3a
2ee4a7a9ec68c75094685c06ec793b614f44c4ce net: remove CNode::m_inbound_onion defaults for explicitness (Jon Atack)
24bda56c29800502953c6a8cd69248e60ff9a0a0 net: make CNode::m_inbound_onion public, drop getter, update tests (Jon Atack)
Pull request description:
Refactoring only, no change in behavior. This is a quick follow-up to #20210 to address these review comments:
- https://github.com/bitcoin/bitcoin/pull/20210#discussion_r528835313
- https://github.com/bitcoin/bitcoin/pull/20210#discussion_r550860416
- https://github.com/bitcoin/bitcoin/pull/20210#issuecomment-766093925
Changes:
- make the `CNode::m_inbound_onion class` member public, update the Doxygen comment, drop the getter, and update the tests
- remove the `CNode::m_inbound_onion` default value initialization in the ctor declaration and the member initializer in favor of always passing it explicitly to the ctor where we initialize it dynamically, to both clarify the caller code and to allow the compiler to warn if it is uninitialized in the ctor or omitted in the caller
ACKs for top commit:
MarcoFalke:
review ACK 2ee4a7a9ec68c75094685c06ec793b614f44c4ce 🏀
vasild:
ACK 2ee4a7a9ec68c75094685c06ec793b614f44c4ce
Tree-SHA512: 72961c91168885a9d881756b10bad9d587f5ce297d5a6493c23e267b7178ff22b697bc6868e7761d6304e372d2781453d30011a020afd506b1e308b4ffa5feee
3f8776a1391c3978ed66144df15fd9bcb9edd35d Re-add dead code detection (flack)
Pull request description:
This re-adds unreachable code detection for Python based on `vulture`.
Effectively, this reverts f4beb4996d27f2cdaf4f0a63e7dc044bf17decce. The difference to the previous version is that this runs with the `--min-confidence 100` setting. From https://pypi.org/project/vulture/:
> Use `--min-confidence 100` to only report code that is guaranteed to be unused within the analyzed files.
So this should avoid the previous issues where static analysis had wrong positives due to the dynamic nature of Python code by only reporting things that are unambiguous (such as code after a `return` statement). As such, there is not suppressions list.
My motivation was mainly #21081 which would have been caught by this (as can be seen by the CI run failing). This is still marked as draft because #21081 is needed to get the linter to pass. Also, there is a second problem that this found (see https://github.com/bitcoin/bitcoin/pull/19509/files#r571454691). From what I can tell, this is a spurious type comment that could just be removed (or if that line has no side effects it could also be deleted altogether?). I could add a commit here to fix it, but I wanted to see if there is interest in having this linter again in the first place
ACKs for top commit:
practicalswift:
ACK 3f8776a1391c3978ed66144df15fd9bcb9edd35d
Tree-SHA512: 52314ad4f627d969de1eb15375ca677ed86a2e816fe773756a1ce22421214ba407b5a09a4bf701a3aab1a10c7b336f548e4cef3327edf154acba55e987db21f6
f64adc1eedff9d342b49d7e6428b2da21130c23c test: remove unused function xor_bytes (Sebastian Falbesoner)
Pull request description:
The function `xor_bytes` was introduced in commit 3c226639eb134314a0640d34e4ccb6148dbde22f (#19953, BIP340-342 validation), even [code-reviewed](https://github.com/bitcoin/bitcoin/pull/19953/files#r509383731), but actually never used. The [default signing algorithm in BIP340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#Default_Signing) needs a xor operation, but this step is currently done by a single xor operation on large integer operands:
```
t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big')
```
Alternatively, we could keep the function and as well use it:
```diff
--- a/test/functional/test_framework/key.py
+++ b/test/functional/test_framework/key.py
@@ -492,7 +492,7 @@ def sign_schnorr(key, msg, aux=None, flip_p=False, flip_r=False):
P = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, sec)]))
if SECP256K1.has_even_y(P) == flip_p:
sec = SECP256K1_ORDER - sec
- t = (sec ^ int.from_bytes(TaggedHash("BIP0340/aux", aux), 'big')).to_bytes(32, 'big')
+ t = xor_bytes(sec.to_bytes(32, 'big'), TaggedHash("BIP0340/aux", aux))
kp = int.from_bytes(TaggedHash("BIP0340/nonce", t + P[0].to_bytes(32, 'big') + msg), 'big') % SECP256K1_ORDER
assert kp != 0
R = SECP256K1.affine(SECP256K1.mul([(SECP256K1_G, kp)]))
```
ACKs for top commit:
practicalswift:
cr ACK f64adc1eedff9d342b49d7e6428b2da21130c23c: untested unused code should be removed
Tree-SHA512: e9afae303488f19c6f6f44874d3537ed1c8164a197490e2b4e34853db886b858825b719648fa1a30b95177dcee9cf241f94ee9b835f0a2cae07024ce38a8c090
6452190841f8da1cdaf899d064974136ab37659f net_processing: simplify MaybeSetPeerAsAnnouncingHeaderAndIDs args (Anthony Towns)
39c2a69bc28eb3e3b5fa15a3965773b459bbf7ad net_processing: move MaybeSetPeerAsAnnouncingHeadersAndIDs into PeerManagerImpl (Anthony Towns)
7b7117efd00acf7609e65d3b4fe5f76e400dda12 net_processing: simplify ProcessGetData and FindTxForGetData args (Anthony Towns)
34207b9004d2069a8fcb32758cd796143eccfb4d net_processing: move FindTxForGetData and ProcessGetData to PeerManagerImpl (Anthony Towns)
d44084883adcf00f50d3d5a9e0c88e3a0b276817 net_processing: simplify PeerManageImpl method args (Anthony Towns)
a490f0a056456d683dd8ef6f89a5af1a13792118 net_processing: move MarkBlockAs*, TipMayBeStale, FindNextBlocksToDL to PeerManagerImpl (Anthony Towns)
052d9bc7e52aea373a316f08d42460ead4ed16c8 net_processing: simplify AlreadyHaveTx args (Anthony Towns)
eeac5062508c98fe58daaec471cdd27f3909b6ec net_processing: move AlreadyHaveTx into PeerManageImpl (Anthony Towns)
9781c08a33569370f191b30cc7e2ce9b5317eb3e net_processing: move some globals into PeerManagerImpl (Anthony Towns)
Pull request description:
Turns some globals into member variables, and simplifies the parameter list for some of net_processing's internal functions. Mostly just serves as a code cleanup at this point.
ACKs for top commit:
jnewbery:
Code review ACK 6452190841f8da1cdaf899d064974136ab37659f
ariard:
Code Review ACK 6452190, changes are pretty straightforward.
MarcoFalke:
Concept ACK 6452190841 I have not reviewed this, but I left a comment 🐡
Tree-SHA512: 381361f9dbfeb851a5522ead3165ce1447a0f212ddea4b483aa38975559ee5ed03a4ba69c24fd69f36847a1eddfef05785f5cbb2fcec5fe50f8b336e8047c3b1
5200929bfe26c549d7da92c0adf8adf61e143416 depends: Include GUIX_ENVIRONMENT in id string (Carl Dong)
4c7d41858821e4fecf7cb0cec3fcad002365e6c9 depends: Improve id string robustness (Carl Dong)
b3bdff42b5a7b4b956da700b187a7254daac54ae build: Proper quoting for var printing targets (Carl Dong)
Pull request description:
```
Environment variables and search paths can drastically effect the
operation of build tools.
Include these in our id string to mitigate against false cache hits.
```
Note to builders: This will invalidate all depends output caches in `BASE_CACHE`
ACKs for top commit:
laanwj:
re-ACK 5200929bfe26c549d7da92c0adf8adf61e143416
Tree-SHA512: e70c98da89cde90dc54bc3be89b925787cf94bbf246e27cc9345816b312073d78a02215448f731f21d8cf033c455234a2377ff1d66c00e1f3db69c9c9687d027
aa929abf8dc022e900755234c857541faeea8239 [docs] Update developer notes to discourage very long lines (John Newbery)
Pull request description:
Mandatory rules on line lengths are bad - there will always be cases where a longer line is more readable than the alternative.
However, very long lines for no good reason _do_ hurt readability. For example, this declaration in validation.h is 274 chars:
```c++
bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);
```
That won't fit on one line without wrapping on my 27" monitor with a comfortable font size. Much easier to read is something like:
```c++
bool ConnectTip(BlockValidationState& state, const CChainParams& chainparams,
CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock,
ConnectTrace& connectTrace, DisconnectedBlockTransactions& disconnectpool)
EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_mempool.cs);
```
Therefore, _discourage_ (don't forbid) line lengths greater than 100 characters in our developer style guide.
100 chars is somewhat arbitrary. The old standard was 80, but that seems very limiting with modern displays.
ACKs for top commit:
fanquake:
ACK aa929abf8dc022e900755234c857541faeea8239 - this is basically just something to point too when a PR has unreasonably long lines for no particularly reason.
practicalswift:
ACK aa929abf8dc022e900755234c857541faeea8239
amitiuttarwar:
ACK aa929abf8dc022e900755234c857541faeea8239
theStack:
ACK aa929abf8dc022e900755234c857541faeea8239
glozow:
ACK aa929abf8d
Tree-SHA512: 17f1b11f811137497ede8851ede93fa612dc622922b5ad7ac8f065ea026d9a718db5b92325754b74d24012b4d45c4e2cd5cd439a6a8d34bbabf5da927d783970
fa051c23860bcdcc871db5ad6b51b8d9ca88da35 doc: Guix is shipped in Debian and Ubuntu (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK fa051c23860bcdcc871db5ad6b51b8d9ca88da35 🚀
Tree-SHA512: f72f546cfc20cf1cc0c26c2306ac06416ada87661596fe811b497cce646aa286dc4aee832145bf838b13fbd3c5f064519eb8c0b4525eb562f2f04f20e2876ffc
9305862f71189d47c873d366bf976622447e18af wallet: load flags before everything else (Sjors Provoost)
Pull request description:
Load and set wallet flags before processing other records. That way we can take them into account while processing those other records.
Suggested here:
https://github.com/bitcoin/bitcoin/pull/16546#discussion_r572334983
ACKs for top commit:
laanwj:
Code review ACK 9305862f71189d47c873d366bf976622447e18af
gruve-p:
ACK 9305862f71
achow101:
ACK 9305862f71189d47c873d366bf976622447e18af
Tree-SHA512: 7104523e369ce3c670571fe5e8b52c67b9ca92b8e36a2da5eb6f9f8bf8ed0544897007257204b68f6f371d682b3ef0d0635d36e6e8416ac74af1999d9fbc869c
c943326d3c06e481c142b112c7e7a0c6ff5a76b3 doc/bips: Add BIPs 43, 44, 49, and 84 (Luke Dashjr)
Pull request description:
If you don't like what they say, please suggest alternatives ;)
ACKs for top commit:
prusnak:
ACK c943326
Tree-SHA512: 7db93f8491289657ec45df30e557eb8572b35201eb29aed1b11bf3949924fce56b4e2d71e1f0acf5d24a01278c0dec99790d632f04c15117010c4ac564368d6b
d4187e46194e7b31f5ace48b08ff64416b967ec4 [test] Use mocktime in test_seed_peers() (Dhruv Mehta)
015637dd445e0158dc763d0d8c55f471d0bc4305 [refactor] Correct log message in net.cpp (Dhruv Mehta)
Pull request description:
The test now takes less than 5 seconds instead of more than 2 minutes
Further context: https://github.com/bitcoin/bitcoin/pull/19884/files#r575336503
Before:
```
2021-02-12T17:22:25.980000Z TestFramework (INFO): Test seed peers, this will take about 2 minutes
2021-02-12T17:24:30.472000Z TestFramework (INFO): Test -networkactive option
```
After:
```
2021-02-12T17:33:39.224000Z TestFramework (INFO): Test seed peers
2021-02-12T17:33:43.139000Z TestFramework (INFO): Test -networkactive option
```
Top commit has no ACKs.
Tree-SHA512: 6d8df7d4433c96268694577e4c10a346785e076d45fa220091875e55def200100e7b827fac2a1f7853a2c2c39e9661e06288dca8c645da9e13d4318a4ff2172e
We can't support a reset of the dbwrapper object when in-memory configuration is used
because it results in the permanent loss of coins. This only affects unittest
configurations (since that's the only place we use in-memory CCoinsViewDB instances).
Values for mainnet and testnet will be specified in a follow-up PR that can be
scrutinized accordingly. This structure is required for use in snapshot activation
logic.
fe3e993968d6b46777d5a16a662cd22790ddf5bb [p2p] No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty. Add -fixedseeds arg. (Dhruv Mehta)
Pull request description:
Closes#19795
Before PR: If `peers.dat` is empty and `-dnsseed=0`, bitcoind will fallback on to fixed seeds but only after a 60 seconds delay.
After PR: There's no 60 second delay.
To reproduce:
`rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0` without and with patch code
Other changes in the PR:
- `-fixedseeds` command line argument added: `-dnsseed=0 -fixedseeds=0 -addnode=X` provides a trusted peer only setup. `-dnsseed=0 -fixedseeds=0` allows for a `addnode` RPC to add a trusted peer without falling back to hardcoded seeds.
ACKs for top commit:
LarryRuane:
re-ACK fe3e993968d6b46777d5a16a662cd22790ddf5bb
laanwj:
re-ACK fe3e993968d6b46777d5a16a662cd22790ddf5bb
Tree-SHA512: 79449bf4e83a315be6dbac9bdd226de89d2a3f7f76d9c5640a2cb3572866e6b0e8ed67e65674c9824054cf13119dc01c7e1a33848daac6b6c34dbc158b6dba8f
060a2a64d40d75fecb60b7d2b9946a67e46aa6fc ci: remove boost thread installation (fanquake)
06e1d7d81d5a56d136c6fc88f09a2b0654a164f9 build: don't build or use Boost Thread (fanquake)
7097add83c8596f81be9edd66971ffd2486357eb refactor: replace Boost shared_mutex with std shared_mutex in sigcache (fanquake)
8e55981ef834490c438436719f95cbaf888c4914 refactor: replace Boost shared_mutex with std shared_mutex in cuckoocache tests (fanquake)
Pull request description:
This replaces `boost::shared_mutex` and `boost::unique_lock` with [`std::shared_mutex`](https://en.cppreference.com/w/cpp/thread/shared_mutex) & [`std::unique_lock`](https://en.cppreference.com/w/cpp/thread/unique_lock).
Even though [some concerns were raised](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) in #16684 with regard to `std::shared_mutex` being unsafe to use across some glibc versions, I still think this change is an improvement. As I mentioned in #21022, I also think trying to restrict standard library feature usage based on bugs in glibc is not only hard to do, but it's not currently clear exactly how we do that in practice (does it also extend to patching out use in our dependencies, should we be implementing more runtime checks for features we are using, when do we consider an affected glibc "old enough" not to worry about? etc). If you take a look through the [glibc bug tracker](https://sourceware.org/bugzilla/describecomponents.cgi?product=glibc) you'll no doubt find plenty of (active) bug reports for standard library code we already using. Obviously not to say we shouldn't try and avoid buggy code where possible.
Two other points:
[Cory mentioned in #21022](https://github.com/bitcoin/bitcoin/pull/21022#issuecomment-769274179):
> It also seems reasonable to me to worry that boost hits the same underlying glibc bug, and we've just not happened to trigger the right conditions yet.
Moving away from Boost to the standard library also removes the potential for differences related to Boosts configuration. Boost has multiple versions of `shared_mutex`, and what you end up using, and what it's backed by depends on:
* The version of Boost.
* The platform you're building for.
* Which version of `BOOST_THREAD_VERSION` is defined: (2,3,4 or 5) default=2. (see [here](https://www.boost.org/doc/libs/1_70_0/doc/html/thread/build.html#thread.build.configuration) for some of the differences).
* Is `BOOST_THREAD_V2_SHARED_MUTEX` defined? (not by default). If so, you might get the ["less performant, but more robust"](https://github.com/boostorg/thread/issues/230#issuecomment-475937761) version of `shared_mutex`.
A lot of these factors are eliminated by our use of depends, but users will have varying configurations. It's also not inconceivable to think that a distro, or some package manager might start defining something like `BOOST_THREAD_VERSION=3`. Boost tried to change the default from 2 to 3 at one point.
With this change, we no longer use Boost Thread, so this PR also removes it from depends, the build system, CI etc.
Previous similar PRs were #19183 & #20922. The authors are included in the commits here.
Also related to #21022 - pthread sanity checking.
ACKs for top commit:
laanwj:
Code review ACK 060a2a64d40d75fecb60b7d2b9946a67e46aa6fc
vasild:
ACK 060a2a64d40d75fecb60b7d2b9946a67e46aa6fc
Tree-SHA512: 572d14d8c9de20bc434511f20d3f431836393ff915b2fe9de5a47a02dca76805ad5c3fc4cceecb4cd43f3ba939a0508178c4e60e62abdbaaa6b3e8db20b75b03
de4238f92f4c067f099663f68d9772105de81d75 build: consolidate reduced export checks (fanquake)
012bdec1b7df01906566a6526e56f27d57d1653b build: add building libconsensus to end-of-configure output (fanquake)
8f360e349e365870b40a6873917c81de714ae41a build: remove ax_gcc_func_attribute macro (fanquake)
f054a089ecfbdc4732e6f705a10e93189074f41c build: remove AX_GCC_FUNC_ATTRIBUTE test for dllimport (fanquake)
7cd0a696643a824ab6f6911278f116f01c5af662 build: test for __declspec(dllexport) in configure (fanquake)
1624e17b5430dfe808bb3b1b79dfa53bf45aa053 build: remove duplicate visibility attribute detection (fanquake)
Pull request description:
Darwin targets do not have a `protected` visibility function attribute, see [LLVM explanation](8e9a505139/clang/lib/Basic/Targets/OSTargets.h (L131)). This means that the `AX_GCC_FUNC_ATTRIBUTE` check for `visibility` fails:
```bash
configure:24513: checking for __attribute__((visibility))
configure:24537: g++ -std=c++11 -o conftest -g -O2 -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DMAC_OSX -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -Wl,-headerpad_max_install_names conftest.cpp >&5
conftest.cpp:35:56: warning: target does not support 'protected' visibility; using 'default' [-Wunsupported-visibility]
int foo_pro( void ) __attribute__((visibility("protected")));
^
1 warning generated.
configure:24537: $? = 0
configure:24550: result: no
```
This leads to `EXPORT_SYMBOL` being [defined to nothing](f4de89edfa/src/script/bitcoinconsensus.h (L29)), as `HAVE_FUNC_ATTRIBUTE_VISIBILITY` is not defined, and when building with reduced exports, you end up with a libbitcoinconsensus.dylib that doesn't export any `_bitcoinconsensus_*` symbols.
```bash
➜ git:(master) nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_
➜ git:(master)
```
We do have a [second check](f4de89edfa/configure.ac (L882)) for the `visibility` attribute, which works for Darwin as it's only testing for default visibility, however the result of this check isn't used at all. It was added in #4725, along with the `--enable-reduce-exports` option, however when libbitcoinconsensus was added in #5235, it used the results of the added `AX_GCC_FUNC_ATTRIBUTE` calls.
This PR removes our usage of the AX_GCC_FUNC_ATTRIBUTE macro entirely, in favour of our own checks in configure. This meant adding a check for `dllexport`, which I've tested as working with both [GCC](https://gcc.gnu.org/onlinedocs/gcc/Microsoft-Windows-Function-Attributes.html) and [Clang](https://releases.llvm.org/10.0.0/tools/clang/docs/AttributeReference.html#dllexport) when building for Windows. I haven't added an equivalent check for `dllimport`, as we weren't actually using the result of that check, we're just testing that `MSC_VER` was defined before using.
With these changes building a libbitcoinconsensus with reduced exports, when targeting Darwin, works as expected:
```bash
./autogen.sh
./configure --disable-tests --disable-bench --with-utils=no --with-daemon=no --with-gui=no --disable-wallet --with-libs=yes --enable-reduce-exports
make -j8
...
nm -C src/.libs/libbitcoinconsensus.dylib | rg _bitcoinconsensus_
000000000000a340 T _bitcoinconsensus_verify_script
00000000000097e0 T _bitcoinconsensus_verify_script_with_amount
000000000000a3c0 T _bitcoinconsensus_version
```
```python
>>> import ctypes
>>> consensus = ctypes.CDLL("src/.libs/libbitcoinconsensus.dylib")
>>> print(consensus.bitcoinconsensus_version())
1
>>> exit()
```
TODO: Modify a CI job to compile with --enable-reduce-exports and check for symbols in shared lib?
ACKs for top commit:
laanwj:
Code review ACK de4238f92f4c067f099663f68d9772105de81d75
Tree-SHA512: d148f3c55d14dac6e9e5b718cc65bb557bcf6f663218d24bc9044b86281bd5dd3d931ebea79c336a58e8ed50d683218c0a9e75494f2267b91097665043e252ae
e987ae5a554c9952812746c29f2766bacea4b727 test: Add test for deterministic UTXO set hash results (Fabian Jahr)
6ccc8fc067bf516cda7bc5d7d721945be5ac2003 test: Add test for gettxoutsetinfo RPC with MuHash (Fabian Jahr)
0d3b2f643d7da3202c0a0e757539208c4aa7c450 rpc: Add hash_type MUHASH to gettxoutsetinfo (Fabian Jahr)
2474645f3b15687e7f196b89eb935d6e6a98a9da refactor: Separate hash and stats calculation in coinstats (Fabian Jahr)
a1fcceac69097a8e6540a6fd8121a5d53022528f refactor: Improve encapsulation between MuHash3072 and Num3072 (Fabian Jahr)
Pull request description:
This is another Pr in the series PRs for Coinstatsindex (see overview in #18000). This PR adds the `hash_type` option `muhash` to `gettxoutsetinfo` through which the user can calculate the serialized muhash of the utxo set. This PR does not use the index yet.
ACKs for top commit:
Sjors:
tACK e987ae5
achow101:
ACK e987ae5a554c9952812746c29f2766bacea4b727
jonatack:
Tested re-ACK e987ae5a554c9952812746c29f2766bacea4b727 per `git diff 3506d90 e987ae5`, reviewed diff, debug built, ran gettxoutsetinfo -signet and help on this branch vs master, at height 23127 both returned `hash_serialized_2` of `2b72d65f3b6efb2311f58374ea2b939abf49684d44f4bafda45faa3b5452a454` and this branch returned `muhash` of `c9f1ff12d345ccf9939c6bbf087e6f7399b6115adee1569287e9c5c43dbb475c`
ryanofsky:
Code review ACK e987ae5a554c9952812746c29f2766bacea4b727. Looks very good. I left one suggestion to simplify code, but feel free to ignore it here and maybe consider it for later since PR has already had a lot of review.
Tree-SHA512: 9a739ce375e73749fa69a467262b60d3e5314ef384e2d7150b3bbc8e4125cd9fd1db95306623bb9a632fcbaf5d9d2bf2f5cc43bf717d4ff5e2c9c4b52dd9296c
We are already testing for this, and our test works correctly with a Darwin
target, where the macro does not. Darwin targets do not support "protected"
visibility.
25f899cc23a791c08e19acae91bebda6c3538d37 log: Move "Pre-allocating up to position 0x[...] in [...].dat" log message to debug category (practicalswift)
acd7980b37b5a71f324f7772d72175c8bd7ab900 log: Move "Leaving block file [...]: [...]" log message to debug category (practicalswift)
Pull request description:
Move `Pre-allocating up to position 0x[…] in […].dat` log message to debug category.
After the cleanup of `-debug=net` log messages PR (#20724) was merged recently the console log now has very high signal to noise ratio. That's great! :)
This PR increases the signal to noise ratio slightly more by moving the most common remaining implementation detail log message (`Pre-allocating up to position 0x[…] in […].dat`) to the debug category where it belongs :)
Expected standard output from `bitcoind` (when in steady state) before this patch:
```
$ src/bitcoind
…
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z Pre-allocating up to position 0x0000000 in blk00000.dat
0000-00-00T00:00:00Z Pre-allocating up to position 0x000000 in rev00000.dat
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
```
Expected standard output from `bitcoind` (when in steady state) after this patch:
```
$ src/bitcoind
…
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
```
I find the latter alternative much easier to visually scan for anomalies (and more aesthetically pleasing TBH!).
Non-GUI users deserve nice interfaces too :)
ACKs for top commit:
laanwj:
ACK 25f899cc23a791c08e19acae91bebda6c3538d37
Tree-SHA512: 5970798c41b041527ebdcbd843c5e136c257c28c3b21fc74102da8970406ca5c0c7e406305c5e6e67de5c1708dc1858af07a77a2e05f44159b7103423e8ab32f
