fa9d5902f7 scripted-diff: gArgs -> args (MarcoFalke)
fa33bc2dab init: Capture copy of blocknotify setting for BlockNotifyCallback (MarcoFalke)
fa40017706 init: Pass reference to ArgsManager around instead of relying on global (MarcoFalke)
Pull request description:
The gArgs global has several issues:
* gArgs is used by each process (bitcoind, bitcoin-qt, bitcoin-wallet, bitcoin-cli, bitcoin-tx, ...), but it is hard to determine which arguments are actually used by each process. For example arguments that have never been registered, but are still used, will always return the fallback value.
* Tests may run several sub-tests, which need different settings. So globals will have to be overwritten, but that is fragile on its own: e.g. https://github.com/bitcoin/bitcoin/pull/19704#issuecomment-678259092 or #19511
The goal is to remove gArgs, but as a first step in that direction this pull will change gArgs in init to use a passed-in reference instead.
ACKs for top commit:
ryanofsky:
Code review ACK fa9d5902f7. Looks good. Nice day to remove some globals, and add some lambdas 👍
fanquake:
ACK fa9d5902f7 - I'm not as familiar with the settings & argument handling code, but this make sense, and is a step in the right direction towards a reduction in the usage of globals. Not a huge fan of the clang-formatting in the scripted diff.
jonasschnelli:
Concept ACK fa9d5902f7
Tree-SHA512: ed00db5f826566c7e3b4d0b3d2ee0fc1a49a6e748e04e5c93bdd694ac7da5598749e73937047d5fce86150d764a067d2ca344ba4ae3eb2704cc5c4fa0d20940f
fad84b7e14 test: Activate segwit in TestChain100Setup (MarcoFalke)
fa11ff2980 test: Pass empty tx pool to block assembler (MarcoFalke)
fa96574b0d test: Move doxygen comment to header (MarcoFalke)
Pull request description:
This fixes not only a TODO in the code, but also prevents a never ending source of uninitialized reads. E.g.
* #18376
* https://github.com/bitcoin/bitcoin/pull/19704#issuecomment-678259092
* ...
ACKs for top commit:
jnewbery:
utACK fad84b7e14
Tree-SHA512: 64cf16a59656d49e022b603f3b06441ceae35a33a4253b4382bc8a89a56e08ad5412c8fa734d0fc7b58586f40ea6d57b348a3b4838bc6890a41ae2ec3902e378
102867c587 net: change CNetAddr::ip to have flexible size (Vasil Dimov)
1ea57ad674 net: don't accept non-left-contiguous netmasks (Vasil Dimov)
Pull request description:
(chopped off from #19031 to ease review)
Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Do not accept
invalid netmasks that have 0-bits followed by 1-bits and only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
ACKs for top commit:
sipa:
utACK 102867c587
MarcoFalke:
Concept ACK 102867c587
ryanofsky:
Code review ACK 102867c587. Just many suggested updates since last review. Thanks for following up on everything!
jonatack:
re-ACK 102867c587 diff review, code review, build/tests/running bitcoind with ipv4/ipv6/onion peers
kallewoof:
ACK 102867c587
Tree-SHA512: d60bf716cecf8d3e8146d2f90f897ebe956befb16f711a24cfe680024c5afc758fb9e4a0a22066b42f7630d52cf916318bedbcbc069ae07092d5250a11e8f762
Before this change `CNetAddr::ip` was a fixed-size array of 16 bytes,
not being able to store larger addresses (e.g. TORv3) and encoded
smaller ones as 16-byte IPv6 addresses.
Change its type to `prevector`, so that it can hold larger addresses and
do not disguise non-IPv6 addresses as IPv6. So the IPv4 address
`1.2.3.4` is now encoded as `01020304` instead of
`00000000000000000000FFFF01020304`.
Rename `CNetAddr::ip` to `CNetAddr::m_addr` because it is not an "IP" or
"IP address" (TOR addresses are not IP addresses).
In order to preserve backward compatibility with serialization (where
e.g. `1.2.3.4` is serialized as `00000000000000000000FFFF01020304`)
introduce `CNetAddr` dedicated legacy serialize/unserialize methods.
Adjust `CSubNet` accordingly. Still use `CSubNet::netmask[]` of fixed 16
bytes, but use the first 4 for IPv4 (not the last 4). Only allow
subnetting for IPv4 and IPv6.
Co-authored-by: Carl Dong <contact@carldong.me>
A netmask that contains 1-bits after 0-bits (the 1-bits are not
contiguous on the left side) is invalid [1] [2].
The code before this PR used to parse and accept such
non-left-contiguous netmasks. However, a coming change that will alter
`CNetAddr::ip` to have flexible size would make juggling with such
netmasks more difficult, thus drop support for those.
[1] https://en.wikipedia.org/wiki/Classless_Inter-Domain_Routing#Subnet_masks
[2] https://tools.ietf.org/html/rfc4632#section-5.1
daed542a12 [net_processing] Move ProcessMessage to PeerLogicValidation (John Newbery)
c556770b5e [net_processing] Change PeerLogicValidation to hold a connman reference (John Newbery)
Pull request description:
Rather than ProcessMessage() being a static function in net_processing.cpp, make it a private member function of PeerLogicValidation. This is the start of moving static functions and global variables into PeerLogicValidation to make it better encapsulated.
ACKs for top commit:
jonatack:
ACK daed542a12 code review and debug tested
promag:
Code review ACK daed542a12.
MarcoFalke:
re-ACK daed542a12, only change is removing second commit 🎴
theStack:
Code Review ACK daed542a12
Tree-SHA512: ddebf410d114d9ad5a9e536950018ff333a347c035d74fcc101fb4a3f20a281782c7eac2b7d1bd1c8f6bc7e59f5b5630fb52c2e1b4c32df454fa584673bd021e
71e0f07e9c util: remove unused c-string variant of atoi64() (Sebastian Falbesoner)
Pull request description:
This is another micro-PR "removing old cruft with potentially sharp edges" (quote by practicalswift, see #19739). Gets rid of the c-string variant of the function `atoi64()`, which is only used in fuzzers and on one place with `wallet/wallet.h` (where it is originally a `std::string` anyways and uses `.c_str()` -- this method call can simply be removed.)
ACKs for top commit:
practicalswift:
ACK 71e0f07e9c -- diff looks correct
laanwj:
ACK 71e0f07e9c
Tree-SHA512: 4d1d28e2f5274fdbe0652e7a0f83dd416f4d19c1e1a49979927960a3ad40b0990eeaa4374656bf2c6998a965a14d62c1bc78303b7d583d3307c17828030a8e3b
356988e200 util: make EncodeBase58Check consume Spans (Sebastian Falbesoner)
f0fce0675d util: make EncodeBase58 consume Spans (Sebastian Falbesoner)
Pull request description:
This PR improves the interfaces for the functions `EncodeBase58{Check}` by using Spans, in a similar fashion to e.g. PRs #19660, #19687. Note that on the master branch there are currently two versions of `EncodeBase58`: one that takes two pointers (marking begin and end) and another one that takes a `std::vector<unsigned char>` const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done for `EncodeBase58Check`, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks).
ACKs for top commit:
laanwj:
Code review ACK 356988e200
Tree-SHA512: 47cfccdd7f3a2d4694bb8785e6e5fd756daee04ce1652ee59a7822e7e833b4a441ae9362b9bd67ea020d2b5b7d927629c9addb6abaa9881d8564fd3b1257f512
7966aa424a Add variables for repeated scripts (MeshCollider)
fec8336ad9 Remove GetScriptForWitness function (MeshCollider)
b887060d06 Replace usage of GetScriptForWitness with GetScriptForDestination (MeshCollider)
Pull request description:
As per this TODO in the code:
> TODO: replace calls to GetScriptForWitness with GetScriptForDestination using the various witness-specific CTxDestination subtypes.
The commit "Add additional check for P2SH before adding extra wrapper" also adds an additional check that the scriptPubKey is a P2SH before auto-wrapping the witness script. We shouldn't wrap the witness script if not. Note: #16251 is even better than this check, please review that.
ACKs for top commit:
instagibbs:
ACK 7966aa424a
jonatack:
Code review re-ACK 7966aa4 per `git range-diff b4d0366 ed266f7 7966aa4`
achow101:
re-ACK 7966aa424a only changes since last is rebase.
Tree-SHA512: 3449e0e83bd842acc7c94544a85367da97ac20d859eefc1a618caef0c98204398c266fe8fb9600b78326df5175402e1ae4a132eb766e2c4485e7cda6a2a95c43
35cd2da623 test: Fix 'make cov' with clang (Hennadii Stepanov)
Pull request description:
This is a follow up of #19688.
With this PR it is possible to do the following:
```
$ ./autogen.sh
$ ./configure --enable-lcov CC=clang CXX=clang++
$ make
$ make cov
```
Currently, on master (8a85377cd0), `make cov` fails to `Processing src/test/test_bitcoin-util_tests.gcda`.
ACKs for top commit:
vasild:
ACK 35cd2da
Crypt-iQ:
ACK 35cd2da
Tree-SHA512: aaf56118e2644064e9738a8279889c617db5805c5c804c904469b24c496bd609f9c5fc2aebcf1a422f8a5ed2eb38bd6e76b484680310b55c36d922b73a4c33cf
dca28634d7 test: ensure OP_1NEGATE satisfies BIP62 minimal push rule (Jon Atack)
e629d07199 Do not turn OP_1NEGATE in scriptSig into 0x0181 in signing code (Pieter Wuille)
Pull request description:
A rebase of #13084 which additionally modifies the test code (unaddressed in the original, assuming sipa is too busy to deal with this at the moment).
Relatively simple bugfix so it'd be good to have merged soon.
Turning OP_1NEGATE into 0x0181 results in a larger-than-necessary data push instead of just actually using the OP_1NEGATE opcode (0x4f). This fails the minimal push rule of BIP 62 and makes the result non-standard.
ACKs for top commit:
fjahr:
Code review ACK dca28634d7
luke-jr:
ACK dca28634d7
jonatack:
ACK dca28634d7
Tree-SHA512: 706d9a2ef20c809dea923e477a873e2fd60db8d0ae64289e510b766a38005c1f31ab0b5883f16b9c7863ff0d3f705e8e413f6121320028ac196b79c3184a4113
Hold a reference to connman rather than a pointer because:
- PeerLogicValidation can't run without a connman
- The pointer never gets reseated
The alternative is to always assert that the pointer is non-null before
dereferencing.
Change the name from connman to m_connman at the same time to conform
with current style guidelines.
37a480e0cd [net] Add addpeeraddress RPC method (John Newbery)
ae8051bbd8 [test] Test that getnodeaddresses() can return all known addresses (John Newbery)
f26502e9fc [addrman] Specify max addresses and pct when calling GetAddresses() (John Newbery)
Pull request description:
Currently addrman only allows a maximum of 1000 records or 23% of all records to be returned in a call to `GetAddr()`. Relax this limit and have the client specify the max records they want. For p2p, behaviour is unchanged (but the rate limiting is set inside net_processing, where it belongs). For RPC, `getnodeaddresses` can now return the complete addrman, which is helpful for testing and monitoring.
Also add a test-only RPC `addpeeraddress`, which adds an IP address:port to addrman. This is helpful for testing (eg #18991).
ACKs for top commit:
naumenkogs:
utACK 37a480e0cd
laanwj:
Code review and lightly manually tested ACK 37a480e0cd
Tree-SHA512: f86dcd410aaebaf6e9ca18ce6f23556e5e4649c1325577213d873aa09967298e65ab2dc19a72670641ae92211a923afda1fe124a82e9d2c1cad73d478ef27fdc
Misbehavior tracking state is now contained in Peer instead of
CNode. It is no longer guarded by cs_main, but instead by a
dedicated m_misbehavior_mutex lock.
This allows us to remove 14 cs_main locks from net_processing.
CAddrMan.GetAddr() would previously limit the number and percentage of
addresses returned (to ADDRMAN_GETADDR_MAX (1000) and
ADDRMAN_GETADDR_MAX_PCT (23) respectively). Instead, make it the callers
responsibility to specify the maximum addresses and percentage they want
returned.
For net_processing, the maximums are MAX_ADDR_TO_SEND (1000) and
MAX_PCT_ADDR_TO_SEND (23). For rpc/net, the maximum is specified by the
client.
01e283068b [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar)
bc5d65b3ca [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar)
2f2e13b6c2 [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar)
7f7b83deb2 [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar)
35839e963b [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar)
4972c21b67 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar)
60156f5fc4 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar)
7b322df629 [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar)
14923422b0 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar)
49efac5cae [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar)
d3698b5ee3 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar)
46578c03e9 [doc] Describe different connection types (Amiti Uttarwar)
442abae2ba [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar)
af59feb052 [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar)
e1bc29812d [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar)
0e52a659a2 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar)
1521c47438 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar)
26304b4100 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar)
3f1b7140e9 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar)
Pull request description:
**This is part 1 of #19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality.
**This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions.
This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types.
(some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.)
Overview of this PR:
* rename `oneshot` to `addrfetch`
* introduce `ConnectionType` enum
* one by one, add different connection types to the enum
* expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts)
* fix the bug in counting different type of connections
* some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive.
ACKs for top commit:
jnewbery:
utACK 01e283068b
laanwj:
Code review ACK 01e283068b, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall
vasild:
ACK 01e283068
sdaftuar:
ACK 01e283068b.
fanquake:
ACK 01e283068b - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now.
jb55:
wow this code was messy before... ACK 01e283068b
Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
Double lock of the same (non-recursive) mutex from the same thread
is producing an undefined behavior. Detect this from DEBUG_LOCKORDER
and react similarly to the deadlock detection.
0a8aa626dd refactor: Make HexStr take a span (Wladimir J. van der Laan)
Pull request description:
Make `HexSt`r take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses.
ACKs for top commit:
elichai:
Code review ACK 0a8aa626dd
hebasto:
re-ACK 0a8aa626dd
jonatack:
re-ACK 0a8aa626dd
Tree-SHA512: 6e178ece5cbac62119c857a10299b1e85422938084c3f03063e17119a5129e0c28016e05a6fabaa4c271a7e0a37c7cd89fa47c435ee19b38a5acfe80d00de992
edc316020e test: Remove duplicate NodeContext hacks (Russell Yanofsky)
Pull request description:
Qt tests currently are currently using two NodeContext structs at the same time, one in interfaces::NodeImpl::m_context, and the other in BasicTestingSetup::m_node, and the tests have hacks transferring state between them.
Fix this by getting rid of the NodeImpl::m_context struct and making it a pointer. This way a common BitcoinApplication object can be used for all qt tests, but they can still have their own testing setups.
Non-test code is changing but non-test behavior is still the same as before.
Motivation for this PR is to be able to remove the "std::move(test.m_node.connman)" and mempool hacks for swapping individual NodeContext members in Qt tests, because followup PR #19099 adds yet another member (wallet_client) that needs to be swapped. After this change, the whole NodeContext struct can be swapped instead of individual members, so the workarounds are less fragile and invasive.
ACKs for top commit:
MarcoFalke:
crACK edc316020e🌮
promag:
ACK edc316020e.
Tree-SHA512: c1650e4127f43a4020304ca7c13b5d9122fb5723aacd8fa1cf855d03c6052fcfb7685810aa2a5ef708561015f0022fecaacbad479295104ca45d2c17579466a4
31cf68a3ad [util] add RunCommandParseJSON (Sjors Provoost)
c17f54ee53 [ci] use boost::process (Sjors Provoost)
32128ba682 [doc] include Doxygen comments for HAVE_BOOST_PROCESS (Sjors Provoost)
3c84d85f7d [build] msvc: add boost::process (Sjors Provoost)
c47e4bbf0b [build] make boost-process opt-in (Sjors Provoost)
929cda5470 configure: add ax_boost_process (Sjors Provoost)
8314c23d7b [depends] boost: patch unused variable in boost_process (Sjors Provoost)
Pull request description:
Prerequisite for external signer support in #16546. Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d).
This adds a new dependency [boost process](https://github.com/boostorg/process/tree/boost-1.64.0). This is part of Boost since 1.64 which is part of `depends`. Because the minimum Boost version is 1.47, this functionality is skipped for older versions of Boost.
Use `./configure --with-boost-process` to opt in, which checks for the presence of Boost::Process.
We add `UniValue runCommandParseJSON(const std::string& strCommand)` to `system.{h,cpp}` which calls an arbitrary command and processes the JSON returned by it. This is currently only called by the test suite.
~For testing purposes this adds a new regtest-only RPC method `runcommand`, as well as `test/mocks/command.py` used by functional tests.~ (this is no longer the case)
TODO:
- [ ] review boost process in #15440
ACKs for top commit:
achow101:
ACK 31cf68a3ad
hebasto:
re-ACK 31cf68a3ad, only rebased (verified with `git range-diff`) and removed an unintentional tab character since the [previous](https://github.com/bitcoin/bitcoin/pull/15382#pullrequestreview-458371035) review.
meshcollider:
Very light utACK 31cf68a3ad, although I am not very confident with build stuff.
promag:
Code review ACK 31cf68a3ad, don't mind the nit.
ryanofsky:
Code review ACK 31cf68a3ad. I left some comments below that could be ignored or followed up later. The current change is clean and comprehensive.
Tree-SHA512: c506e747014b263606e1f538ed4624a8ad7bcf4e025cb700c12cc5739964e254dc04a2bbb848996b170e2ccec3fbfa4fe9e2b3976b191222cfb82fc3e6ab182d
63e9e40b73 test: Add LockStackEmpty() (Hennadii Stepanov)
42b2a95373 test: Repeat deadlock tests (Hennadii Stepanov)
1f96be25b0 Preserve initial state if push_lock() throws exception (Hennadii Stepanov)
Pull request description:
On master (e3fa3c7d67) if the `push_lock()` throws the "potential deadlock detected" exception (via the `potential_deadlock_detected()` call), the `LockData` instance internal state differs from one when the `push_lock()` was called. This non-well behaviour makes (at least) testing brittle.
This PR preserves the `LockData` instance initial state if `push_lock()` throws an exception, and improves the `sync_tests` unit test.
ACKs for top commit:
MarcoFalke:
re-ACK 63e9e40b73
vasild:
ACK 63e9e40
Tree-SHA512: 7679182154ce5f079b44b790faf76eb5f553328dea70a326ff6b600db70e2f9ae015a33a104ca070cb660318280cb79b6b42e37ea5166f26f9e627ba721fcdec
77c507358b Make Hash[160] consume range-like objects (Pieter Wuille)
02c4cc5c5d Make CHash256/CHash160 output to Span (Pieter Wuille)
0ef97b1b10 Make MurmurHash3 consume Spans (Pieter Wuille)
e549bf8a9a Make CHash256 and CHash160 consume Spans (Pieter Wuille)
2a2182c387 Make script/standard's BaseHash Span-convertible (Pieter Wuille)
e63dcc3a67 Add MakeUCharSpan, to help constructing Span<[const] unsigned char> (Pieter Wuille)
567825049f Make uint256 Span-convertible by adding ::data() (Pieter Wuille)
131a2f0337 scripted-diff: rename base_blob::data to m_data (Pieter Wuille)
Pull request description:
This makes use of the implicit constructions and conversions to Span introduced in #18468 to simplify the hash.h interface:
* All functions that take a pointer and a length are changed to take a Span instead.
* The Hash() and Hash160() functions are changed to take in "range" objects instead of begin/end iterators.
ACKs for top commit:
laanwj:
re-ACK 77c507358b
jonatack:
Code review re-ACK 77c5073 per `git range-diff 14ceddd 49fc016 77c5073`
Tree-SHA512: 9ec929891b1ddcf30eb14b946ee1bf142eca1442b9de0067ad6a3c181e0c7ea0c99c0e291e7f6e7a18bd7bdf78fe94ee3d5de66e167401674caf91e026269771
3bd67ba5a4 Test addr response caching (Gleb Naumenko)
cf1569e074 Add addr permission flag enabling non-cached addr sharing (Gleb Naumenko)
acd6135b43 Cache responses to addr requests (Gleb Naumenko)
7cc0e8101f Remove useless 2500 limit on AddrMan queries (Gleb Naumenko)
ded742bc5b Move filtering banned addrs inside GetAddresses() (Gleb Naumenko)
Pull request description:
This is a very simple code change with a big p2p privacy benefit.
It’s currently trivial to scrape any reachable node’s AddrMan (a database of all nodes known to them along with the timestamps).
We do have a limit of one GETADDR per connection, but a spy can disconnect and reconnect even from the same IP, and send GETADDR again and again.
Since we respond with 1,000 random records at most, depending on the AddrMan size it takes probably up to 100 requests for an spy to make sure they scraped (almost) everything.
I even have a script for that. It is totally doable within couple minutes.
Then, with some extra protocol knowledge a spy can infer the direct peers of the victim, and other topological stuff.
I suggest to cache responses to GETADDR on a daily basis, so that an attacker gets at most 1,000 records per day, and can’t track the changes in real time. I will be following up with more improvements to addr relay privacy, but this one alone is a very effective. And simple!
I doubt any of the real software does *reconnect to get new addrs from a given peer*, so we shouldn’t be cutting anyone.
I also believe it doesn’t have any negative implications on the overall topology quality. And the records being “outdated” for at most a day doesn’t break any honest assumptions either.
ACKs for top commit:
jnewbery:
reACK 3bd67ba5a4
promag:
Code review ACK 3bd67ba5a4.
ariard:
Code Review ACK 3bd67ba
Tree-SHA512: dfa5d03205c2424e40a3f8a41af9306227e1ca18beead3b3dda44aa2a082175bb1c6d929dbc7ea8e48e01aed0d50f0d54491caa1147471a2b72a46c3ca06b66f
fae8c28dae Pass mempool pointer to GetCoinsCacheSizeState (MarcoFalke)
fac674db20 Pass mempool pointer to UnloadBlockIndex (MarcoFalke)
faec851b6e test: Simplify cs_main locks (MarcoFalke)
Pull request description:
Split out from #19556
Instead of relying on the implicit mempool global, pass a mempool pointer (which can be `0`). This helps with testing, code clarity and unlocks the features described in #19556.
ACKs for top commit:
jnewbery:
code review ACK fae8c28dae
fjahr:
Code review ACK fae8c28dae
darosior:
Tested ACK fae8c28dae
jamesob:
ACK fae8c28dae ([`jamesob/ackr/19604.1.MarcoFalke.pass_mempool_pointer_to`](https://github.com/jamesob/bitcoin/tree/ackr/19604.1.MarcoFalke.pass_mempool_pointer_to))
Tree-SHA512: fa687518c8cda4a095bdbdfe56e01fae2fb16c13d51efbb1312cd6dc007611fc47f53f475602e4a843e3973c9410e6af5a81d6847bd2399f8262ca7205975728
bcfebb6d55 net: save the network type explicitly in CNetAddr (Vasil Dimov)
100c64a95b net: document `enum Network` (Vasil Dimov)
Pull request description:
(chopped off from https://github.com/bitcoin/bitcoin/pull/19031 to ease review)
Before this change, we would analyze the contents of `CNetAddr::ip[16]`
in order to tell which type is an address. Change this by introducing a
new member `CNetAddr::m_net` that explicitly tells the type of the
address.
This is necessary because in BIP155 we will not be able to tell the
address type by just looking at its raw representation (e.g. both TORv3
and I2P are "seemingly random" 32 bytes).
As a side effect of this change we no longer need to store IPv4
addresses encoded as IPv6 addresses - we can store them in proper 4
bytes (will be done in a separate commit). Also the code gets
somewhat simplified - instead of
`memcmp(ip, pchIPv4, sizeof(pchIPv4)) == 0` we can use
`m_net == NET_IPV4`.
ACKs for top commit:
troygiorshev:
reACK bcfebb6d55 via `git range-diff master 64897c5 bcfebb6`
jonatack:
re-ACK bcfebb6 per `git diff 662bb25 bcfebb6`, code review, debug build/tests clean, ran bitcoind.
laanwj:
Code review ACK bcfebb6d55
Tree-SHA512: 9347e2a50feac617a994bfb46a8f77e31c236bde882e4fd4f03eea4766cd5110216f5f3d24dee91d25218bab7f8bb6e1d2d6212a44db9e34594299fd6ff7606b
f19fdd47a6 test: add test for CChainState::ResizeCoinsCaches() (James O'Beirne)
8ac3ef4699 add ChainstateManager::MaybeRebalanceCaches() (James O'Beirne)
f36aaa6392 Add CChainState::ResizeCoinsCaches (James O'Beirne)
b223111da2 txdb: add CCoinsViewDB::ChangeCacheSize (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
---
In the assumeutxo implementation draft (#15056), once a UTXO snapshot is loaded, a new chainstate object is created after initialization. This means that we have to reclaim some of the cache that we've allocated to the original chainstate (per `dbcache=`) to repurpose for the snapshot chainstate.
Furthermore, it makes sense to have different cache allocations depending on which chainstate is more active. While the snapshot chainstate is working to get to the network tip (and the background validation chainstate is idle), it makes sense that the snapshot chainstate should have the majority of cache allocation. And contrariwise once the snapshot has reached network tip, most of the cache should be given to the background validation chainstate.
This set of changes (detailed in the commit messages) allows us to dynamically resize the various coins caches. None of the functionality introduced here is used at the moment, but will be in the next AU PR (which introduces `ActivateSnapshot`).
`ChainstateManager::MaybeRebalanceCaches()` defines the (somewhat normative) cache allocations between the snapshot and background validation chainstates. I'd be interested in feedback if anyone has thoughts on the proportions I've set there.
ACKs for top commit:
ajtowns:
weak utACK f19fdd47a6 -- didn't find any major problems, but not super confident that I didn't miss anything
fjahr:
Code review ACK f19fdd4
ryanofsky:
Code review ACK f19fdd47a6. Only change since last review is constructor cleanup (no change in behavior). I think the suggestions here from ajtowns and others are good, but shouldn't delay merging the PR (and hold up assumeutxo)
Tree-SHA512: fffb7847fb6993dd4a1a41cf11179b211b0b20b7eb5f7cf6266442136bfe9d43b830bbefcafd475bfd4af273f5573500594aa41fff03e0ed5c2a1e8562ff9269
Before this change, we would analyze the contents of `CNetAddr::ip[16]`
in order to tell which type is an address. Change this by introducing a
new member `CNetAddr::m_net` that explicitly tells the type of the
address.
This is necessary because in BIP155 we will not be able to tell the
address type by just looking at its raw representation (e.g. both TORv3
and I2P are "seemingly random" 32 bytes).
As a side effect of this change we no longer need to store IPv4
addresses encoded as IPv6 addresses - we can store them in proper 4
bytes (will be done in a separate commit). Also the code gets
somewhat simplified - instead of
`memcmp(ip, pchIPv4, sizeof(pchIPv4)) == 0` we can use
`m_net == NET_IPV4`.
Co-authored-by: Carl Dong <contact@carldong.me>
9c69cfe4c5 Add <datadir>/settings.json persistent settings storage. (Russell Yanofsky)
eb682c5700 util: Add ReadSettings and WriteSettings functions (Russell Yanofsky)
Pull request description:
Persistent settings are used in followup PRs #15936 to unify gui settings between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.
ACKs for top commit:
MarcoFalke:
Approach re-ACK 9c69cfe4c5🌾
jnewbery:
utACK 9c69cfe4c5
Tree-SHA512: 39fcc6051717117c9141e934de1d0d3f739484be4685cdf97d54de967c8c816502b4fd0de12114433beaa5c5b7060c810fd8ae4e2b3ce7c371eb729ac01ba2e1
c8992e8959 test: Fix fuzzer compilation on macOS fixes#19557 (freenancial)
Pull request description:
fixes#19557
Before the fix:
```
➜ bitcoin git:(fix-fuzzer-macos) make
Making all in src
CXX test/fuzz/addition_overflow-addition_overflow.o
In file included from test/fuzz/addition_overflow.cpp:7:
./test/fuzz/util.h:335:13: error: no matching function for call to 'AdditionOverflow'
if (AdditionOverflow((uint64_t)fuzzed_file->m_offset, random_bytes.size())) {
^~~~~~~~~~~~~~~~
./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('unsigned long long' vs. 'unsigned long')
NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
^
./test/fuzz/util.h:346:13: error: no matching function for call to 'AdditionOverflow'
if (AdditionOverflow(fuzzed_file->m_offset, n)) {
^~~~~~~~~~~~~~~~
./test/fuzz/util.h:201:16: note: candidate template ignored: deduced conflicting types for parameter 'T' ('long long' vs. 'long')
NODISCARD bool AdditionOverflow(const T i, const T j) noexcept
^
```
After the fix:
```
➜ bitcoin git:(fix-fuzzer-macos) ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=/usr/local/opt/llvm/bin/clang CXX=/usr/local/opt/llvm/bin/clang++ --disable-asm && make clean && make -j5
...
...
CXXLD test/fuzz/uint256_deserialize
Making all in doc/man
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `all-am'.
```
ACKs for top commit:
fanquake:
ACK c8992e8959 - tested that compiling works on macOS.
MarcoFalke:
review ACK c8992e8959
Tree-SHA512: 965cdc61b30db0e2209c91b29f0d42de927a9a5b85e1e70f22d1452e0955f876726c7a8c1d1a5f448f12bf24eec3000802071cd4ae28d8605343fd43d174ca84
c0f09c2c9d fuzz: add missing overrides to signature_checker (Jon Atack)
Pull request description:
These functions in `fuzz/signature_checker.cpp` override virtual member functions and should be marked `override` instead of `virtual`, which is for introducing a new virtual function. The overridden virtual functions are in `script/interpreter.h:151/156/161`.
Also, per MarcoFalke suggestion, add missing parentheses in `fuzz/scriptnum_ops.cpp` and remove useless `unsigned int >= 0` conditional in `fuzz/script.cpp`.
These changes fix 5 compile warnings in gcc 10 and 3 in clang 11/12.
ACKs for top commit:
vasild:
ACK c0f09c2
MarcoFalke:
review ACK c0f09c2c9d
Tree-SHA512: 76ce73ec577c1f23cf8646c31d44dcd6c6303732c47187d041a8921d0d24a50163989a375352ebc221abf2ac337bc0902149be46b6f9eebc071d2f364c407f71
and also
- add missing parentheses in fuzz/scriptnum_ops.cpp
- remove useless unsigned int conditional in fuzz/script.cpp
These changes fix 5 compile warnings in gcc 10.
4455949d6f Make test DoS_mapOrphans deterministic (David Reikher)
Pull request description:
This pull request proposes a solution to make the test `DoS_mapOrphans` in denialofservice_tests.cpp have deterministic coverage.
The `RandomOrphan` function in denialofservice_tests.cpp and the implicitly called function `ecdsa_signature_parse_der_lax` in pubkey.cpp were causing the non-deterministic test coverage.
In the former, if a random orphan was selected the index of which is bigger than the max. orphan index in `mapOrphanTransactions`, the last orphan was returned from `RandomOrphan`. If the random number generated was never large enough, this condition would not be fulfilled and the corresponding branch wouldn't run. The proposed solution is to force one of the 50 dependant orphans to depend on the last orphan in `mapOrphanTransactions` using the newly introduced function `OrphanByIndex` (and passing it a large uint256), forcing this branch to run at least once.
In the latter, if values for ECDSA `R` or `S` (or both) had no leading zeros, some code would not be executed. The solution was to find a constant signature that would be comprised of `R` and `S` values with leading zeros and calling `CPubKey::Verify` at the end of the test with this signature forcing this code to always run at least once at the end even if it hadn't throughout the test.
To test that the coverage is (at least highly likely) deterministic, I ran
`contrib/devtools/test_deterministic_coverage.sh denialofservice_tests/DoS_mapOrphans 1000`
and the result was deterministic coverage across 1000 runs.
Also - removed denialofservice_tests test entry from the list of non-deterministic tests in the coverage script.
ACKs for top commit:
MarcoFalke:
ACK 4455949d6f
Tree-SHA512: 987eb1f94b80d5bec4d4944e91ef43b9b8603055750362d4b4665b7f011be27045808aa9f4c6ccf8ae009b61405f9a1b8671d65a843c3328e5b8acce1f1c00a6
The RandomOrphan function and the function ecdsa_signature_parse_der_lax
in pubkey.cpp were causing non-deterministic test coverage.
Force seed in the beginning of the test to make it deterministic.
The seed is selected carefully so that all branches of the function
ecdsa_signature_parse_der_lax are executed. Prior to this fix, the test
was exhibiting non-deterministic coverage since none of the ECDSA
signatures that were generated during the test had leading zeroes in
either R, S, or both, resulting in some branches of said function not
being executed. The seed ensures that both conditions are hit.
Removed denialofservice_tests test entry from the list of non-deterministic
tests in the coverage script.
ad6c34881d tests: Add fuzzing harness for CBlockPolicyEstimator::{Read,Write} (policy/fees.h) (practicalswift)
614e0807a8 tests: Add fuzzing harness for CBufferedFile::{SetPos,GetPos,GetType,GetVersion} (stream.h) (practicalswift)
7bcc71e5f8 tests: Add fuzzing harness for LoadExternalBlockFile(...) (validation.h) (practicalswift)
9823376030 tests: Add fuzzing harness for CBufferedFile (streams.h) (practicalswift)
f3aa659be6 tests: Add fuzzing harness for CAutoFile (streams.h) (practicalswift)
e507c0799d tests: Add serialization/deserialization fuzzing helpers WriteToStream(…)/ReadFromStream(…) (practicalswift)
e48094a506 tests: Add FuzzedAutoFileProvider which provides a CAutoFile interface to FuzzedDataProvider (practicalswift)
9dbcd6854c tests: Add FuzzedFileProvider which provides a FILE* interface to FuzzedDataProvider using fopencookie (practicalswift)
Pull request description:
Add fuzzing harnesses for `CAutoFile`, `CBufferedFile`, `LoadExternalBlockFile` and other `FILE*` consumers:
* Add `FuzzedFileProvider` which provides a `FILE*` interface to `FuzzedDataProvider` using `fopencookie`
* Add `FuzzedAutoFileProvider` which provides a `CAutoFile` interface to `FuzzedDataProvider`
* Add serialization/deserialization fuzzing helpers `WriteToStream(…)`/`ReadFromStream(…)`
* Add fuzzing harness for `CAutoFile` (`streams.h`)
* Add fuzzing harness for `CBufferedFile` (`streams.h`)
* Add fuzzing harness for `LoadExternalBlockFile(...)` (`validation.h`)
* Add fuzzing harness for `CBlockPolicyEstimator::Read` and `CBlockPolicyEstimator::Write` (`policy/fees.h`)
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
Crypt-iQ:
Tested ACK ad6c348
Tree-SHA512: a38e142608218496796a527d7e59b74e30279a2815450408b7c27a76ed600cebc6b88491e831665a0639671e2d212453fcdca558500bbadbeb32b267751f8f72
f58c4b538e [tests] Remove unnecessary cs_mains in denialofservice_tests (Matt Corallo)
Pull request description:
9fdf05d70c resolved some lock
inversion warnings in denialofservice_tests, but left in a number
of cs_main locks that are unnecessary (introducing lock inversion
warnings in future changes).
ACKs for top commit:
promag:
ACK f58c4b538e.
jonatack:
ACK f58c4b538e verified the test locks correspond to the locks in net/net_processing, and the debug build is clean/unit tests pass.
Tree-SHA512: de2d9b2a8f08081b2ce31e18585e4677b167a11752b797d790c281575d7dfef3587f8be4fc7f8f16771141b6ff0b0145c7488cf30e79256b0043947c67a6182c
0c8461a88e refactor: replace CConnman pointers by references in net_processing.cpp (Sebastian Falbesoner)
Pull request description:
This is a follow-up to the recently merged PR https://github.com/bitcoin/bitcoin/pull/19053, replacing ~~two more types of~~ one more type of pointer (CConnman) by references to increase the code quality -- pointers should either check for `nullptr` or be replaced by references, and the latter strategy seems to be more reasonable.
Again, to keep the review burden managable, the changes are kept simple,
* only tackling `CConnman*` ~~and `BanMan*`~~ pointers
* only within the net_processing module, i.e. no changes that would need adaption in other modules
* keeping the names of the variables as they are
ACKs for top commit:
jnewbery:
utACK 0c8461a88e
MarcoFalke:
ACK 0c8461a88e🕧
Tree-SHA512: 79dc05144bcfb5e0bbc62180285aadcc6199f044fa3016c0f54f7b7f45037415260970037bd63b18fafefb8aef448549dae14b780bafb540fa2373f493a17f71
9fdf05d70c resolved some lock
inversion warnings in denialofservice_tests, but left in a number
of cs_main locks that are unnecessary (introducing lock inversion
warnings in future changes).
fa108d6a75 test: update tests for peer discouragement (Jon Atack)
1a9f462caa gui, doc: rm Ban Score in GUI Peers window/release notes updates (Jon Atack)
Pull request description:
This is the third `-banscore` PR in the mini-series described in #19464. See that PR for the intention and reasoning.
- no longer display "Ban Score" in the GUI peers window and add a release note, plus release note fixups per https://github.com/bitcoin/bitcoin/pull/19464#pullrequestreview-447452052
- update tests (`src/test/denialofservice_tests.cpp` and `test/functional/p2p_leak.py`) from banning to discouragement and per https://github.com/bitcoin/bitcoin/pull/19464#issuecomment-658052518
ACKs for top commit:
jnewbery:
ACK fa108d6a75
laanwj:
ACK fa108d6a75
Tree-SHA512: 58a449b3f47b8cb5490b34e4442ee8675bfad1ce48af4e4fd5c67715b0c1a596fb8e731d42e576b4c3b64627f76e0a68cbb1da9ea9f588a5932fe119baf40d50
cca7c577d5 tests: Add fuzzing harness for ChaCha20Poly1305AEAD (practicalswift)
2fc4e5916c tests: Add fuzzing harness for ChaCha20 (practicalswift)
e9e8aac029 tests: Add fuzzing harness for CHKDF_HMAC_SHA256_L32 (practicalswift)
ec86ca1aaa tests: Add fuzzing harness for poly1305_auth(...) (practicalswift)
4cee53bba7 tests: Add fuzzing harness for AES256CBCEncrypt/AES256CBCDecrypt (practicalswift)
9352c32325 tests: Add fuzzing harness for AES256Encrypt/AES256Decrypt (practicalswift)
Pull request description:
Add fuzzing harness for `AES{CBC,}256{Encrypt,Decrypt}`, `poly1305_auth`, `CHKDF_HMAC_SHA256_L32`, `ChaCha20` and `ChaCha20Poly1305AEAD`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
laanwj:
ACK cca7c577d5
Tree-SHA512: cff9acefe370c12a3663aa55145371df835479c6ab8f6d81bbf84e0f81a9d6b0d94e45ec545f9dd5e1702744eaa7947a1f4ffed0171f446fc080369161afd740
fa53635381 util: Make Assert work with any value (MarcoFalke)
Pull request description:
Goal is to avoid compile failures
ACKs for top commit:
jonatack:
ACK fa53635381
ryanofsky:
Code review ACK fa53635381. Looks like if argument is an lvalue this effectively does:
Tree-SHA512: a5cf47a8bb2fa1bd8b8895774f33de50ad803165d6f7b520351be1cfcd5612d5d97c51d118461331d30640186c470879e5ad19e3333e09e72685c5e4e4f23079
Qt tests currently are currently using two NodeContext structs at the same
time, one in interfaces::NodeImpl::m_context, and the other in
BasicTestingSetup::m_node, and the tests have hacks transferring state between
them.
Fix this by getting rid of the NodeImpl::m_context struct and making it a
pointer. This way a common BitcoinApplication object can be used for all qt
tests, but they can still have their own testing setups.
Non-test code is changing but non-test behavior is still the same as before.
Motivation for this PR is to be able to remove the
"std::move(test.m_node.connman)" and mempool hacks for swapping individual
NodeContext members in Qt tests, because followup PR #19099 adds yet another
member (wallet_client) that needs to be swapped. After this change, the whole
NodeContext struct can be swapped instead of individual members, so the
workarounds are less fragile and invasive.
Persistent settings are used in followup PRs #15936 to unify gui settings
between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to
the loadwallet RPC and maintain a dynamic list of wallets that should be loaded
on startup that also can be shared between bitcoind and bitcoin-qt.
97846d7f5b tests: Add fuzzing harness for BanMan (practicalswift)
deba199f1c tests: Add ConsumeSubNet(...). Move and increase coverage in ConsumeNetAddr(...). (practicalswift)
Pull request description:
Add fuzzing harness for `BanMan`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
Top commit has no ACKs.
Tree-SHA512: f4126c15bbb77638833367d73f58193c8f05d16bed0b1d6c33b39387d5b610ff34af78cd721adb51778062ce3ac5e79756d1c3895ef54c6c80c61dcf056e94ff
fab5586122 doc: Use precise permission flags where possible (MarcoFalke)
Pull request description:
Instead of mentioning the all-encompassing `-whitelist*` settings, change the docs to mention the exact permission flag that will influence the behaviour.
This is needed because in the future, the too-broad `-whitelist*` settings (they either include *all* permission flags or apply to *all* peers) might be deprecated to require the permission flags to be enumerated.
Alternatively, in the future there could be an RPC to set the net permission flags on an existing connection, in which case the `-whitelist*` terminology is of no help.
ACKs for top commit:
jnewbery:
reACK fab5586122
fjahr:
Code review ACK fab5586122
jonatack:
ACK fab5586122
Tree-SHA512: c7dea3e577d90103bb2b0ffab7b7c8640b388932a3a880f69e2b70747fc9213dc1f437085671fd54c902ec2a578458b8a2fae6dbe076642fb88efbf9fa9e679c
20d31bdd92 tests: Avoid fuzzer-specific nullptr dereference in libevent when handling PROXY requests (practicalswift)
Pull request description:
Avoid constructing requests that will be interpreted by libevent as PROXY requests to avoid triggering a `nullptr` dereference. Split out from #19074 as suggested by MarcoFalke.
The dereference (`req->evcon->http_server`) takes place in `evhttp_parse_request_line` and is a consequence of our hacky but necessary use of the internal function `evhttp_parse_firstline_` in the `http_request` fuzzing harness.
The suggested workaround is not aesthetically pleasing, but it successfully avoids the troublesome code path.
`" http:// HTTP/1.1\n"` was a crashing input prior to this workaround.
Before this PR:
```
$ echo " http:// HTTP/1.1" > input
$ src/test/fuzz/http_request input
src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
Running: input
AddressSanitizer:DEADLYSIGNAL
=================================================================
==27905==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000108 (pc 0x55a169b7e053 bp 0x7ffd452f1160 sp 0x7ffd452f10e0 T0)
==27905==The signal is caused by a READ memory access.
==27905==Hint: address points to the zero page.
#0 0x55a169b7e053 in evhttp_parse_request_line depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:1883:37
#1 0x55a169b7d9ae in evhttp_parse_firstline_ depends/work/build/x86_64-pc-linux-gnu/libevent/2.1.11-stable-36daee64dc1/http.c:2041:7
#2 0x55a1687f624e in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/http_request.cpp:51:9
…
$ echo $?
1
```
After this PR:
```
$ echo " http:// HTTP/1.1" > input
$ src/test/fuzz/http_request input
src/test/fuzz/http_request: Running 1 inputs 1 time(s) each.
Running: input
Executed input in 0 ms
***
*** NOTE: fuzzing was not performed, you have only
*** executed the target code on a fixed set of inputs.
***
$ echo $?
0
```
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
Top commit has no ACKs.
Tree-SHA512: 7a6b68e52cbcd6c117487e74e47760fe03566bec09b0bb606afb3b652edfd22186ab8244e8e27c38cef3fd0d4a6c237fe68b2fd22e0970c349e4ab370cf3e304
fa3365430c net: Use mockable time for ping/pong, add tests (MarcoFalke)
faab4aaf2f util: Add count_microseconds helper (MarcoFalke)
Pull request description:
Switch `CNode::m_ping_start` and `CNetMessage::m_time` to mockable time, so that tests can be added.
Mockable time is also type-safe, since it uses `std::chrono`
ACKs for top commit:
jonatack:
Code review re-ACK fa33654 re-read code, verified rebase per `git range-diff 4b5c919 fa94d6f fa33654`, previous tested ACKs still valid
troygiorshev:
ACK fa3365430c
Tree-SHA512: 7d632bd6019ce7c882029e71b667a61517e783af82755a85dd979ef09380934e172dec8b8f91d57b200a30a6e096aeaf01f19fee7f3aed0e0e871c72eb44d70e
fa0540cd46 net: Extract download permission from noban (MarcoFalke)
Pull request description:
It should be possible to grant nodes in a local network (e.g. home, university, enterprise, ...) permission to download blocks even after the maxuploadtarget is hit.
Currently this is only possible by setting the `noban` permission, which has some adverse effects, especially if the peers can't be fully trusted.
Fix this by extracting a `download` permission from `noban`.
ACKs for top commit:
jonatack:
ACK fa0540c
Sjors:
re-utACK fa0540cd46
Tree-SHA512: 255566baa43ae925d93f5d0a3aa66b475a556d1590f662a88278a4872f16a1a05739a6119ae48a293011868042e05cb264cffe5822a50fb80db7333bf44376d9
1cabbddbca refactor: Use uint16_t instead of unsigned short (Aaron Hook)
Pull request description:
I wanted to see if the `up for grabs` label works and looked at PR #17822 originally opend by ahook I saw it had many acks for example by jonatack and practicalswift but needed rebasing.
So I checked out the remote branch rebased it resolved three conflicts and continued the rebase.
Hope everything is as expected (:
ACKs for top commit:
sipsorcery:
ACK 1cabbddbca.
practicalswift:
ACK 1cabbddbca -- patch looks correct :)
laanwj:
ACK 1cabbddbca
hebasto:
ACK 1cabbddbca, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 0e6bf64f274aae5dacb188358b4d5f65ccb207d4f70922f039bc4ed7934709418ddad19f8bfb7462517427837c3d2bb3f86ef284bb40e87119aad2a1e148d9d6
2ad58381ff Clean up separated ban/discourage interface (Pieter Wuille)
b691f2df5f Replace automatic bans with discouragement filter (Pieter Wuille)
Pull request description:
This patch improves performance and resource usage around IP addresses that are banned for misbehavior. They're already not actually banned since #14929, as connections from them are still allowed, but they are preferred for eviction if the inbound connection slots are full.
Stop treating these like manually banned IP ranges, and instead just keep them in a rolling Bloom filter of misbehaving nodes, which isn't persisted to disk or exposed through the ban framework. The effect remains the same: preferred for eviction, avoided for outgoing connections, and not relayed to other peers.
Also change the name of this mechanism to "discouraged" to better reflect reality.
ACKs for top commit:
naumenkogs:
utACK 2ad58381ff
amitiuttarwar:
code review ACK 2ad58381ff
jonatack:
ACK 2ad5838 per changes since last review `git range-diff 3276c14 1f7e0ca 2ad5838`
jnewbery:
Code review ACK 2ad58381ff
Tree-SHA512: 5dedef401d9cbfa026812651303e6286223563dbeed7a10766ed536ac9e3f29ed4bd0df29cc6deadceeb35cbe9f066346add14ef0833958ca9f93d123fe7aab5
40506bf93f test: Test gettxouttsetinfo hash_type option (Fabian Jahr)
f17a4d1c4d rpc: Add hash_type NONE to gettxoutsetinfo (Fabian Jahr)
a712cf6f68 rpc: gettxoutsetinfo can specify hash_type (only legacy option for now) (Fabian Jahr)
605884ef21 refactor: Extract GetBogoSize function (Fabian Jahr)
Pull request description:
This is another intermediate part of the Coinstats Index (tracked in #18000).
Sjors suggested [here](https://github.com/bitcoin/bitcoin/pull/18000#issuecomment-641423019) that the part of the changes in #19145 that don't rely on the new `hash_type` muhash, i.e. that are for `hash_type=none`, could be merged separately from everything involving muhash. So these changes are extracted from #19145 here and can be merged without any other requirements.
Building the index with no UTXO set hash is still valuable because `gettxoutsetinfo` can still be used to audit the `total_amount` for example. By itself this PR is not a huge improvement, `hash_type=none` is speeding up `gettxoutsetinfo` by about 10%, but it enables the implementation of an index on top of it in a follow-up and that means large parts of the index code of Coinstats Index can be merged while reviews for the hashing algorithm might take longer.
ACKs for top commit:
MarcoFalke:
ACK 40506bf93f 🖨
Sjors:
tACK 40506bf93f
Tree-SHA512: 3964c2b8eed427511b1aa9b2ef285dff27dc4d1537d72c3911e435b6e6b40912232da4acb3a09bd19a0372ddffa44103388d8a650169d95a4a727b970d210add
fab80fef61 refactor: Remove unused EnsureChainman (MarcoFalke)
fa34587f1c scripted-diff: Replace EnsureChainman with Assert in unit tests (MarcoFalke)
fa6ef701ad util: Add Assert identity function (MarcoFalke)
fa457fbd33 move-only: Move NDEBUG compile time check to util/check (MarcoFalke)
Pull request description:
The utility function is primarily useful to dereference pointer types, which are known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are started: https://github.com/bitcoin/bitcoin/pull/18923/files#diff-fdb2a1a1d8bc790fcddeb6cf5a42ac55R503 . Instead of silently relying on that assumption, `Assert` can be used to abort the program and avoid UB should the assumption ever be violated.
ACKs for top commit:
promag:
Tested ACK fab80fef61.
ryanofsky:
Code review ACK fab80fef61
Tree-SHA512: 830fba10152ba17d47c4dd42809c7e26f9fe6d38e17a2d5b3f054fd644a5c4c9841286ac421ec9bb28cea9f5faeb659740fcf00de6cc589d423fee7694c42d16
This patch improves performance and resource usage around IP
addresses that are banned for misbehavior. They're already not
actually banned, as connections from them are still allowed,
but they are preferred for eviction if the inbound connection
slots are full.
Stop treating these like manually banned IP ranges, and instead
just keep them in a rolling Bloom filter of misbehaving nodes,
which isn't persisted to disk or exposed through the ban
framework. The effect remains the same: preferred for eviction,
avoided for outgoing connections, and not relayed to other peers.
Also change the name of this mechanism to better reflect reality;
they're not banned, just discouraged.
Contains release notes and several interface improvements by
John Newbery.
84d295e513 tests: Check that segwit inputs in psbt have both UTXO types (Andrew Chow)
4600479058 psbt: always put a non_witness_utxo and don't remove it (Andrew Chow)
5279d8bc07 psbt: Allow both non_witness_utxo and witness_utxo (Andrew Chow)
72f6bec1da rpc: show both UTXOs in decodepsbt (Andrew Chow)
Pull request description:
Due to recent changes to hardware wallets, the full previous transaction will need to be provided for segwit inputs. Since some software may be checking for the existence of a `witness_utxo` to determine whether to produce a segwit signature, we keep that field to ease the transition.
Because all of the sanity checks implemented by the `IsSane` functions were related to having mixed segwit and non-segwit data in a PSBT, those functions are removed as those checks are no longer proper.
Some tests are updated/removed to accommodate this and a simple test added to check that both UTXOs are being added to segwit inputs.
As discussed in the wallet IRC meeting, our own signer will not require `non_witness_utxo` for segwit inputs.
ACKs for top commit:
Sjors:
utACK 84d295e513 (didn't retest compared to 836d6fc, but fortunately HWI's CI tracks our master branch, with a bunch of hardware wallet simulators)
ryanofsky:
Code review re-ACK 84d295e513. No changes since last review, but now I understand the context better. I think it would good to improve the comments as suggested https://github.com/bitcoin/bitcoin/pull/19215#discussion_r447889473 and maybe refer to
meshcollider:
utACK 84d295e513
Tree-SHA512: ccc1fd3c16ac3859f5aca4fa489bd40f68be0b81bbdc4dd51188bbf28827a8642dc8b605a37318e5f16cf40f1c4910052dace2f27eca21bb58435f02a443e940
Also adds CCoinsViewCache::ReallocateCache() to attempt to free
memory that the cacheCoins's allocator may be hanging onto when
downsizing the cache.
Adds `CChainState::m_coins{tip,db}_cache_size_bytes` data members
so that we can reference cache size on a per-chainstate basis for
flushing.
99993489da test: Set -logthreadnames in unit tests (MarcoFalke)
fa4ea997b4 init: Setup scheduler in tests and init in exactly the same way (MarcoFalke)
Pull request description:
Generally the unit tests are single threaded, with the exception of the script check threads, the schedule, and optionally indexer threads.
Like the functional tests, the thread name can serve additional debug information, so set `-logthreadnames` in unit tests.
Can be tested with
```
./src/test/test_bitcoin -l test_suite -t validation_tests/test_combiner_all -- DEBUG_LOG_OUT
ACKs for top commit:
laanwj:
ACK 99993489da
Tree-SHA512: 3bdbfc211da146da64b50b0826246aff5c611a84b69ab896a55b3c9d1adc92c5975da36ab92aee577df82e229c4326b477f4105bfdd1a5df4c9a0b018cf61602
fa525e4d1c net: Avoid wasting inv traffic during IBD (MarcoFalke)
fa06d7e934 refactor: block import implies IsInitialBlockDownload (MarcoFalke)
faba65e696 Add ChainstateManager::ActiveChainstate (MarcoFalke)
fabf3d64ff test: Add FeeFilterRounder test (MarcoFalke)
Pull request description:
Tx-inv messages are ignored during IBD, so it would be nice if we told peers to not send them in the first place. Do that by sending two `feefilter` messages: One when the connection is made (and the node is in IBD), and another one when the node leaves IBD.
ACKs for top commit:
jamesob:
ACK fa525e4d1c ([`jamesob/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d`](https://github.com/jamesob/bitcoin/tree/ackr/19204.1.MarcoFalke.p2p_reduce_inv_traffic_d))
naumenkogs:
utACK fa525e4
gzhao408:
ACK fa525e4d1c
jonatack:
re-ACK fa525e4 checked diff `git range-diff 19612ca fa8a66c fa525e4`, re-reviewed, ran tests, ran a custom p2p IBD behavior test at 9321e0f223.
hebasto:
re-ACK fa525e4d1c, only rebased since the [previous](https://github.com/bitcoin/bitcoin/pull/19204#pullrequestreview-429519667) review (verified with `git range-diff`).
Tree-SHA512: 2c22a5def9822396fca45d808b165b636f1143c4bdb2eaa5c7e977f1f18e8b10c86d4c180da488def38416cf3076a26de15014dfd4d86b2a7e5af88c74afb8eb
fa32adf9dc scripted-diff: TxoutType C++11 scoped enum class (MarcoFalke)
fa95a694c4 doc: Update outdated txnouttype documentation (MarcoFalke)
fa58469c77 rpc: Properly use underlying type in GetAllOutputTypes (MarcoFalke)
fa41c65702 rpc: Simplify GetAllOutputTypes with the Join helper (MarcoFalke)
Pull request description:
Non-scoped enums can accidentally and silently decay into an integral type. Also, the symbol names of the keys are exported to the surrounding (usually global) namespace.
Fix both issues by switching to an `enum class TxoutType` in a (mostly) scripted-diff.
ACKs for top commit:
practicalswift:
ACK fa32adf9dc -- patch looks correct
hebasto:
re-ACK fa32adf9dc, since fa5997bd6fc82e16b597ea96e3c5c665f1f174ab (https://github.com/bitcoin/bitcoin/pull/19114#pullrequestreview-421425198) rebased only (verified with `git range-diff`).
Tree-SHA512: f42a9db47f9be89fa4bdd8d2fb05a16726286d8b12e3d87327b67d723f91c7d5a57deb4b2ddae9e1d16fee7a5f8c00828b6dc8909c5db680fc5e0a3cf07cd465
1087807b2b tests: Provide main(...) function in fuzzer (practicalswift)
Pull request description:
Provide `main(...)` function in fuzzer. Allow building uninstrumented harnesses with only `--enable-fuzz`.
This PR restores the behaviour to how things worked prior to #18008. #18008 worked around an macOS specific issue but did it in a way which unnecessarily affected platforms not in need of the workaround :)
Before this patch:
```
# Build uninstrumented fuzzing harness (no libFuzzer/AFL/other-fuzzer-instrumentation)
$ ./configure --enable-fuzz
$ make
CXXLD test/fuzz/span
/usr/lib/gcc/x86_64-linux-gnu/7/../../../x86_64-linux-gnu/Scrt1.o: In function `_start':
(.text+0x20): undefined reference to `main'
collect2: error: ld returned 1 exit status
Makefile:7244: recipe for target 'test/fuzz/span' failed
make[2]: *** [test/fuzz/span] Error 1
make[2]: *** Waiting for unfinished jobs....
$
```
After this patch:
```
# Build uninstrumented fuzzing harness (no libFuzzer/AFL/other-fuzzer-instrumentation)
$ ./configure --enable-fuzz
$ make
$ echo foo | src/test/fuzz/span
$
```
The examples above show the change in non-macOS functionality. macOS functionality is unaffected by this patch.
ACKs for top commit:
MarcoFalke:
ACK 1087807b2b
Tree-SHA512: 9c16ea32ffd378057c4fae9d9124636d11e3769374d340f68a1b761b9e3e3b8a33579e60425293c96b8911405d8b96ac3ed378e669ea4c47836af06892aca73d
67bb7be864 tests: Add fuzzing harness for CHash{160,256}, C{HMAC_,}SHA{1,256,512}, CRIPEMD160, CSipHasher, etc. (practicalswift)
Pull request description:
Add fuzzing harness for `CHash{160,256}`, `C{HMAC_,}SHA{1,256,512}`, `CRIPEMD160`, `CSipHasher`, etc.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
Top commit has no ACKs.
Tree-SHA512: 5377b361097211a7d0b90a26ed1c6dadb9ecce11349036d19f8c9ad2818cd98709bbcbf1c2361dd18eae122b8dbce1c71bb5aa2e85660677e235b8974ae33fcc
fa8337fcdb clang-format scheduler (MarcoFalke)
fa3d41b5ab doc: Switch scheduler to doxygen comments (MarcoFalke)
fac43f9889 scheduler: Replace stop(true) with StopWhenDrained() (MarcoFalke)
fa9cca0550 doc: Remove unused documentation about unimplemented features (MarcoFalke)
fab2950d70 doc: Switch boost::thread to std::thread in scheduler (MarcoFalke)
fa9819695a test: Remove unused scheduler.h include from the common setup (MarcoFalke)
fa609c4f76 scheduler: Remove unused REVERSE_LOCK (MarcoFalke)
Pull request description:
This accumulates a bunch of cleanup that was long overdue, but I haven't yet gotten around to address. Specifically, but not limited to:
* Remove unused code, documentation and includes
* Upgrade to doxygen documentation
Please refer to the individual commits for more details.
ACKs for top commit:
jnewbery:
utACK fa8337fcdb
Tree-SHA512: 0c825ad9767e2697a3ef1ec1be13fdc2b18eeb7493ad0be5b65cc9f209391e78b17ee66e35e094c5e171c12b0f1624f287a110f6bddaf3024b708877afa8552e
37ae687f95 Add tests for CPubKey serialization/unserialization (Elichai Turkel)
9b8907fade Check size after Unserializing CPubKey (Elichai Turkel)
Pull request description:
Found by practicalswift, closes#19235
Currently all the public API(except the pointer-like API) in CPubKey that sets/constructs a pubkey goes through `CPubKey::Set` which checks if that the length and size match and if not invalidates the key.
This adds the same check to `CPubKey::Unserialize`, sadly I don't see an easy way to just push this to the existing checks in `CPubKey::Set` but it's only a simple condition.
The problem with not invalidating is that if you write a pubkey like: `{0x02,0x00}` it will think the actual length is 33(because of `size()`) and will access uninitialized memory if you call any of the functions on CPubKey.
ACKs for top commit:
practicalswift:
re-ACK 37ae687f95
jonatack:
Code review re-ACK 37ae687 per `git diff eab8ee3 37ae687` only change since last review at eab8ee3 is passing the `pubkey` param by reference to const instead of by value in `src/test/key_tests.cpp::CmpSerializationPubkey`
MarcoFalke:
ACK 37ae687f95
Tree-SHA512: 30173755555dfc76d6263fb6a59f41be36049ffae7b4e1b92b922d668f5e5e2331f7374d5fa10d5d59fc53020d2966156905ffcfa8b8129c1f6d0ca062174ff1
-BEGIN VERIFY SCRIPT-
# General rename helper: $1 -> $2
rename_global() { sed -i "s/\<$1\>/$2/g" $(git grep -l "$1"); }
# Helper to rename TxoutType $1
rename_value() {
sed -i "s/ TX_$1,/ $1,/g" src/script/standard.h; # First strip the prefix in the definition (header)
rename_global TX_$1 "TxoutType::$1"; # Then replace globally
}
# Change the type globally to bring it in line with the style-guide
# (clsses are UpperCamelCase)
rename_global 'enum txnouttype' 'enum class TxoutType'
rename_global 'txnouttype' 'TxoutType'
# Now rename each enum value
rename_value 'NONSTANDARD'
rename_value 'PUBKEY'
rename_value 'PUBKEYHASH'
rename_value 'SCRIPTHASH'
rename_value 'MULTISIG'
rename_value 'NULL_DATA'
rename_value 'WITNESS_V0_KEYHASH'
rename_value 'WITNESS_V0_SCRIPTHASH'
rename_value 'WITNESS_UNKNOWN'
-END VERIFY SCRIPT-
51e9393c1f refactor: s/command/msg_type/ in CNetMsgMaker and CSerializedNetMsg (Sebastian Falbesoner)
Pull request description:
Follow-up PR for #18533 -- another small step towards getting rid of the confusing "command" terminology. Also see PR #18610 which tackled the functional tests.
ACKs for top commit:
MarcoFalke:
ACK 51e9393c1f
Tree-SHA512: bb6f05a7be6823d5c4eab1d05b31fee944e700946827ad9425d59a3957fd879776c88c606319cbe9832d9451b275baedf913b71429ea3e01e4e82bf2d419e819
fa1904e5f0 net: Remove dead logging code (MarcoFalke)
fac12ebf4f net: Avoid redundant and confusing FAILED log (MarcoFalke)
Pull request description:
Remove a redundant and confusing "FAILED" log message and gets rid of the unused return type in `ProcessMessage`
ACKs for top commit:
jnewbery:
utACK fa1904e5f0
gzhao408:
utACK fa1904e5f0
troygiorshev:
ACK fa1904e5f0
naumenkogs:
utACK fa1904e
Tree-SHA512: bfa553d5efa022727ed17877fb7c08c14849d804fe6d6a7ce172d513857beba35de41ea40b27ff1aedf68b81e2cda7b2a948ac985fcaaf1b6cfb96cce4837c90
26acc8dd9b Add sanity check asserts to span when -DDEBUG (Pieter Wuille)
2676aeadfa Simplify usage of Span in several places (Pieter Wuille)
ab303a16d1 Add Span constructors for arrays and vectors (Pieter Wuille)
bb3d38fc06 Make pointer-based Span construction safer (Pieter Wuille)
1f790a1147 Make Span size type unsigned (Pieter Wuille)
Pull request description:
This improves our Span class by making it closer to the C++20 `std::span` one:
* ~~Support conversion between compatible Spans (e.g. `Span<char>` to `Span<const char>`).~~ (done in #18591)
* Make the size type `std::size_t` rather than `std::ptrdiff_t` (the C++20 one underwent the same change).
* Support construction of Spans directly from arrays, `std::string`s, `std::array`s, `std::vector`s, `prevector`s, ... (for all but arrays, this only works for const containers to prevent surprises).
And then make use of those improvements in various call sites.
I realize the template magic used looks scary, but it's only needed to make overload resultion make the right choices. Note that the operations done on values are all extremely simple: no casts, explicit conversions, or warning-silencing constructions. That should hopefully make it simpler to review.
ACKs for top commit:
laanwj:
Code review ACK 26acc8dd9b
promag:
Code review ACK 26acc8dd9b.
Tree-SHA512: 5a5bd346a140edf782b5b3b3f04d9160c7b9e9def35159814a07780ab1dd352545b88d3cc491e0f80d161f829c49ebfb952fddc9180f1a56f1257aa51f38788a
The utility is primarily useful to dereference pointer types, which are
known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are
started. Instead of silently relying on that assumption, Assert can be
used to abort the program and avoid UB should the assumption ever be
violated.
- Move the decision whether to translate an error message to where it is
defined. This simplifies call sites: no more `InitError(Untranslated(...))`.
- Make all functions in `util/error.h` consistently return a
`bilingual_str`. We've decided to use this as error message type so
let's roll with it.
This has no functional changes: no messages are changed, no new
translation messages are defined.
b00266fe0c refactor: replace pointers by references within tx_verify.{h,cpp} (Sebastian Falbesoner)
Pull request description:
This PR gets rid of another unnecessary use of raw pointers, similar to PR #19053 (see also issue #19062 where useful commands for finding potential candidates are listed) but in the tx verification module.
For the functions `CalculateSequenceLocks()` and `SequenceLocks()`, the `prevHeights` vector parameter type is changed to be passed as a reference. Note that there were no checks for null pointers -- if one would pass `nullptr` to one of the functions, the following line would immediately lead to a crash:
dcacea096e/src/consensus/tx_verify.cpp (L32)
ACKs for top commit:
Empact:
Code Review ACK b00266fe0c
Tree-SHA512: 0eb71591467905434082029128bdca4df94988c372af40dca325654f6c002c72a00c73776cb5e72d6de2b2f218649211a5dbf19300a2e01f1841d6034e0f01e0
89f9fef1f7 refactor: Specify boost/thread/thread.hpp explicitly (Hennadii Stepanov)
fad8c890f5 txdb: Remove unused boost/thread (MarcoFalke)
faa958bc28 txindex: Remove unused boost/thread (MarcoFalke)
Pull request description:
There are predefined interruption points for `boost::thread`: https://www.boost.org/doc/libs/1_71_0/doc/html/thread/thread_management.html#interruption_points
However, non-boost threads such as `std::thread` or the `main()` thread can obviously not be interrupted. So remove all unused boost/thread from methods that are never executed in a `boost::thread`.
Most of them were accompanied by a `ShutdownRequested` anyway. So even if the current thread was a `boost::thread`, the interruption point would be redundant. (We only interrupt threads during shutdown)
ACKs for top commit:
fanquake:
ACK 89f9fef1f7
hebasto:
ACK 89f9fef1f7, tested on Linux Mint 19.3 (x86_64), verified shutdown in different scenarios.
Tree-SHA512: 17221dadedf2d107e5bda9e4f371cc4f8ffce6ad27cae41aa2b8f1150d8f1adf23d396585ca4a2dd25b1dc6f0d5c81fecd950d8557966ccb45a6d4a85a331d90
8b3136bd30 refactor: replace CNode pointers by references within net_processing.{h,cpp} (Sebastian Falbesoner)
Pull request description:
This PR is inspired by a [recent code review comment](https://github.com/bitcoin/bitcoin/pull/19010#discussion_r426954791) on a PR that introduced new functions to the net_processing module. The point of the discussion was basically that whenever we pass something not by value (in the concrete example it was about `CNode*` and `CConnman*`) we should either use
* a pointer (```CType*```) with null pointer check or
* a reference (```CType&```)
To keep things simple, this PR for a first approach
* only tackles `CNode*` pointers
* only within the net_processing module, i.e. no changes that would need adaption in other modules
* keeps the names of the variables as they are
I'm aware that PRs like this are kind of a PITA to review, but I think the code quality would increase if we get rid of pointers without nullptr check -- bloating up the code by adding all the missing checks would be the worse alternative, in my opinion.
Possible follow-up PRs, in case this is received well:
* replace CNode pointers by references for net module
* replace CConnman pointers by references for net_processing module
* ...
ACKs for top commit:
MarcoFalke:
ACK 8b3136bd30🔻
practicalswift:
ACK 8b3136bd30
Tree-SHA512: 15b6a569ecdcb39341002b9f4e09b38ed4df077e3a3a50dfb1b72d98bdc9f9769c7c504f106456aa7748af8591af7bb836b72d46086df715ab116e4ac3224b3b
fab860aed4 fuzz: Stop nodes in process_message* fuzzers (MarcoFalke)
6666c828e0 fuzz: Give CNode ownership to ConnmanTestMsg in process_message fuzz harness (MarcoFalke)
Pull request description:
Background is that I saw an integer overflow in net_processing
```
#30629113 REDUCE cov: 25793 ft: 142917 corp: 3421/2417Kb lim: 4096 exec/s: 89 rss: 614Mb L: 1719/4096 MS: 1 EraseBytes-
net_processing.cpp:977:25: runtime error: signed integer overflow: 2147483624 + 100 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior net_processing.cpp:977:25 in
net_processing.cpp:985:9: runtime error: signed integer overflow: -2147483572 - 100 cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior net_processing.cpp:985:9 in
```
Telling from the line numbers, it looks like `nMisbehavior` wrapped around.
Fix that by calling `StopNodes` after each exec, which should clear the node state and thus `nMisbehavior`.
ACKs for top commit:
practicalswift:
ACK fab860aed4
Tree-SHA512: 891c081d5843565d891aec028b6c27ef3fa39bc40ae78238e81d8f784b4d4b49cb870998574725a5159dd03aeeb2e0b9bc3d3bb51d57d1231ef42e3394b2d639
f898ef65c9 tests: Add fuzzing harness for functions in script/sign.h (practicalswift)
c91d2f0615 tests: Add fuzzing harness for functions in script/sigcache.h (practicalswift)
d3d8adb79f tests: Add fuzzing harness for functions in script/interpreter.h (practicalswift)
fa80117cfd tests: Add fuzzing harness for functions in script/descriptor.h (practicalswift)
43fb8f0ca3 tests: Add fuzzing harness for functions in script/bitcoinconsensus.h (practicalswift)
8de72711c6 tests: Fill fuzzing coverage gaps for functions in script/script.h, script/script_error.h and script/standard.h (practicalswift)
c571ecb071 tests: Add fuzzing helper functions ConsumeDataStream, ConsumeTxDestination and ConsumeUInt160 (practicalswift)
Pull request description:
Add fuzzing harnesses for functions in `script/`:
* Add fuzzing helper functions `ConsumeDataStream` and `ConsumeUInt160`
* Fill fuzzing coverage gaps for functions in `script/script.h`, `script/script_error.h` and `script/standard.h`
* Add fuzzing harness for functions in `script/bitcoinconsensus.h`
* Add fuzzing harness for functions in `script/descriptor.h`
* Add fuzzing harness for functions in `script/interpreter.h`
* Add fuzzing harness for functions in `script/sigcache.h`
* Add fuzzing harness for functions in `script/sign.h`
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
ACK f898ef65c9🔉
Tree-SHA512: f6e77b34dc79f23de5fa9e38ac06e6554b5b946ec3e9a67e2bd982e60aca37ce844f785457ef427a5e3b45e31c305456bca8587cc9f4a0b50b3852e39726eb04
9a19c9ada5 Always define the raii_event_tests test suite (Craig Andrews)
Pull request description:
The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test.
This improves upon 95f97f4 actually fixing https://github.com/bitcoin/bitcoin/issues/9493
ACKs for top commit:
MarcoFalke:
ACK 9a19c9ada5🎹
Tree-SHA512: 3c42f17a9b5d56c8841f3aa9ac19da91c10aff210026266f31f7eb98a62528740d7c518c121452b68e8f801d6c80ecfb627d137ec6ed533289fa3beb08b4f176
f871f15c9d scripted-diff: replace gArgs with argsman (glowang)
357f02bf29 Create a local class inherited from BasicTestingSetup with a localized args manager and put it into the getarg_tests namespace (glowang)
Pull request description:
Replaced the global argsManager gArgs with a locally defined one in getarg_tests. This is to avoid confusion in arg settings between the test's ArgsManager and the #18804
ACKs for top commit:
MarcoFalke:
ACK f871f15c9d
ryanofsky:
Code review ACK f871f15c9d. Changes look good and thanks for updating. In future would recommend using clang-format-diff and following [coding style](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) notes, because it's atypical to indent namespace content, or indent protected keywords or put spaces around ::. Also it's fragile to define test setup class in a namespace, but test setup methods outside of the namespace and inside the test fixture instead. Would be simpler to just define the testing setup completely before using it without a namespace like: 8ad5f1c376/src/test/rpc_tests.cpp (L23) and it would have been a slightly smaller change too.
Tree-SHA512: 016594639396d60667fadec8ea80ef7af634fbb2014c704f02406fe3251c5362757c21f1763d8bdb94ca4a3026ab9dc786a92a9a934efc8cd807655d9deee779
5478d6c099 logging: thread safety annotations (Anthony Towns)
e685ca1992 util/system.cpp: add thread safety annotations for dir_locks (Anthony Towns)
a788789948 test/checkqueue_tests: thread safety annotations (Anthony Towns)
479c5846f7 rpc/blockchain.cpp: thread safety annotations for latestblock (Anthony Towns)
8b5af3d4c1 net: fMsgProcWake use LOCK instead of lock_guard (Anthony Towns)
de7c5f41ab wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool (Anthony Towns)
c3cf2f5501 rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable (Anthony Towns)
Pull request description:
In a few cases we need to use `std::mutex` rather than the sync.h primitives. But `std::lock_guard<std::mutex>` doesn't include the clang thread safety annotations unless you also use clang's C library, which means you can't indicate when variables should be guarded by `std::mutex` mutexes.
This adds an annotated version of `std::lock_guard<std::mutex>` to threadsafety.h to fix that, and modifies places where `std::mutex` is used to take advantage of the annotations.
It's based on top of #16112, and turns the thread safety comments included there into annotations.
It also changes the RAII classes in wallet/wallet.h and rpc/blockchain.cpp to just use the atomic<bool> flag for synchronisation rather than having a mutex that doesn't actually guard anything as well.
ACKs for top commit:
MarcoFalke:
ACK 5478d6c099🗾
hebasto:
re-ACK 5478d6c099, only renamed s/`MutexGuard`/`LockGuard`/, and dropped the commit "test/util_threadnames_tests: add thread safety annotations" since the [previous](https://github.com/bitcoin/bitcoin/pull/16127#pullrequestreview-414184113) review.
ryanofsky:
Code review ACK 5478d6c099. Thanks for taking suggestions! Only changes since last review are dropping thread rename test commit d53072ec730d8eec5a5b72f7e65a54b141e62b19 and renaming mutex guard to lock guard
Tree-SHA512: 7b00d31f6f2b5a222ec69431eb810a74abf0542db3a65d1bbad54e354c40df2857ec89c00b4a5e466c81ba223267ca95f3f98d5fbc1a1d052a2c3a7d2209790a
c57f03ce17 refactor: Replace const char* to std::string (Calvin Kim)
Pull request description:
Rationale: Addresses #19000
Some functions should be returning std::string instead of const char*.
This commit changes that.
Main benefits/reasoning:
1. The functions never return nullptr, so returning a string makes code at call sites easier to review (reviewers don't have to read the source code to verify that a nullptr is never returned)
2. All call sites convert to string anyway
ACKs for top commit:
MarcoFalke:
re-ACK c57f03ce17 (no changes since previous review) 🚃
Empact:
Fair enough, Code Review ACK c57f03ce17
practicalswift:
ACK c57f03ce17 -- patch looks correct
hebasto:
re-ACK c57f03ce17
Tree-SHA512: 9ce99bb38fe399b54844315048204cafce0f27fd8f24cae357fa7ac6f5d8094d57bbf5f5c1f5878a65f2d35e4a3f95d527eb17f49250b690c591c0df86ca84fd
71f016c6eb Remove old serialization primitives (Pieter Wuille)
92beff15d3 Convert LimitedString to formatter (Pieter Wuille)
ef17c03e07 Convert wallet to new serialization (Pieter Wuille)
65c589e45e Convert Qt to new serialization (Pieter Wuille)
Pull request description:
This is the final step 🥳 of the serialization improvements extracted from #10785.
It converts the LimitedString wrapper to a new-style formatter, and updates the wallet and Qt code to use the new serialization framework. Finally all remaining old primitives are removed.
ACKs for top commit:
jonatack:
ACK 71f016c6eb reviewed diff, builds/tests/re-fuzzed.
laanwj:
Code review ACK 71f016c6eb
Tree-SHA512: d952194bc73259f6510bd4ab1348a1febbbf9862af30f905991812fb0e1f23f15948cdb3fc662be54d648e8f6d95b11060055d2e7a8c2cb5bf008224870b1ea1
fab6b9d18f validation: Mark g_chainman DEPRECATED (MarcoFalke)
fa1d97b256 validation: Make ProcessNewBlock*() members of ChainstateManager (MarcoFalke)
fa24d49098 validation: Make PruneOneBlockFile() a member of ChainstateManager (MarcoFalke)
fa84b1cd84 validation: Make LoadBlockIndex() a member of ChainstateManager (MarcoFalke)
fa05fdf0f1 net: Pass chainman into PeerLogicValidation (MarcoFalke)
fa7b626d7a node: Add chainman alias for g_chainman (MarcoFalke)
Pull request description:
The global `g_chainman` has recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future.
The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager.
I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all.
ACKs for top commit:
ryanofsky:
Code review ACK fab6b9d18f. Had to be rebased but still looks good
Tree-SHA512: dcbf114aeef4f8320d466369769f22ce4dd8f46a846870354df176c3de9ff17c64630fbd777e7121d7470d7a8564ed8d37b77168746e8df7489c6877e55d7b4f
b3f7f375ef refactor: Remove g_rpc_node global (Russell Yanofsky)
ccb5059ee8 scripted-diff: Remove g_rpc_node references (Russell Yanofsky)
6fca33b2ed refactor: Pass NodeContext to RPC and REST methods through util::Ref (Russell Yanofsky)
691c817b34 Add util::Ref class as temporary alternative for c++17 std::any (Russell Yanofsky)
Pull request description:
This PR removes the `g_rpc_node` global, to get same benefits we see removing other globals and make RPC code more testable, modular, and reusable.
This uses a hybrid of the approaches suggested in #17548. Instead of using `std::any`, which isn't available in c++11, or `void*`, which isn't type safe, it uses a small new `util::Ref` helper class, which acts like a simplified `std::any` that only holds references, not values.
Motivation for writing this was to provide an simpler alternative to #18647 by Harris Brakmić (brakmic) which avoids some shortcomings of that PR (https://github.com/bitcoin/bitcoin/pull/18647#issuecomment-617878826)
ACKs for top commit:
MarcoFalke:
re-ACK b3f7f375ef, only change is adding back const and more tests 🚾
ajtowns:
ACK b3f7f375ef
Tree-SHA512: 56292268a001bdbe34d641db1180c215351503966ff451e55cc96c9137f1d262225d7d7733de9c9da7ce7d7a4b34213a98c2476266b58c89dbbb0f3cb5aa5d70
f9ee0f37c2 Add comments to CustomUintFormatter (Pieter Wuille)
4eb5643e35 Convert everything except wallet/qt to new serialization (Pieter Wuille)
2b1f85e8c5 Convert blockencodings_tests to new serialization (Pieter Wuille)
73747afbbe Convert merkleblock to new serialization (Pieter Wuille)
d06fedd1bc Add SER_READ and SER_WRITE for read/write-dependent statements (Russell Yanofsky)
6f9a1e5ad0 Extend CustomUintFormatter to support enums (Russell Yanofsky)
769ee5fa00 Merge BigEndian functionality into CustomUintFormatter (Pieter Wuille)
Pull request description:
The next step of changes from #10785.
This:
* Adds support for enum serialization to `CustomUintFormatter`, used in `CAddress` for service flags.
* Merges `BigEndian` into `CustomUintFormatter`, used in `CNetAddr` for port numbers.
* Converts everything (except wallet and gui) to use the new serialization framework.
ACKs for top commit:
MarcoFalke:
re-ACK f9ee0f37c2, only change is new documentation commit for CustomUintFormatter 📂
ryanofsky:
Code review ACK f9ee0f37c2. Just new commit adding comment since last review
jonatack:
Code review re-ACK f9ee0f37c2 only change since last review is an additional commit adding Doxygen documentation for `CustomUintFormatter`.
Tree-SHA512: e7a0a36afae592d5a4ff8c81ae04d858ac409388e361f2bc197d9a78abca45134218497ab2dfd6d031e0cce0ca586cf857077b7c6ce17fccf67e2d367c1b6cd4
cd34038cbd Switch from Optional<T> to std::optional<T> (C++17). Run clang-format. (practicalswift)
fb559c1170 tests: Fill fuzzing coverage gaps for functions in util/translation.h (practicalswift)
b74f3d6c45 tests: Fill fuzzing coverage gaps for functions in consensus/validation.h (practicalswift)
c0bbf8193d tests: Fill fuzzing coverage gaps for functions in primitives/block.h (practicalswift)
Pull request description:
* Fill fuzzing coverage gaps for functions in `consensus/validation.h`
* Fill fuzzing coverage gaps for functions in `primitives/block.h`
* Fill fuzzing coverage gaps for functions in `util/translation.h`
* Switch from `Optional<T>` to `std::optional<T>` (C++17). Run `clang-format`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
Top commit has no ACKs.
Tree-SHA512: d6aa4634c3953ade173589a8239bd230eb317ef897835a8557acb73df01b25e5e17bf46f837838e59ec04c1f3d3b7d1309ba68c8a264d17b938215512c9e6085
0000ea3265 test: Add test for GetRandMillis and GetRandMicros (MarcoFalke)
fa0e5b89cf Add templated GetRandomDuration<> (MarcoFalke)
Pull request description:
A naive implementation of this template is dangerous, because the call site might accidentally omit the template parameter:
```cpp
template <typename D>
D GetRandDur(const D& duration_max)
{
return D{GetRand(duration_max.count())};
}
BOOST_AUTO_TEST_CASE(util_time_GetRandTime)
{
std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1});
// Want seconds to be in range [0..1hour), but always get zero :((((
BOOST_CHECK_EQUAL(rand_hour.count(), 0);
}
```
Luckily `std::common_type` is already specialised in the standard lib for `std::chrono::duration` (https://en.cppreference.com/w/cpp/chrono/duration/common_type). And its effect seem to be that the call site must always specify the template argument explicitly.
So instead of implementing the function for each duration type by hand, replace it with a templated version that is safe to use.
ACKs for top commit:
laanwj:
Code review ACK 0000ea3265
promag:
Code review ACK 0000ea3265.
jonatack:
ACK 0000ea3 thanks for the improved documentation. Code review, built, ran `src/test/test_bitcoin -t random_tests -l test_suite` for the new unit tests, `git diff fa05a4c 0000ea3` since previous review:
hebasto:
ACK 0000ea3265 with non-blocking [nit](https://github.com/bitcoin/bitcoin/pull/18781#discussion_r424924671).
Tree-SHA512: e89d46e31452be6ea14269ecbbb2cdd9ae83b4412cd14dff7d1084283092722a2f847cb501e8054394e4a3eff852f9c87f6d694fd008b3f7e8458cb5a3068af7
Fix the following error in travis:
test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor
const BlockValidationState state_dummy;
7777f2a4bb miner: Avoid stack-use-after-return in validationinterface (MarcoFalke)
fa5ceb25fc test: Remove UninterruptibleSleep from test and replace it by SyncWithValidationInterfaceQueue (MarcoFalke)
fa770ce7fe validationinterface: Rework documentation, Rename pwalletIn to callbacks (MarcoFalke)
fab6d060ce test: Add unregister_validation_interface_race test (MarcoFalke)
Pull request description:
When a validationinterface has itself unregistered in one thread, but is about to get executed in another thread [1], there is a race:
* The validationinterface destructing itself
* The validationinterface getting dereferenced for execution
[1] 64139803f1/src/validationinterface.cpp (L82-L83)
This happens in the miner. More generally it happens everywhere where at least one thread is generating notifications and another one is unregistering a validationinterface.
This issue has been fixed in commit ab31b9d6fe, but the fix has not been applied to the miner.
Example where this happened in practice: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675322230#L4414
ACKs for top commit:
promag:
Code review ACK 7777f2a4bb.
laanwj:
Code review ACK 7777f2a4bb
Tree-SHA512: 8087119243c71ba18a823a63515f3730d127162625d8729024278b447af29e2ff206f4840ee3d90bf84f93a2c5ab73b76c7e7044c83aa93b5b51047a166ec3d3
This commit is (intentionally) adding a broken test. The test is broken
because it registering a subscriber object that can go out of scope
while events are still being sent.
To run the broken test and reproduce the bug:
- Remove comment /** and */
- ./configure --with-sanitizers=address
- export ASAN_OPTIONS=detect_leaks=0
- make
- while ./src/test/test_bitcoin -t validationinterface_tests/unregister_validation_interface_race --catch_system_errors=no ; do true; done
68537275bd build: Enable -Werror=sign-compare (Ben Woosley)
eac6a3080d refactor: Rework asmap Interpret to avoid ptrdiff_t (Ben Woosley)
df37377e30 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: 22d11187ee vs 75fb37ce68
ACKs for top commit:
fjahr:
re-ACK 68537275bd
practicalswift:
ACK 68537275bd
Tree-SHA512: 14b5daa38c496fb51548feb30fb4dd179e6f76a8d355f52bc8e2a18f2f9340f0bc98dcf36d8b3d6521045d013891c3103749a4eda88ceef00202a6a0cf93f73c
748977690e Add asmap_direct fuzzer that tests Interpreter directly (Pieter Wuille)
7cf97fda15 Make asmap Interpreter errors fatal and fuzz test it (Pieter Wuille)
c81aefc537 Add additional effiency checks to sanity checker (Pieter Wuille)
fffd8dca2d Add asmap sanity checker (Pieter Wuille)
5feefbe6e7 Improve asmap Interpret checks and document failures (Pieter Wuille)
2b3dbfa5a6 Deal with decoding failures explicitly in asmap Interpret (Pieter Wuille)
1479007a33 Introduce Instruction enum in asmap (Pieter Wuille)
Pull request description:
This improves/documents the failure cases inside the asmap interpreter. None of the changes are bug fixes (they only change behavior for corrupted asmap files), but they may make things easier to follow.
In a second step, a sanity checker is added that effectively executes every potential code path through the asmap file, checking the same failure cases as the interpreter, and more. It takes around 30 ms to run for me for a 1.2 MB asmap file.
I've verified that this accepts asmap files constructed by https://github.com/sipa/asmap/blob/master/buildmap.py with a large dataset, and no longer accepts it with 1 bit changed in it.
ACKs for top commit:
practicalswift:
ACK 748977690e modulo feedback below.
jonatack:
ACK 748977690e code review, regular build/tests/ran bitcoin with -asmap, fuzz build/ran both fuzzers overnight.
fjahr:
ACK 748977690e
Tree-SHA512: d876df3859735795c857c83e7155ba6851ce839bdfa10c18ce2698022cc493ce024b5578c1828e2a94bcdf2552c2f46c392a251ed086691b41959e62a6970821
fa47cf9d95 wallet: Fix typo in assert that is compile-time true (MarcoFalke)
Pull request description:
Commit 92bcd70808 presumably added a check that a `dest` of type `CNoDestination` implies an empty `scriptChange`.
However, it accidentally checked for `boost::variant::empty`, which always returns false: https://www.boost.org/doc/libs/1_72_0/doc/html/boost/variant.html#id-1_3_46_5_4_1_1_16_2-bb
ACKs for top commit:
Sjors:
utACK fa47cf9d95
Tree-SHA512: 9626b1e2947039853703932a362c2ee204e002d3344856eb93eef0e0f833401336f2dfa80fd43b83c8ec6eac624e6302aee771fb67aec436ba6483be02b8d615
1ad8ea2b73 net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (Sebastian Falbesoner)
Pull request description:
The BIP37 bloom filter class `CBloomFilter` contains two flags `isEmpty`/`isFull` together with an update method with the purpose to, according to the comments, "avoid wasting cpu", i.e. the mechanism should serve as an optimization for the trivial cases of empty (all bits zero) or full (all bits one) filters.
However, the real reason of adding those flags (introduced with commit 37c6389c5a by gmaxwell) was a _covert fix_ of [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700), a vulnerability that allowed a divide-by-zero remote node crash.
According to gmaxwell himself (https://github.com/bitcoin/bitcoin/pull/9060#issuecomment-257749165):
> the IsEmpty/IsFull optimizations were largely a pretextual optimization intended to make unexploitable a remote crash vulnerability (integer division by zero) that existed in the original bloom filtering code without disclosing it. I'm doubtful that they are all that useful. :)
For more information on how to trigger this crash, see PR https://github.com/bitcoin/bitcoin/pull/18515 which contains a detailled description and a regression test. It has also been discussed on a [recent PR club meeting on fuzzing](https://bitcoincore.reviews/18521.html).
The covert fix code already led to issues and PR based on the wrong assumption that the flags are there for optimization reasons (see #16886 and #16922). This PR gets rid of the flags and the update method and just focuses on the CVE fix itself, i.e. it can be seen as a revert of the covert fix commit modulo the actual fix.
ACKs for top commit:
meshcollider:
utACK 1ad8ea2b73
laanwj:
Concept and code review ACK 1ad8ea2b73
jkczyz:
ACK 1ad8ea2b73
MarcoFalke:
ACK 1ad8ea2b73
fjahr:
Code review ACK 1ad8ea2b73
Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
38e49ded8b tests: Add fuzzing harness for MessageSign, MessageVerify and other functions in util/message.h (practicalswift)
Pull request description:
Add fuzzing harness for `MessageSign`, `MessageVerify` and other functions in `util/message.h`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
vasild:
utACK 38e49ded8b
Tree-SHA512: 4f83718365d9c7e772a4ccecb31817bf17117efae2bfaf6e9618ff17908def0c8b97b5fa2504d51ab38b2e6f82c046178dd751495cc37ab4779c0b1ac1a4d211
b56607a89b Remove CCoinsViewCache::GetValueIn(...) (practicalswift)
Pull request description:
Remove `CCoinsViewCache::GetValueIn(...)`.
Fixes#18858.
It seems like `GetValueIn` was added in #748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in #8498 ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017).
`CCoinsViewCache::GetValueIn(…)` performs money summation like this:
```c++
CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
{
if (tx.IsCoinBase())
return 0;
CAmount nResult = 0;
for (unsigned int i = 0; i < tx.vin.size(); i++)
nResult += AccessCoin(tx.vin[i].prevout).out.nValue;
return nResult;
}
```
Note that no check is done to make sure that the resulting `nResult` is such that it stays within the money bounds (`MoneyRange(nResult)`), or that the summation does not trigger a signed integer overflow.
Proof of concept output:
```
coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long'
GetValueIn = -9221444073709551616
```
Proof of concept code:
```c++
CMutableTransaction mutable_transaction;
mutable_transaction.vin.resize(4393);
Coin coin;
coin.out.nValue = MAX_MONEY;
assert(MoneyRange(coin.out.nValue));
CCoinsCacheEntry coins_cache_entry;
coins_cache_entry.coin = coin;
coins_cache_entry.flags = CCoinsCacheEntry::DIRTY;
CCoinsView backend_coins_view;
CCoinsViewCache coins_view_cache{&backend_coins_view};
CCoinsMap coins_map;
coins_map.emplace(COutPoint{}, std::move(coins_cache_entry));
coins_view_cache.BatchWrite(coins_map, {});
const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction});
std::cout << "GetValueIn = " << total_value_in << std::endl;
```
ACKs for top commit:
MarcoFalke:
ACK b56607a89b
promag:
Code review ACK b56607a89b.
jb55:
ACK b56607a89b
hebasto:
ACK b56607a89b, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6
06e434d7d9 test: fix message for ECC_InitSanityCheck test (fanquake)
Pull request description:
OpenSSL is long gone.
ACKs for top commit:
laanwj:
Good catch. ACK 06e434d7d9
Tree-SHA512: 1a920fd6493e0374ca00633407e0130f987b136bc68d2062402747bda16a1e588a12bd8b0b8cdef828c9911f210386cfbdb25d478cb9b684d52769d197032064
692f8307fc test: add test for witness commitment index (fanquake)
06442549f8 validation: Add minimum witness commitment size constant (fanquake)
Pull request description:
16101de5f3: Per [BIP 141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Commitment_structure), the witness commitment structure is at least 38 bytes,
OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte
SHA256 hash. It can be longer, however any additional data has no
consensus meaning.
54f8c48d6a: As per BIP 141, if there is more than 1 pubkey that matches the witness
commitment structure, the one with the highest output index should be
chosen. This adds a sanity check that we are doing that, which will fail
if anyone tries to "optimize" GetWitnessCommitmentIndex() by returning
early.
ACKs for top commit:
MarcoFalke:
ACK 692f8307fc🌵
jonatack:
Code review ACK 692f830
ajtowns:
ACK 692f8307fc
jnewbery:
utACK 692f8307fc
laanwj:
ACK 692f8307fc
Tree-SHA512: 7af3fe4b8a52fea2cdd0aec95f7bb935351a77b73d934bc88d6625a3503311b2a062cba5190b2228f97caa76840db3889032d910fc8e318ca8e7810a8afbafa0
32b6b386a5 tests: Sort fuzzing harnesses (practicalswift)
e1e181fad1 tests: Add fuzzing coverage for JSONRPCTransactionError(...) and RPCErrorFromTransactionError(...) (practicalswift)
103b6ecce0 tests: Add fuzzing coverage for TransactionErrorString(...) (practicalswift)
dde508b8b0 tests: Add fuzzing coverage for ParseFixedPoint(...) (practicalswift)
1532259fca tests: Add fuzzing coverage for FormatHDKeypath(...) and WriteHDKeypath(...) (practicalswift)
90b635e84e tests: Add fuzzing coverage for CHECK_NONFATAL(...) (practicalswift)
a4e3d13df6 tests: Add fuzzing coverage for StringForFeeReason(...) (practicalswift)
a19598cf98 tests: Add fuzzing harness for functions in system.h (ArgsManager) (practicalswift)
Pull request description:
Add fuzzing harnesses for various classes/functions in `util/`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
Top commit has no ACKs.
Tree-SHA512: d27947220850c2a202c7740f44140c17545f45522596912452ccab0c2f5379abeb07cc769982c7855cb465059425206371a2b75ee1c285b03984161c9619d0b0
As per BIP 141, if there is more than 1 pubkey that matches the witness
commitment structure, the one with the highest output index should be
chosen. This adds a sanity check that we are doing that, which will fail
if anyone trys to "optimise" GetWitnessCommitmentIndex() be returning
early.
Add unit test calling CreateWalletFromFile, which isn't currently called from
other unit tests, with some basic checks to make sure it rescans and registers
for notifications correctly.
Motivation for this change was to try to write a test that would fail without
the early `handleNotifications` call in ef8c6ca60767cac589d98ca57ee33179608ccda8
from https://github.com/bitcoin/bitcoin/pull/16426, but succeed with it:
ef8c6ca607/src/wallet/wallet.cpp (L3978-L3986)
However, writing a full test for the race condition that call prevents isn't
possible without the locking changes from #16426. So this PR just adds as much
test coverage as is possible now.
This new test is also useful for https://github.com/bitcoin/bitcoin/pull/15719,
since it detects the stale notifications.transactionAddedToMempool notifications
that PR eliminates.
fd8e99da57 tests: Add fuzzing harness for functions in primitives/transaction.h (practicalswift)
d5a31b7cb4 tests: Add fuzzing harness for functions in primitives/block.h (practicalswift)
Pull request description:
Add fuzzing harnesses for various classes/functions in `primitives/`.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
Top commit has no ACKs.
Tree-SHA512: ed54bd5b37ff5e40cfa8d3cd8c65d91a2f64fca87b6a5c3b8ddd6becd876ed172735fb53da4d00a86f318fb94517afd179e07cb28a43edf301ffe4dad703cca4
21fa0a44ab [docs] use consistent naming for possible_overwrite (John Newbery)
2685c214cc [tests] small whitespace fixup (John Newbery)
e9936966c0 scripted-diff: Rename PRUNED to SPENT in coins tests (John Newbery)
c205979031 [docs] Improve commenting in coins.cpp|h (John Newbery)
Pull request description:
- Add full commenting for spentness / DIRTYness / FRESHness and which combinations are valid
- Remove the 'pruned' terminology, which doesn't make sense since per-txout chainstate db was merged (#10195).
- Rename `potential_overwrite` to `possible_overwrite` to standardize terminology (there were previously examples of both, which made searching the codebase difficult).
- Make other minor improvements to the comments
ACKs for top commit:
jonatack:
Re-ACK 21fa0a4 per `git diff 98bee55 21fa0a4` the only change since my previous review is the following code commenting diff in `src/coins.cpp::L177-179`; rebuilt/ran unit tests anyway as a sanity check on the unit test changes.
Tree-SHA512: 391e01588ef5edb417250080cec17361f982c4454bc5f8c6d78bbd528c68a2bb94373297760691295c24660ce1022ad3ef7599762f736c8eed772ce096d38c3d
ccccd51908 script: Remove undocumented and unused operator+ (MarcoFalke)
Pull request description:
This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data.
Removing the operator is also going to protect against accidentally reintroducing bugs like this 6ff5f718b6 (diff-8458adcedc17d046942185cb709ff5c3L1135) (last time it was used).
ACKs for top commit:
laanwj:
ACK ccccd51908
Tree-SHA512: 43898ac77e4d9643d9f8ac6f8f65497a4f0bbb1fb5dcaecc839c3719aa36181ba77befb213e59a9f33a20a29e0173a0e9c4763b1930940b32c3d1598b3e39af9
3718ae2ef8 [tests] Don't initialize PrecomputedTransactionData in txvalidationcache tests (John Newbery)
Pull request description:
PrecomputedTransactionData is initialized inside CheckInputScripts(). No need to pre-initialize it before calling into CheckInputScripts().
Normally, I wouldn't bother, but we're making changes to `PrecomputedTransactionData` in #17977 which would break these tests without removing these constructions. Might as well get these changes out of the way here.
ACKs for top commit:
robot-visions:
ACK 3718ae2ef8
sipa:
utACK 3718ae2ef8
Tree-SHA512: bc9c095035a7072a2a91941df38cdbb969e817264efbaa6dcb88cc3ab132d9264aa0751fa588d1a5e45f37b4d2bb1903cda078765f0bbcc87d9cc47cbec5356a
6f8b498d18 fuzz: http_request workaround for libevent < 2.1.1 (Sebastian Falbesoner)
Pull request description:
The fuzz test `http_request` calls the following two internal libevent functions:
* `evhttp_parse_firstline_`
* `evhttp_parse_headers_`
Before libevent 2.1.1 however, internal functions names didn't end with an underscore (see libevent commit 8ac3c4c25b and [Changelog for 2.1.1.-alpha](https://github.com/libevent/libevent/blob/master/ChangeLog#L1830) when the change was first mentioned) hence the build fails with a linking error.
This PR adds a preprocessor workaround to the test that checks for the libevent version (via ~`_EVENT_NUMERIC_VERSION`~ `LIBEVENT_VERSION_NUMBER`) and creates wrapper functions mapping to naming scheme without underscore in case the version is older than 2.1.1.
Tested with Ubuntu Xenial 16.04.6 LTS and clang-8.
ACKs for top commit:
hebasto:
ACK 6f8b498d18, tested on xenial:
Tree-SHA512: 3b9e0147b8aea22e417d418e3b6d4905f5be131c2b0ae4b0f8b9411c5606d2e22f1b23e1ecc6980ecab907c61404de09e588aae1ac43cf70cf9e8d006bbdee73
fa4632c417 test: Move boost/stdlib includes last (MarcoFalke)
fa488f131f scripted-diff: Bump copyright headers (MarcoFalke)
fac5c37300 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 fa4632c417
jonatack:
ACK fa4632c417, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
69ffddc83e refactor: Remove unused methods CBloomFilter::reset()/clear() (Sebastian Falbesoner)
Pull request description:
The method `CBloomFilter::reset()` was introduced by commit d2d7ee0e86 in 2015, but was never ever used, as far as I could find. As discovered by MarcoFalke, the method `clear()` is also unused outside of unit tests and is hence also removed.
ACKs for top commit:
MarcoFalke:
re-ACK 69ffddc83e
jonatack:
ACK 69ffddc83e, code review, compiled a fuzz build and started the bloom_filter fuzz test as a sanity check.
promag:
ACK 69ffddc83e.
Tree-SHA512: 6c53678545ad8e2fa1ffc0a8838e450462f26748a60632f738dc020f0eb494ae2c32841e6256e266ed9140177257a78b707123421942f3819a14ffcb9a99322f
bee88b8c58 tests: have coins simulation test also use CCoinsViewDB (James O'Beirne)
Pull request description:
Before this change, the coins simulation test uses a base view of type
CCoinsViewTest, which has no relevance outside of the unittest suite. Might as
well reuse this testcase with a more realistic configuration that has
CCoinsViewDB (i.e. in-memory leveldb) at the bottom of the view structure.
This adds explicit use of CCoinsViewDB in the unittest suite.
#### Before change
```
./src/test/test_bitcoin --run_test=coins_tests --catch_system_errors=no 21.99s user 0.04s system 99% cpu 22.057 total
```
#### After change
```
./src/test/test_bitcoin --run_test=coins_tests --catch_system_errors=no 78.80s user 0.04s system 100% cpu 1:18.82 total
```
ACKs for top commit:
ryanofsky:
Code review ACK bee88b8c58
Tree-SHA512: 75296b2bcbae2f46e780489aafb032592544a15c384d569d016005692fe79fe60d7f05857cf25cc7b0f9ab1c53b47886a6c71cca074a03fb9afec30e1f376858
fa176e253f test: Avoid accessing free'd memory in validation_chainstatemanager_tests (MarcoFalke)
Pull request description:
ACKs for top commit:
ryanofsky:
Code review ACK fa176e253f, though if you have to update this again, would suggest separating txindex test cleanup and the chainstatemanager test fix in separate commits, or identifying which part of the change is the bugfix fix in the commit description. Also to clean up the txindex test it might make sense to call SyncWithValidationInterfaceQueue in the test destructor to prevent nondeterminism in other tests
Tree-SHA512: 34c5dca283a7c205cd42b6aa59f00a71fd1bd980bc3d6640a18b280be11470bfabb2fd8c93fadde6fb8e084bcf96c80ec3aa72bbccccfde8a8260d173eaad08f
48973402d8 wallet: Avoid use of Chain::Lock in CWallet::GetKeyBirthTimes (Russell Yanofsky)
e958ff9ab5 wallet: Avoid use of Chain::Lock in CWallet::CreateTransaction (Russell Yanofsky)
c0d07dc4cb wallet: Avoid use of Chain::Lock in CWallet::ScanForWalletTransactions (Russell Yanofsky)
1be8ff280c wallet: Avoid use of Chain::Lock in rescanblockchain (Russell Yanofsky)
3cb85ac594 wallet refactor: Avoid use of Chain::Lock in CWallet::RescanFromTime (Russell Yanofsky)
f7ba881bc6 wallet: Avoid use of Chain::Lock in listsinceblock (Russell Yanofsky)
bc96a9bfc6 wallet: Avoid use of Chain::Lock in importmulti (Russell Yanofsky)
25a9fcf9e5 wallet: Avoid use of Chain::Lock in importwallet and dumpwallet (Russell Yanofsky)
c1694ce6bb wallet: Avoid use of Chain::Lock in importprunedfunds (Russell Yanofsky)
ade5f87971 wallet refactor: Avoid use of Chain::Lock in qt wallettests (Russell Yanofsky)
f6da44ccce wallet: Avoid use of Chain::Lock in tryGetTxStatus and tryGetBalances (Russell Yanofsky)
bf30cd4922 refactor: Add interfaces::FoundBlock class to selectively return block data (Russell Yanofsky)
Pull request description:
This is a set of changes updating wallet code to make fewer calls to `Chain::Lock` methods, so the `Chain::Lock` class will be easier to remove in #16426 with fewer code changes and small changes to behavior.
ACKs for top commit:
MarcoFalke:
re-ACK 48973402d8, only change is fixing bug 📀
fjahr:
re-ACK 48973402d8, reviewed rebase and changes since last review, built and ran tests locally
ariard:
Coce Review ACK 4897340, only changes are one suggested by last review on more accurate variable naming, human-readable output, args comments in `findCommonAncestor`
Tree-SHA512: cfd2f559f976b6faaa032794c40c9659191d5597b013abcb6c7968d36b2abb2b14d4e596f8ed8b9a077e96522365261299a241a939b3111eaf729ba0c3ef519b
c9017ce3bc protect g_chainman with cs_main (James O'Beirne)
2b081c4568 test: add basic tests for ChainstateManager (James O'Beirne)
4ae29f5f0c use ChainstateManager to initialize chainstate (James O'Beirne)
5b690f0aae refactor: move RewindBlockIndex to CChainState (James O'Beirne)
89cdf4d569 validation: introduce unused ChainstateManager (James O'Beirne)
8e2ecfe249 validation: add CChainState.m_from_snapshot_blockhash (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 changeset introduces `ChainstateManager`, which is responsible for creating and managing access to multiple chainstates. Until we allow chainstate creation from UTXO snapshots (next assumeutxo PR?) it's basically unnecessary, but it is a prerequisite for background IBD support.
Changes are also made to the initialization process to make use of `g_chainman` and thus clear the way for multiple chainstates being loaded on startup.
One immediate benefit of this change is that we no longer have the `g_blockman` global, but instead have the ChainstateManager inject a reference of its shared BlockManager into any chainstate it creates.
Another immediate benefit is that uses of `ChainActive()` and `ChainstateActive()` are now covered by lock annotations. Because use of `g_chainman` is annotated to require cs_main, these two functions subsequently follow.
Because of whitespace changes, this diff looks bigger than it is. E.g., 4813167d98 is most easily reviewed with
```sh
git show --color-moved=dimmed_zebra -w 4813167d98
```
ACKs for top commit:
MarcoFalke:
re-ACK c9017ce3bc📙
fjahr:
Code Review Re-ACK c9017ce3bc
ariard:
Code Review ACK c9017ce
ryanofsky:
Code review ACK c9017ce3bc. No changes since last review other than a straight rebase
Tree-SHA512: 3f250d0dc95d4bfd70852ef1e39e081a4a9b71a4453f276e6d474c2ae06ad6ae6a32b4173084fe499e1e9af72dd9007f4a8a375c63ce9ac472ffeaada41ab508
01a3392b1b Drop bitcoin-wallet dependency on libevent (Russell Yanofsky)
0660119ac3 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 01a3392b1b.
Tree-SHA512: d2245e912ab494cccceeb427a1eca8e55b01a0006ff93eebcfb5461ae7cecd1083ac2de443d9db036b18bdc6f0fb615546caaa20c585046f66d234937f74870a
b1d24d1d03 Reorder the test instructions by number (Pieter Wuille)
c2ccadc26a Merge and generalize case 3 and case 6 (Pieter Wuille)
402ad5aaca Only run sanity check once at the end (Pieter Wuille)
eda8309bfc Assert immediately rather than caching failure (Pieter Wuille)
55608455cb Make a fuzzer-based copy of the prevector randomized test (Pieter Wuille)
Pull request description:
The current prevector test effectively randomly generates a number of operations to perform on a prevector and a normal vector, and checks consistency between the two.
By converting this into a fuzzer the operations can be targetted rather than random.
ACKs for top commit:
MarcoFalke:
ACK b1d24d1d03🍬
Tree-SHA512: 2b5c62abcd5fee94f42db03400531484d98c59e7f4308e0e683c61aabcd9ce42f85c5d058d2d5e7f8221124f71d2112b6a5f3c80e5d0fdae265a70647747e92f
cdfb8e7afa tests: Add fuzzing harness for HTTPRequest, libevent's evhttp and related functions (practicalswift)
Pull request description:
Add fuzzing harness for `HTTPRequest`, `libevent`'s `evhttp` and related functions.
ACKs for top commit:
laanwj:
ACK cdfb8e7afa
Tree-SHA512: da481afed5eb3232d3f3d0583094e56050e6234223dfcb356d8567fe0616336eb1b78c5e6821325fc9767e385e5dfaf3c96f0d35ffdb67f18d74f9a9a9464e24
7777e3624f scripted-diff: Replace strCommand with msg_type (MarcoFalke)
Pull request description:
Receiving a message is not a command, but simply a message of some type
ACKs for top commit:
promag:
ACK 7777e3624f.
naumenkogs:
ACK 7777e36
practicalswift:
ACK 7777e3624f -- I've always thought the `strCommand` name is confusing :)
theStack:
ACK 7777e36
Tree-SHA512: 662bac579064c621191916274314b85111cfb4df488f00893ceb16def1c47af4b2a0f34cd7349722099b5a9d23160edb8eb999841f1d64af3e0da02e4870b4bf
283bd72156 tests: Add coverage of {,Incremental}DynamicUsage(const std::set<X, Y>& s) to existing fuzzer (practicalswift)
bf76000493 tests: Add fuzzing harness for classes/functions in cuckoocache.h (practicalswift)
57890b2555 tests: Add fuzzing harness for classes/functions in checkqueue.h (practicalswift)
2df5701e90 tests: Add coverage of GetVirtualTransactionSize(...) to existing fuzzer (practicalswift)
7b9a2dc864 tests: Add fuzzing harness for AdditionOverflow(...) (practicalswift)
44fb2a596b tests: Add fuzzing harness for FeeFilterRounder (practicalswift)
Pull request description:
Includes:
```
tests: Add fuzzing harness for FeeFilterRounder
tests: Add fuzzing harness for classes/functions in checkqueue.h
tests: Add fuzzing harness for classes/functions in cuckoocache.h
tests: Add coverage of {,Incremental}DynamicUsage(const std::set<X, Y>& s) to existing fuzzer
tests: Add coverage of GetVirtualTransactionSize(...) to existing fuzzer
tests: Add fuzzing harness for AdditionOverflow(...)
```
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core.
ACKs for top commit:
MarcoFalke:
ACK 283bd72156
Tree-SHA512: 2361edfb5c47741b22d9fb996836c5250c5a26bc5e956039ea6a0c55ba2d36c78f241d66f85bc02f5b85b9b83d5fde56a5c4702b9d1b7ac4a9a3ae391ca79eaa
Use TestingSetup fixture to fix unregister_all_during_call test not calling
UnregisterBackgroundSignalScheduler, which could trigger an assert in
RegisterBackgroundSignalScheduler when called in later tests
Failure reported by fanquake <fanquake@gmail.com>
https://github.com/bitcoin/bitcoin/pull/18551#issuecomment-610974251
9e071b0089 test: remove rapidcheck integration and tests (fanquake)
Pull request description:
Whilst the property tests are interesting, ultimately [rapidcheck](https://github.com/emil-e/rapidcheck) integration in this repository has not gained much traction. We have a limited number of tests, and they are rarely (if ever) run. Have discussed this with Chris Stewart.
ACKs for top commit:
practicalswift:
ACK 9e071b0089
Tree-SHA512: d0c12af3163382eee8413da420c63e39265a7b700709a05d518445832d45e049aed9508e32524db5228fe3ac114609a00b7bb890be047c07032e44a5ef4611e9
Don't require urlDecode function in wallet code since urlDecode implementation
currently uses libevent. Just call urlDecode indirectly though URL_DECODE
function pointer constant if available.
In bitcoind and bitcoin-qt, URL_DECODE is implemented and used to interpret RPC
wallet requests. In bitcoin-wallet, URL_DECODE is null to avoid depending on
libevent.
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change affects behavior in a few small ways.
- If there's no max_height specified, percentage progress is measured ending at
wallet last processed block instead of node tip
- More consistent error reporting: Early check to see if start_block is on the
active chain is removed, so start_block is always read and the triggers an
error if it's unavailable
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. The rescanblockchain error height error checking
will just be stricter in this case and only accept values up to the last
processed height
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip. Previously listsinceblock might not have returned
all transactions up to the claimed "lastblock" value in this case, resulting in
race conditions and potentially missing transactions in cases where
listsinceblock was called in a loop like
https://github.com/bitcoin/bitcoin/issues/14338#issuecomment-426706574
This is a step toward removing the Chain::Lock class and reducing cs_main
locking.
This change only affects behavior in the case where wallet last block processed
falls behind the chain tip, in which case the "Block not found in chain" error
will be stricter and not allow importing data from a blocks between the wallet
last processed tip and the current node tip.
FoundBlock class allows interfaces::Chain::findBlock to return more block
information without having lots of optional output parameters. FoundBlock class
is also used by other chain methods in upcoming commits.
There is mostly no change in behavior. Only exception is
CWallet::RescanFromTime now throwing NonFatalCheckError instead of
std::logic_error.
11a520f679 tests: Add fuzzing harness for functions/classes in random.h (practicalswift)
64d277bbbc tests: Add fuzzing harness for LimitedString (serialize.h) (practicalswift)
f205cf7fef tests: Add fuzzing harness for functions/classes in span.h (practicalswift)
9718f38f54 tests: Add fuzzing harness for functions/classes in merkleblock.h (practicalswift)
a16ea051f9 tests: Add fuzzing harness for functions/classes in flatfile.h (practicalswift)
Pull request description:
* Add fuzzing harness for functions/classes in `flatfile.h`
* Add fuzzing harness for functions/classes in `merkleblock.h`
* Add fuzzing harness for functions/classes in `span.h`
* Add fuzzing harness for `LimitedString` (`serialize.h`)
* Add fuzzing harness for functions/classes in `random.h`
Top commit has no ACKs.
Tree-SHA512: 6f7e0f946f1062d51216990cde9672b4e896335152548ace3d8711e4969c3e3c8566d01d915b72adcda5c1caa9c2e34da6b7473b55a229f5b77239d3b0ba4b67
faaf1cb5b9 util: Replace i64tostr with ToString (MarcoFalke)
fac96fff62 util: Remove unused itostr (MarcoFalke)
Pull request description:
Currently unused, but if someone really needed to use a helper with this functionality in the future, they could use `ToString`.
ACKs for top commit:
laanwj:
ACK faaf1cb5b9
promag:
Code review ACK faaf1cb5b9.
Tree-SHA512: 42180c03f51d677f7b69da23c7868bdd88944335fad0752fcc307f2c3e3c69f1cc1b316ac0875bcefb9a69c5d55200d7cf66843ea4c0f0f26baf7a054b96c1bb
9ab14e4d21 Limit decimal range of numbers ParseScript accepts (pierrenn)
Pull request description:
Following up on this suggestion : https://github.com/bitcoin/bitcoin/pull/18413#issuecomment-602966490, prevent the output of `atoi64` in the `core_read.cpp:ParseScript` helper to send to `CScriptNum::serialize` values wider than 32-bit.
Since the `ParseScript` helper is only used by the tool defined in `bitcoin-tx.cpp`, this only prevents users to provide too much unrealistic values.
ACKs for top commit:
laanwj:
ACK 9ab14e4d21
Tree-SHA512: ee228269d19d04e8fee0aa7c0ae2bb0a2b437b8e574356e8d9b2279318242057d51fcf39a842aa3afe27408d0f2d5276df245d07a3f4828644a366f80587b666
7834c3b9ec tests: Add fuzzing harness for functions/classes in chain.h (practicalswift)
d7930c4326 tests: Add fuzzing harness for functions/classes in protocol.h (practicalswift)
Pull request description:
Add fuzzing harnesses for functions/classes in `chain.h` and `protocol.h`.
Top commit has no ACKs.
Tree-SHA512: ac2d66bc678ebba0ffbbc42e77806eaf3bb07413ff19219c7a83b171ccd4601e0aa8546ee7ffe8018ca4de12d080f79f693d184cc337c234cde641803279f00c
d056df033a Replace std::to_string with locale-independent alternative (Ben Woosley)
Pull request description:
Addresses #17866 following practicalswift's suggestion:
https://github.com/bitcoin/bitcoin/issues/17866#issuecomment-584287299
~Used ::ToString to avoid aliasing issues. Left uses in QT and test.~
ACKs for top commit:
practicalswift:
ACK d056df033a
laanwj:
ACK d056df033a
Tree-SHA512: 9e6966a9cdd14f4a1a40d9f0fa7c402aed22b2f1ad8681708e22b050d51a91c5d62220a9ec4c425be2d57acf5c964fca87a5e981b5cbff048bc3b6720dae92b7
4308aa67e3 tests: Add fuzzing harness for functions in net_permissions.h (practicalswift)
43ff0d91f8 tests: Add fuzzing harness for functions in timedata.h (practicalswift)
a8695db785 tests: Add fuzzing harness for functions in addrdb.h (practicalswift)
Pull request description:
Add fuzzing harnesses for functions in `addrdb.h`, `net_permissions.h` and `timedata.h`.
Top commit has no ACKs.
Tree-SHA512: ea41431e7f1944ecd0c102e6ea04e70d6763dc9b6e3a0949a4f7299897a92fa3e8e7139f9f65b9508ce8d45613ea24ec0fd6d4a8be3cfd7c23136512b17770eb
5aab011805 test: add unit test for non-standard "scriptsig-not-pushonly" txs (Sebastian Falbesoner)
Pull request description:
Approaches another missing unit test of issue #17394: Checks that the function `IsStandardTx()` returns rejection reason "scriptsig-not-pushonly" if any one of the input's scriptSig consists of any other ops than just PUSHs.
ACKs for top commit:
MarcoFalke:
ACK 5aab011805🍟
practicalswift:
ACK 5aab011805 -- patch looks correct
Tree-SHA512: fbe25bcf57e5f0c8d2397eb67e61fe8d9145ba83032789adb2b67d6fcbcd87e6427e9d965e8cd7bbaaea482e39ec2f110f71ef2de079c7d1fba2712848caa9ba
e57980b473 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f361 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f5 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d75 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b473, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
5e47b19e50 tests: Add harness which fuzzes EvalScript and VerifyScript using a fuzzed signature checker (practicalswift)
Pull request description:
Add harness which fuzzes `EvalScript` and `VerifyScript` using a fuzzed signature checker.
Test this PR using:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/signature_checker
…
```
Closes#17986.
Top commit has no ACKs.
Tree-SHA512: a9988f8fa7919fe470756ca3e4e75764a589f590769aab452c8f4c254cf41667793e52131d470a12629ec3681fa7fc20091f371b8f3e3eec105674c2769e7d7e
fa36f3a295 refactor: move DUMP_BANS_INTERVAL to banman.h (MarcoFalke)
fadafb83cf scheduler: Make schedule* methods type safe (MarcoFalke)
fa70ccc6c4 scheduler: Use C++11 member initialization, add shutdown assert (MarcoFalke)
Pull request description:
Main benefit is that stuff like `15 * 60 * 1000` is replaced by `minutes{15}`
ACKs for top commit:
vasild:
ACK fa36f3a (code review, not tested)
ajtowns:
ACK fa36f3a295
jonatack:
ACK fa36f3a
Tree-SHA512: f35f1a1d643dfa676bd47474659f6492ed05cca04cdb556064b126f654a6a44a4b93fcaddcdcd41faf81b8f11439c11e5c7ab88685ba2eef12f7188843d17ad8
fa7fea3654 refactor: Remove mempool global from net (MarcoFalke)
Pull request description:
To increase modularisation and simplify testing, remove the mempool global from net in favour of a mempool member.
This is done in the same way it was done for the connection manager global.
ACKs for top commit:
jnewbery:
code review ACK fa7fea3654
Tree-SHA512: 0e3e1eefa8d6e46367bc6991d5f36c636b15ae4a3bda99b6fe6715db3240771c3d87943c6eb257d69f31929fa2f1d0973e14fc9d1353a27551dbe746eae36857
09e25071f4 Cache parent xpub inside of BIP32PubkeyProvider (Andrew Chow)
deb791c7ba Only cache xpubs that have a hardened last step (Andrew Chow)
f76733eda5 Cache the immediate derivation parent xpub (Andrew Chow)
58f54b686f Add DescriptorCache* read_cache and DescriptorCache* write_cache to Expand and GetPubKey (Andrew Chow)
66c2cadc91 Rename BIP32PubkeyProvider.m_extkey to m_root_extkey (Andrew Chow)
df55d44d0d Track the index of the key expression in PubkeyProvider (Andrew Chow)
474ea3b927 Introduce DescriptorCache struct which caches xpubs (Andrew Chow)
Pull request description:
Improves the descriptor cache by changing it from a `std::vector<unsigned char>` to a newly introduced `DescriptorCache` class. Instead of serializing pubkeys and whatever else we would want to cache in a way that may not be backwards compatible, we instead create a `DescriptorCache` object and populate it. This object contains only an xpub cache. Since the only `PubkeyProvider` that used the cache is the `BIP32PubkeyProvider` we just have it store the xpubs instead of the pubkeys. This allows us to have both the parent xpub and the child xpubs in the same container. The map is keyed by `KeyOriginInfo`.
Sine we are caching `CExtPubKey`s in `DescriptorCache`, `BIP32PubKeyProviders` can use the cached parent xpubs to derive the children if unhardened derivation is used in the last step. This also means that we can still derive the keys for a `BIP32PubkeyProvider` that has hardened derivation steps. When combined with descriptor wallets, this should allow us to be able to import a descriptor with an `xprv` and hardened steps and still be able to derive from it. In that sense, this is an alternative to #18163
To test that this works, the tests have been updated to do an additional `Expand` at the `i + 1` position. This expansion is not cached. We then do an `ExpandFromCache` at `i + 1` and use the cache that was produced by the expansion at `i`. This way, we won't have the child xpubs for `i + 1` but we will have the parent xpubs. So this checks whether the parent xpubs are being stored and can be used to derive the child keys. Descriptors that have a hardened last step are skipped for this part of the test because that will always require private keys.
ACKs for top commit:
instagibbs:
code review re-re-ACK 09e25071f4
Sjors:
re-ACK 09e25071f4
Tree-SHA512: 95c8d0092274cdf115ce39f6d49dec767679abf3758d5b9e418afc308deca9dc6f67167980195bcc036cd9c09890bbbb39ec1dacffbfacdc03efd72a7e23b276
fab7d14ea5 test: Check that wait_until returns if time point is in the past (MarcoFalke)
Pull request description:
Add an explicit regression test for the condvar bug (#18227), so that this doesn't happen again
ACKs for top commit:
laanwj:
ACK fab7d14ea5
Tree-SHA512: 6ec0d0b3945cae87a001e367af34cca1953a8082b4a0d9f8a20d30acd1f36363e98035d4eb173ff786cf6692d352d41f960633415c46394af042eb44e3b5ad71
9220a0fdd0 tests: Add one specialized ProcessMessage(...) fuzzing binary per message type for optimal results when using coverage-guided fuzzing (practicalswift)
fd1dae10b4 tests: Add fuzzing harness for ProcessMessage(...) (practicalswift)
Pull request description:
Add fuzzing harness for `ProcessMessage(...)`. Enables high-level fuzzing of the P2P layer.
All code paths reachable from this fuzzer can be assumed to be reachable for an untrusted peer.
Seeded from thin air (an empty corpus) this fuzzer reaches roughly 20 000 lines of code.
To test this PR:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/process_message
…
```
Worth noting about this fuzzing harness:
* To achieve a reasonable number of executions per seconds the state of the fuzzer is unfortunately not entirely reset between `test_one_input` calls. The set-up (`FuzzingSetup` ctor) and tear-down (`~FuzzingSetup`) work is simply too costly to be run on every iteration. There is a trade-off to handle here between a.) achieving high executions/second and b.) giving the fuzzer a totally blank slate for each call. Please let me know if you have any suggestion on how to improve this situation while maintaining >1000 executions/second.
* To achieve optimal results when using coverage-guided fuzzing I've chosen to create one specialised fuzzing binary per message type (`process_message_addr`, `process_message_block`, `process_message_blocktxn `, etc.) and one general fuzzing binary (`process_message`) which handles all messages types. The latter general fuzzer can be seeded with inputs generated by the former specialised fuzzers.
Happy fuzzing friends!
ACKs for top commit:
MarcoFalke:
ACK 9220a0fdd0🏊
Tree-SHA512: c314ef12b0db17b53cbf3abfb9ecc10ce420fb45b17c1db0b34cabe7c30e453947b3ae462020b0c9f30e2c67a7ef1df68826238687dc2479cd816f0addb530e5
6590395f60 tests: Remove FUZZERS_MISSING_CORPORA (practicalswift)
815c7a6793 tests: Add basic fuzzing harness for CNetAddr/CService/CSubNet related functions (netaddress.h) (practicalswift)
Pull request description:
Add basic fuzzing harness for `CNetAddr`/`CService`/`CSubNet` related functions (`netaddress.h`).
To test this PR:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz \
--with-sanitizers=address,fuzzer,undefined
$ make
$ src/test/fuzz/netaddress
…
```
Top commit has no ACKs.
Tree-SHA512: 69dc0e391d56d5e9cdb818ac0ac4b69445d0195f714442a06cf662998e38b6e0bbaa635dce78df37ba797feed633e94abba4764b946c1716d392756e7809112d
Also adds tests for this:
For ranged descriptors with unhardened derivation, we expect to
find parent keys in the cache but no child keys.
For descriptors containing an xpub but do not have unhardened derivation
(i.e. hardened derivation or single xpub with or without derivation),
we expect to find all of the keys in the cache, and the same
number of keys in the cache as in the SigningProvider.
For everything else (no xpub), nothing should be cached at all.
Have Expand, ExpandFromCache, and ExpandHelper take additional DescriptorCache
parameters. These are then passed into PubkeyProvider::GetPubKey which
also takes them as arguments.
Reading and writing to the cache is pushed down into GetPubKey. The old cache where
pubkeys are serialized to a vector is completely removed and instead xpubs are being
cached in DescriptorCache.
70a6b529f3 lint-cppcheck: Remove -DHAVE_WORKING_BOOST_SLEEP_FOR (Anthony Towns)
294937b39d scheduler_tests: re-enable mockforward test (Anthony Towns)
cea19f6859 Drop unused reverselock.h (Anthony Towns)
d0ebd93270 scheduler: switch from boost to std (Anthony Towns)
b9c4260127 sync.h: add REVERSE_LOCK (Anthony Towns)
306f71b4eb scheduler: don't rely on boost interrupt on shutdown (Anthony Towns)
Pull request description:
Replacing boost functionality with C++11 stuff.
Motivated by #18227, but should stand alone. Changing from `boost::condition_var` to `std::condition_var` means `threadGroup.interrupt_all` isn't enough to interrupt `serviceQueue` anymore, so that means calling `stop()` before `join_all()` is needed. And the existing reverselock.h code doesn't work with sync.h's DebugLock code (because the reversed lock won't be removed from `g_lockstack` which then leads to incorrect potential deadlock warnings), so I've replaced that with a dedicated class and macro that's aware of our debug lock behaviour.
Fixes#16027, Fixes#14200, Fixes#18227
ACKs for top commit:
laanwj:
ACK 70a6b529f3
Tree-SHA512: d1da13adeabcf9186d114e2dad9a4fdbe2e440f7afbccde0c13dfbaf464efcd850b69d3371c5bf8b179d7ceb9d81f4af3cc22960b90834e41eaaf6d52ef7d331
Changes from boost::chrono to std::chrono, boost::condition_var to
std::condition_var, boost::mutex to sync.h Mutex, and reverselock.h to
sync.h REVERSE_LOCK. Also adds threadsafety annotations to CScheduler
members.
