| Age | Commit message (Collapse) | Author |
|---|
|
location if -logsourcelocations is set
b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 log: Prefix log messages with function name if -logsourcelocations is set (practicalswift)
Pull request description:
Prefix log messages with function name if `-logfunctionnames` is set.
Yes, exactly like `-logthreadnames` but for function names instead of thread names :)
This is a small developer ergonomics improvement: I've found this to be a cheap/simple way to correlate log output and originating function.
For me it beats the ordinary cycle of 1.) try to figure out a regexp matching the static part of the dynamic log message, 2.) `git grep -E 'Using .* MiB out of .* requested for signature cache'`, 3.) `mcedit filename.cpp` (`openemacs filename.cpp` works too!) and 4.) search for log message and scroll up to find the function name :)
Without any logging parameters:
```
$ src/bitcoind -regtest
2020-08-25T03:29:04Z Using RdRand as an additional entropy source
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z block tree size = 1
2020-08-25T03:29:04Z nBestHeight = 0
2020-08-25T03:29:04Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z 0 addresses found from DNS seeds
```
With `-logthreadnames` and `-logfunctionnames`:
```
$ src/bitcoind -regtest -logthreadnames -logfunctionnames
2020-08-25T03:29:04Z [init] [ReportHardwareRand] Using RdRand as an additional entropy source
2020-08-25T03:29:04Z [init] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
2020-08-25T03:29:04Z [init] [LoadChainTip] Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
2020-08-25T03:29:04Z [init] [AppInitMain] block tree size = 1
2020-08-25T03:29:04Z [init] [AppInitMain] nBestHeight = 0
2020-08-25T03:29:04Z [loadblk] [LoadMempool] Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2020-08-25T03:29:04Z [dnsseed] [ThreadDNSAddressSeed] 0 addresses found from DNS seeds
```
ACKs for top commit:
laanwj:
Code review ACK b4511e2e2ed1a6077ae6826a9ee6b7a311293d08
MarcoFalke:
review ACK b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 🌃
Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
|
|
9f21ed4037758f407b536c0dd129f8da83173c79 [test] Check user agent string from test framework connections (John Newbery)
9ce4c3c4c1682032500c97af2d6383897b87c413 [test] Add P2P_SERVICES to p2p.py (John Newbery)
010542614dbebba5f5ad6a58c0554930e9e214fc [test] Move MY_RELAY to p2p.py (John Newbery)
9b4054cb7af22123c7fcc4989e143606a630b2af [test] Move MY_SUBVERSION to p2p.py (John Newbery)
7e158a69104831611462cb555da931331b237c78 [test] Move MY_VERSION to p2p.py (John Newbery)
652311165c4ef298dab71d7162f9054abf439f77 [test] Move MIN_VERSION_SUPPORTED to p2p.py (John Newbery)
Pull request description:
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. This PR moves test framework specific constants to p2p.py.
It also changes the SUBVERSION constant to be a string instead of a bytes object. That means that it needs to be explicitly converted to a bytes object to serialize into a version message. Failing to do so would cause an easy-to-spot bug. This should avoid silent failures like the one solved in #20522.
ACKs for top commit:
laanwj:
Code review ACK 9f21ed4037758f407b536c0dd129f8da83173c79
Tree-SHA512: 41d46575ac0ec36ad074d6c6a5b9cef50b05eeb8ddd8ed0a8f0d0c4617cc7b8baa6580af5b83a668230ce1ac27bf0e56914d0361a48b1b05fd75e2e60350eeaf
|
|
de6b389d5db7b8426313c5be6fbd290f992c5aa8 tests: Test getaddressinfo parent_desc (Andrew Chow)
e4ac869a0a0083e2e3af3b56301bd5c8e0cf650b rpc: Add parent descriptor to getaddressinfo output (Andrew Chow)
bbe4a36152fb8d9c8c3682ca2380f1c88cca61cb wallet: Add GetDescriptorString to DescriptorScriptPubKeyMan (Andrew Chow)
9be1437c49f986e8ed964d5f863b4bbcec340751 descriptors: Add ToNormalizedString and tests (Andrew Chow)
Pull request description:
Adds `parent_desc` field to the `getaddressinfo` RPC to export a public descriptor. Using the given address, `getaddressinfo` will look up which `DescriptorScriptPubKeyMan` can be used to produce that address. It will then return the descriptor for that `DescriptorScriptPubKeyMan` in the `parent_desc` field. The descriptor will be in a normalized form where the xpub at the last hardened step is derived so that the descriptor can be imported to other wallets. Tests are added to check that the correct descriptor is being returned for the wallet's addresses and that these descriptors can be imported and used in other wallets.
As part of this PR, a `ToNormalizedString` function is added to the descriptor classes. This really only has an effect on `BIP32PubkeyProvider`s that have hardened derivation steps. Tests are added to check that normalized descriptors are returned.
ACKs for top commit:
Sjors:
utACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
S3RK:
Tested ACK de6b389
jonatack:
Tested ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8 modulo a few minor comments
fjahr:
Code review ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
meshcollider:
Tested ACK de6b389d5db7b8426313c5be6fbd290f992c5aa8
Tree-SHA512: a633e4a39f2abbd95afd7488484cfa66fdd2651dac59fe59f2b80a0940a2a4a13acf889c534a6948903d701484a2ba1218e3081feafe0b9a720dccfa9e43ca2b
|
|
84716b134e9afca2fba7731de4af1f22fa1b1518 Add "index/blockfilterindex -> validation -> index/blockfilterindex" to expected circular dependencies (Jonas Schnelli)
ab3a0a2fb915d8b8384c30a8b38b4b5cc35236fd Add functional test for blockfilterindex in prune-mode (Jonas Schnelli)
c286a22f7b63a8bd336d5d7606c339053ee0054b Add debug startup parameter -fastprune for more effective pruning tests (Jonas Schnelli)
5e112269c311a559bfded814d3c3c438349a1986 Avoid pruning below the blockfilterindex sync height (Jonas Schnelli)
00d57ff76854938ead800767fb673a8af46eac8e Avoid accessing nullpointer in BaseIndex::GetSummary() (Jonas Schnelli)
6abe9f5b11cd4a5ecb6caca8443fe2950a417842 Allow blockfilter in conjunction with prune (Jonas Schnelli)
Pull request description:
Maintaining the blockfilterindexes in prune mode is possible and may lead to efficient p2p based rescans of wallets (restore backups, import/sweep keys) beyond the prune height (rescans not part of that PR).
This PR allows running the blockfilterindex(es) in conjunction with pruning.
* Bitcoind/Qt will shutdown during startup when missing block data has been detected ([re]enable `-blockfilterindex` when we already have pruned)
* manual block pruning is disabled during blockfilterindex sync
* auto-pruning is delayed during blockfilterindex sync
ToDos:
* [x] Functional tests
ACKs for top commit:
fjahr:
Code review ACK 84716b1
ryanofsky:
Code review ACK 84716b134e9afca2fba7731de4af1f22fa1b1518. Only changes since last review were suggested new FindFilesToPrune argument and test.
benthecarman:
tACK 84716b134e9afca2fba7731de4af1f22fa1b1518
Tree-SHA512: 91d832c6c562c463f7ec7655c08956385413a99a896640b9737bda0183607fac530435d03d87c3c0e70c61ccdfe73fe8f3639bc7d26d33ca7e60925ebb97d77a
|
|
Add a check that new connections from the test framework to the
node have the correct user agent string. This makes bugs easier
to detect if the user agent string ever changes.
|
|
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore specify the nServices value in the calling code,
not in the messages.py module.
|
|
messages.py is for message and primitive data structures. Specifics
about the test framework's p2p implementation should be in p2p.py.
Also rename to P2P_VERSION_RELAY. Also rename msg_version.nRelay to
relay. In Bitcoin Core, this is referred to as fRelay, since it's a
bool, so this field has always been misnamed.
|
|
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_SUBVERSION to p2p.py.
Also rename to P2P_SUBVERSION.
|
|
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MY_VERSION to p2p.py.
Also rename to P2P_VERSION to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.
Also always set the nVersion field in CBlockLocator to 0 and ignore the
field in deserialized messages. The field is not currently used for
anything in Bitcoin Core.
|
|
The messages.py module should contain code and helpers for
[de]serializing p2p messages. Specific usage of those messages should
be in p2p.py. Therefore move MIN_VERSION_SUPPORTED to p2p.py.
Also rename to MIN_P2P_VERSION_SUPPORTED to distinguish it from
other versioning used in Bitcoin/Bitcoin Core.
|
|
de85af5cce727981383ac0fe81f635451b331f23 test: store subversion (user agent) as string in msg_version (Sebastian Falbesoner)
Pull request description:
It seems more natural to treat the "subversion" field (=user agent string, see [BIP 14](https://github.com/bitcoin/bips/blob/master/bip-0014.mediawiki#Proposal)) of a node as pure string rather than a bytestring within the test framework. This is also suggested with the naming prefix in `msg_version.strSubVer`: one probably wouldn't expect a field starting with "str" to be a bytestring that needs further decoding to be useful. This PR moves the encoding/decoding parts to the serialization/deserialization routines so that the user doesn't have to bother with that anymore.
Note that currently, in the master branch the `msg_version.strSubVer` is never read (only in `msg_version.__repr__`); However, one issue that is solved by this PR came up while testing #19509 (not merged yet): A decoding script for binary message capture files takes use of the functional test framework convert it into JSON format. Bytestrings will be convered to hexstrings, while pure strings will (surprise surprise) end up without modification in the file.
So without this patch, we get:
```
$ jq . out.json | grep -m5 strSubVer
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
"strSubVer": "2f5361746f7368693a302e32302e312f"
"strSubVer": "2f5361746f7368693a32312e39392e302f"
```
After this patch:
```
$ jq . out2.json | grep -m5 strSubVer
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
"strSubVer": "/Satoshi:0.20.1/"
"strSubVer": "/Satoshi:21.99.0/"
```
ACKs for top commit:
jnewbery:
utACK de85af5cce727981383ac0fe81f635451b331f23
Tree-SHA512: ff23642705c858e8387a625537dfec82e6b8a15da6d99b8d12152560e52d243ba17431b602b26f60996d897e00e3f37dcf8dc8a303ffb1d544df29a5937080f9
|
|
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
|
|
expected circular dependencies
|
|
|
|
automate helps
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
|
|
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
|
|
|
|
Test case now takes < 5 seconds instead of > 2 minutes
|
|
|
|
|
|
Don't return null snapshotblockhash values to avoid caller complexity/confusion.
|
|
peers.dat is empty
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
|
|
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
|
|
Add -fixedseeds arg.
|
|
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
|
|
fabeb5b9c7f678ab3bc24c1860f8514ac52bb56f fuzz: Disable shuffle when merge=1 (MarcoFalke)
Pull request description:
This should hopefully help make the deletion of fuzz inputs more deterministic.
My tests (N=1) revealed that without this patch 7000 files differ (https://github.com/bitcoin-core/qa-assets/pull/44#issuecomment-768841467). With this patch, "only" 2000 files differ.
ACKs for top commit:
practicalswift:
cr ACK fabeb5b9c7f678ab3bc24c1860f8514ac52bb56f: `-shuffle=0` and `-prefer_small=1` make sense
Tree-SHA512: 21a701f52450d402a91dd6e0b33d564c63a9c3b919738eb9a80c24d48fc5b964088e325470738f39af0d595612c844acc7bf0941590cc2dc8c6f6ee4cb69c861
|
|
Speeds up the zmq test roughly by a factor of 2x (~20 sec. instead of
~40 sec.) and also avoids timeouts on the synchronization methods
(sync_mempool() / sync_blocks()) that happened with a slight chance.
This is due to the fact that there is no upper bound on the trickle
relay time, so even the default of 60s is sometimes too low. Fixed by
enabling immediate tx relay on node1.
|
|
After connecting the subscriber sockets to the node, there is no
guarantee that the node's zmq publisher interfaces are ready yet, which
means that potentially the first expected notification messages could
get lost and the test fails. Currently this is handled by just waiting
for a short period of time (200ms), which works most of the time but is
still problematic, as in some rare cases the setup time takes much
longer, even in the range of multiple seconds.
The solution in this commit approaches the problem by using a more
robust method of syncing up, originally proposed by instagibbs:
1. Generate a block on the node
2. Try to receive a notification on all subscribers
3. If all subscribers get a message within the timeout (1 second),
we are done, otherwise repeat starting from step 1
|
|
The ZMQSubscriber reception methods currently assert that the first
received publisher message has a sequence number of zero. In order to
fix the current test flakiness via "syncing up" to nodes in the setup
phase, we have to cope with the situation that messages get lost and the
first actual received message has a sequence number larger than zero.
|
|
|
|
|
|
|
|
fa0a4d6c605b8ed47796f68068d6273bef7fcaef test: remove assert_blockchain_height (MarcoFalke)
Pull request description:
This simplifies the code and solves intermittent timeouts caused by commit 0d39b5848a7a341cd2b958336861cdd4098e2616.
E.g. https://cirrus-ci.com/task/5196092369272832?command=ci#L3126
```
test 2021-02-08T12:27:56.275000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 127, in main
self.run_test()
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_assumevalid.py", line 180, in run_test
self.assert_blockchain_height(self.nodes[0], 101)
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_assumevalid.py", line 92, in assert_blockchain_height
assert False, "blockchain too short after timeout: %d" % current_height
AssertionError: blockchain too short after timeout: 101
ACKs for top commit:
glozow:
ACK fa0a4d6c60
Tree-SHA512: 3859b0c1581c21f03c775f119204cc3a219e5d86346883634886f6da46feaf254b9c6c49c1ec4581ffe409cdf05f6e91ec38c3976c3daf4a9e67f96ddc1e0dce
|
|
fa362064e383163a2585ffbc71ac1ea3bcc92663 rpc: Return total fee in mempool (MarcoFalke)
Pull request description:
This avoids having to loop over the whole mempool to query each entry's fee
ACKs for top commit:
achow101:
ACK fa362064e383163a2585ffbc71ac1ea3bcc92663
glozow:
ACK https://github.com/bitcoin/bitcoin/pull/20944/commits/fa362064e383163a2585ffbc71ac1ea3bcc92663 🧸
jnewbery:
ACK fa362064e383163a2585ffbc71ac1ea3bcc92663
Tree-SHA512: e2fa1664df39c9e187f9229fc35764ccf436f6f75889c5a206d34fff473fc21efbf2bb143f4ca7895c27659218c22884d0ec4195e7a536a5a96973fc9dd82d08
|
|
|
|
9913419cc9db5f8ce7afa0c3774468c330136064 test: remove type: comments in favour of actual annotations (fanquake)
Pull request description:
Now that we require Python 3.6+, we should be using variable type
annotations directly rather than `# type:` comments.
Also takes care of the discarded value issue in p2p_message_capture.py.
See: https://github.com/bitcoin/bitcoin/pull/19509/files#r571674446.
ACKs for top commit:
MarcoFalke:
review ACK 9913419cc9db5f8ce7afa0c3774468c330136064
jnewbery:
Code review ACK 9913419cc9db5f8ce7afa0c3774468c330136064
Tree-SHA512: 63aba5eef6c1320578f66cf8a6d85ac9dbab9d30b0d21e6e966be8216e63606de12321320c2958c67933bf68d10f2e76e9c43928e5989614cea34dde4187aad8
|
|
0d39b5848a7a341cd2b958336861cdd4098e2616 test: fix timeout decrease in feature_assumevalid (Bruno Garcia)
Pull request description:
This PR fixes the timeout decrease in assert_blockchain_height function.
ACKs for top commit:
practicalswift:
cr ACK 0d39b5848a7a341cd2b958336861cdd4098e2616: patch looks correct
theStack:
ACK 0d39b5848a7a341cd2b958336861cdd4098e2616 ⏲️
Tree-SHA512: ae3c83420b4de4ad41f1b20b6e77c3a26a8c5ac4fb136b2645fde119545a413c61312f76a16473141774bc955d30ac4fc86e5ca6cf729f8978a17d0dab520feb
|
|
Now that we require Python 3.6+, we should be using variable type
annotations directly rather than # type: comments.
Also takes care of the discarded value issue in p2p_message_capture.py.
See: https://github.com/bitcoin/bitcoin/pull/19509/files#r571674446.
|
|
5e0cd25e29541e6c19559fb5c2555e008ed896fa fix the unreachable code at feature_taproot (Bruno Garcia)
Pull request description:
This PR removes the unnecessary return statement at the beginning of the function that makes the rest of the function unreachable.
ACKs for top commit:
practicalswift:
cr ACK 5e0cd25e29541e6c19559fb5c2555e008ed896fa: patch looks correct!
sipa:
ACK 5e0cd25e29541e6c19559fb5c2555e008ed896fa.
theStack:
Tested ACK 5e0cd25e29541e6c19559fb5c2555e008ed896fa 🏔️
sanket1729:
tACK 5e0cd25e29541e6c19559fb5c2555e008ed896fa. I noted this a while ago while fixing feature_taproot.py for elements. Verified that the extreme ranges of CScriptNum are correct and the overflow case for `CHECKSIGADD` works as intended. Adding 1 to 2^31 - 1 results in an overflow, but the interpreter puts a `vch` of corresponding to 2^31 on stack. Even though it cannot be converted to CscriptNum(restricted to 4 bytes), it's result can still be compared by OP_EQUAL.
Tree-SHA512: fff9be3be94f4b3f3ccf24bf588d96e84d14806f82692dccd31631b0e5c79a7575a96c308cb5a4f610ab02e2f854b899f374437c33ecf6d52055d333f2de9b27
|
|
49797c3ccfbb9f7ac9c1fbb574d35b315c103805 tests: Disable bdb dump test when no bdb (Andrew Chow)
1194cf9269e6c4bf67b09c4766f42bf173d12c0a Fix wallet_send.py wallet setup to work with descriptors (Andrew Chow)
fbaea7bfe44822710a36601c6b0febbd5e33dfbd Require legacy wallet for wallet_upgradewallet.py (Andrew Chow)
b1b679e0ab7a9981e3e78424fe8836edd59abf6f Explicitly mark legacy wallet tests as such (Andrew Chow)
09514e1bef46444a67cde9ff13e76bd4b9f8c7ac Setup wallets for interface_zmq.py (Andrew Chow)
4d03ef9a73ceb5ef4e9d184a135bca6bdeb8c311 Use MiniWallet in rpc_net.py (Andrew Chow)
4de23824b0c711ece68f9fc007ffac12126710aa Setup wallets for interface_bitcoin_cli.py (Andrew Chow)
7c71c627d28f0cddaf2349a55336278a681c27c2 Setup wallets with descriptors for feature_notifications (Andrew Chow)
1f1bef8dbab7225884d769a45477ee11d0ebf654 Have feature_filelock.py test both bdb and sqlite, depending on compiled (Andrew Chow)
c77975abc0123b29b0eb3481b8916e7c025b7c4c Disable upgrades tests that require BDB if BDB is not compiled (Andrew Chow)
1f20cac9d41e507901a2811d6db7147d7ab0321b Disable wallet_descriptor.py bdb format check if BDB is not compiled (Andrew Chow)
3641597d7ef6f5097a9e93cab3ef7e0f9c820296 tests: Don't make any wallets unless wallet is required (Andrew Chow)
b9b88f57a9b9a28e0f0614c12ae3012cf5050b10 Skip legacy wallet reliant tests if BDB is not compiled (Andrew Chow)
6f36242389bd3e7eacf594ce90491e8ccca70f3a tests: Set descriptors default based on compilation (Andrew Chow)
Pull request description:
This PR fixes tests for when BDB is not compiled. Tests which rely on or test legacy wallet behavior are disabled and skipped when BDB is not compiled. For the components of some tests that are for legacy wallet things, those parts of the tests are skipped.
For the majority of tests, changes are made so that they can be run with either legacy wallets or descriptor wallets without materially effecting the test. Most tests only need the wallet for balance and transactions, so the type of wallet is not an important part of those tests. Additionally, some tests are wallet agnostic and modified to instead use the test framework's MiniWallet.
ACKs for top commit:
laanwj:
ACK 49797c3ccfbb9f7ac9c1fbb574d35b315c103805
ryanofsky:
Code review ACK 49797c3ccfbb9f7ac9c1fbb574d35b315c103805. Only change since last review is dropping last commit. Previous review w/ suggestions for future followup is https://github.com/bitcoin/bitcoin/pull/20267#pullrequestreview-581508843
Tree-SHA512: 69659f8a81fb437ecbca962f4082c12835282dbf1fba7d9952f727a49e01981d749af9b09feda1c8ca737516c7d7a08ef17e782795df3fa69892d5021b41c1ed
|
|
|
|
|
|
590bda79e876d9b959083105b8c7c41dd87706eb scripted-diff: Remove setup_clean_chain if default is not changed (Fabian Jahr)
98892f39e3d079c73bff7f2a5d5420fa95270497 doc: Improve setup_clean_chain documentation (Fabian Jahr)
Pull request description:
The first commit improves documentation on setup_clean_chain which is misunderstood quite frequently. Most importantly it fixes the TestShell docs which are simply incorrect.
The second commit removes the instances of `setup_clean_clain` in functional tests where it is not changing the default.
This used to be part of #19168 which also sought to rename`setup_clean_chain`.
ACKs for top commit:
jonatack:
ACK 590bda79e876d9b959083105b8c7c41dd87706eb
Tree-SHA512: a7881186e65d31160b8f84107fb185973b37c6e50f190a85c6e2906a13a7472bb4efa9440bd37fe0a9ac5cd2d1e8559870a7e4380632d9a249eca8980b945f3e
|
|
fa61b9d1a68820758f9540653920deaeae6abe79 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24a36b62df35d12ecf6c6370671568c8 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac17f93decd4ee38c956e7aa55983f0d test: Add tests (MarcoFalke)
fac05ccdade8b34c969b9cd9b37b355bc0aabf9c wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937
Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a68820758f9540653920deaeae6abe79
fjahr:
Code review ACK fa61b9d1a68820758f9540653920deaeae6abe79
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
|
|
bff7c66e67aa2f18ef70139338643656a54444fe Add documentation to contrib folder (Troy Giorshev)
381f77be858d7417209b6de0b7cd23cb7eb99261 Add Message Capture Test (Troy Giorshev)
e4f378a505922c0f544b4cfbfdb169e884e02be9 Add capture parser (Troy Giorshev)
4d1a582549bc982d55e24585b0ba06f92f21e9da Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97bec09dd5fcc043d8659d8ec5dfb87c2 Add CaptureMessage (Troy Giorshev)
dbf779d5deb04f55c6e8493ce4e12ed4628638f3 Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67aa2f18ef70139338643656a54444fe only some minor changes: 👚
jnewbery:
utACK bff7c66e67aa2f18ef70139338643656a54444fe
theStack:
re-ACK bff7c66e67aa2f18ef70139338643656a54444fe
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
|
|
Signed-off-by: practicalswift <practicalswift@users.noreply.github.com>
|
|
Co-authored-by: MarcoFalke falke.marco@gmail.com
Co-authored-by: sinetek pitwuu@gmail.com
|
