e528075189 tests: Add fuzzing harness for Lookup(...)/LookupHost(...)/LookupNumeric(...)/LookupSubNet(...) (practicalswift)
c6b4bfb4b3 net: Make DNS lookup code testable (practicalswift)
Pull request description:
Make DNS lookup mockable/testable/fuzzable.
Add fuzzing harness for `Lookup(…)`/`LookupHost(…)`/`LookupNumeric(…)`/`LookupSubNet(…)`.
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:
cr ACK e528075189
vasild:
ACK e528075189
Tree-SHA512: 9984c2e2fedc3c1e1c3dbd701bb739ebd2f01766e6e83543dae5ae43eb8646c452bba0e101dd2f06079e5258bd5846c7d27a60ed5d77c1682b54c9544ffad443
9048c58e10 Remove pointer cast in CRPCTable::dumpArgMap (Russell Yanofsky)
14f3d9b908 refactor: Add RPC server ExecuteCommands function (Russell Yanofsky)
6158a6d397 refactor: Replace JSONRPCRequest fHelp field with mode field (Russell Yanofsky)
Pull request description:
This change is needed to fix the `rpc_help.py` test failing in #10102: https://cirrus-ci.com/task/5469433013469184?command=ci#L2275
The [`CRPCTable::dumpArgMap`](16b784d953/src/rpc/server.cpp (L492)) method currently works by casting RPC `unique_id` integer field to a function pointer, and then calling it. The `unique_id` field wasn't supposed to be used this way (it's meant to be used to detect RPC aliases) and as a result, this code segfaults in the `rpc_help.py` test in multiprocess PR #10102 because wallet RPC functions aren't directly accessible from the node process.
Fix this by adding a new `GET_ARGS` RPC request mode to retrieve argument information similar to the way the `GET_HELP` mode retrieves help information.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
MarcoFalke:
re-ACK 9048c58e10👑
Tree-SHA512: cd1a01c1daa5bde2c2455b63548371ee4cf39688313969ad2016d9a0fd4344102e3fd43034058f253364518e9632d57cf21abffad0d6a2c0c94b7a6921cbe615
1a6323bdbe doc: update developer notes for removal of MakeUnique (fanquake)
3ba2840e7e scripted-diff: remove MakeUnique<T>() (fanquake)
Pull request description:
Since requiring C++17, this is just pointless abstraction. I think we should just "tear the band-aid off" and remove it. Similar to the changes happening in #21366.
Also, having a comment saying this is deprecated doesn't prevent it's usage in new code. i.e : https://github.com/bitcoin/bitcoin/pull/20946#discussion_r561949731.
The repository is fairly quiet at the moment, so any potential complaints about having to rebase should be minimal. Might as well get this over and done with.
ACKs for top commit:
jnewbery:
utACK 1a6323bdbe
practicalswift:
cr ACK 1a6323bdbe: patch looks correct
ajtowns:
ACK 1a6323bdbe -- code review only
glozow:
ACK 1a6323bdbe looks correct
Tree-SHA512: 4a14b9611b60b9b3026b54d6f5a2dce4c5d9b63a7b93d7de1307512df736503ed84bac66e7b93372c76e3117f49bf9f29cd473d3a47cb41fb2775bc10234736f
a67983cd6d net_processing: Add review-only assertion to PeerManager (Carl Dong)
272d993e75 scripted-diff: net_processing: Use existing chainman (Carl Dong)
021a04a469 net_processing: Move some static functions to PeerManager (Carl Dong)
91c5b68acd node/ifaces: ChainImpl: Use existing NodeContext member (Carl Dong)
8a1d580b21 node/ifaces: NodeImpl: Use existing NodeContext member (Carl Dong)
4cde4a701b node: Use existing NodeContext (Carl Dong)
106bcd4f39 node/coinstats: Pass in BlockManager to GetUTXOStats (Carl Dong)
2c3ba00693 miner: Pass in blockman to ::RegenerateCommitments (Carl Dong)
2afcf24408 miner: Remove old CreateNewBlock w/o chainstate param (Carl Dong)
46b7f29340 scripted-diff: Invoke CreateNewBlock with chainstate (Carl Dong)
d0de61b764 miner: Pass in chainstate to BlockAssembler::CreateNewBlock (Carl Dong)
a04aac493f validation: Remove extraneous LoadGenesisBlock function prototype (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Based on:
- [x] #21055 | [Bundle 3/n] Prune g_chainman usage in mempool-related validation functions
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
laanwj:
Code review ACK a67983cd6d
ryanofsky:
Code review ACK a67983cd6d. Only change since last review new first commit fixing header declaration, and rebase
glozow:
code review ACK a67983cd6d
Tree-SHA512: dce182a18b88be80cbf50978d4ba8fa6ab0f01e861d09bae0ae9364051bb78f9334859d164b185b07f1d70a583e739557fab6d820cac8c37b3855b85c2a6771b
In glib 2.13 memcpy was changed such that the way it copied bytes was reversed.
This caused all sorts of issues for existing software, which depended on the
existing behavior (when they should have been using memmove). See:
https://sourceware.org/bugzilla/show_bug.cgi?id=12518https://bugzilla.redhat.com/show_bug.cgi?id=638477
Now that we require glibc 2.17+ (#17538), we should be well clear of having to
maintain our memcpy -> memmove aliasing, which was introduced in #4339.
0c471a5f30 tests: check never active versionbits (Anthony Towns)
3ba9283a47 tests: more helpful errors for failing versionbits tests (Anthony Towns)
Pull request description:
Extracted from #19573 to make review easier. I also reviewed it myself.
I added some comments to the test: bae9a45219 (r585486781)
I also moved some `TestState` changes from the second to the first commit, to reduce the latter diff.
ACKs for top commit:
ajtowns:
ACK 0c471a5f30
MarcoFalke:
review ACK 0c471a5f30🔓
Tree-SHA512: 61f8d1ecaf38a6cd13db1cf71c89b8c4d2f5852ef77c5e7ecb9bd78eb216545037411641bb101cf0740c5c47845ac327954ee25b676d63779c5f148719ac5caf
e11b649650 validation: CVerifyDB::VerifyDB: Use locking annotation (Carl Dong)
03f75c42e1 validation: Use existing chain member in CChainState::LoadGenesisBlock (Carl Dong)
5e4af77380 validation: Use existing chain member in CChainState::AcceptBlock (Carl Dong)
fee73347c0 validation: Pass in chain to FindBlockPos+SaveBlockToDisk (Carl Dong)
a9d28bcd8d validation: Use *this in CChainState::ActivateBestChainStep (Carl Dong)
4744efc9ba validation: Pass in chainstate to CTxMemPool::check (Carl Dong)
1fb7b2c595 validation: Use *this in CChainState::InvalidateBlock (Carl Dong)
8cdb2f7e58 validation: Move LoadBlockIndexDB to CChainState (Carl Dong)
8b99efbcc0 validation: Move invalid block handling to CChainState (Carl Dong)
2bdf37fe18 validation: Pass in chainstate to CVerifyDB::VerifyDB (Carl Dong)
31eac50c72 validation: Remove global ::VersionBitsTip{State,SinceHeight,Statistics} (Carl Dong)
63e4c7316a validation: Pass in chainstate to ::PruneBlockFilesManual (Carl Dong)
4bada76237 validation: Pass in chainstate to UpdateTip (Carl Dong)
a3ba08ba7d validation: Remove global ::{{Precious,Invalidate}Block,ResetBlockFailureFlags} (Carl Dong)
4927c9e699 validation: Remove global ::LoadGenesisBlock (Carl Dong)
9da106be4d validation: Check chain tip is non-null in CheckFinalTx (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Based on:
- [x] #20750 | [Bundle 2/n] Prune g_chainman usage in mempool-related validation functions
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
Note to self:
- [x] Address: https://github.com/bitcoin/bitcoin/pull/20750#discussion_r579400663
ACKs for top commit:
laanwj:
Code review ACK e11b649650
Tree-SHA512: 205a451a741e32f17d5966de289f2f5a3f0817738c0087b70ff4755ddd217b53d01050ed396669bda2b1d216a88d927b9778777f9ff95ab1fe20e59c5f341776
fa59ad5130 fuzz: Add missing include (test/util/setup_common.h) (MarcoFalke)
Pull request description:
`src/test/fuzz/socks5.cpp` is using the symbol `BasicTestingSetup`, which is defined in `src/test/util/setup_common.h`.
Currently compilation happens to succeed because the needed dependency is indirectly included. Compilation will break as soon as the indirect dependency is broken. According to the dev notes, everything that is used must be included.
Fix the issue by including the missing include.
ACKs for top commit:
fanquake:
ACK fa59ad5130
Tree-SHA512: 9359d5d288ebc5a53d753ebed1ee8d49ddcfe12aeb56054ea43654c0d915337bb0dce7c8a7178e94711ff8dacd1b3ea0a2871b21b1709cd9786efc0c1ef532b3
0eaea66e8b Make tx relay data structure use std::chrono types (Pieter Wuille)
55e82881a1 Make all Poisson delays use std::chrono types (Pieter Wuille)
c733ac4d8a Convert block/header sync timeouts to std::chrono types (Pieter Wuille)
4d98b401fb Change all ping times to std::chrono types (Pieter Wuille)
Pull request description:
(Picking up #20044. Rebased against master.)
This changes various uses of integers to represent timestamps and durations to `std::chrono` duration types with type-safe conversions, getting rid of various `.count()`, constructors, and conversion factors.
ACKs for top commit:
jnewbery:
utACK 0eaea66e8b
vasild:
ACK 0eaea66e8b
MarcoFalke:
re-ACK 0eaea66e8b, only changes: minor rename, using C++11 member initializer, using 2min chrono literal, rebase 🤚
ajtowns:
utACK 0eaea66e8b
Tree-SHA512: 2dbd8d53bf82e98f9b4611e61dc14c448e8957d1a02575b837fadfd59f80e98614d0ccf890fc351f960ade76a6fb8051b282e252e81675a8ee753dba8b1d7f57
1f05dbd06d util: Avoid invalid integer negation in ValueFromAmount: make ValueFromAmount(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min() (practicalswift)
7cc75c9ba3 util: Avoid invalid integer negation in FormatMoney: make FormatMoney(const CAmount& n) well-defined also when n is std::numeric_limits<CAmount>::min() (practicalswift)
Pull request description:
Avoid invalid integer negation in `FormatMoney` and `ValueFromAmount`.
Fixes #20402.
Before this patch:
```
$ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
$ make -C src/ test/test_bitcoin
$ src/test/test_bitcoin -t rpc_tests/rpc_format_monetary_values -t util_tests/util_FormatMoney
core_write.cpp:21:29: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount'
(aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior core_write.cpp:21:29 in
test/rpc_tests.cpp(186): error: in "rpc_tests/rpc_format_monetary_values":
check ValueFromAmount(std::numeric_limits<CAmount>::min()).write() == "-92233720368.54775808" has failed
[--92233720368.-54775808 != -92233720368.54775808]
util/moneystr.cpp:16:34: runtime error: negation of -9223372036854775808 cannot be represented in type 'CAmount'
(aka 'long'); cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior util/moneystr.cpp:16:34 in
test/util_tests.cpp(1188): error: in "util_tests/util_FormatMoney":
check FormatMoney(std::numeric_limits<CAmount>::min()) == "-92233720368.54775808" has failed
[--92233720368.-54775808 != -92233720368.54775808]
```
After this patch:
```
$ CC=clang CXX=clang++ ./configure --with-sanitizers=undefined
$ make -C src/ test/test_bitcoin
$ src/test/test_bitcoin -t rpc_tests/rpc_format_monetary_values -t util_tests/util_FormatMoney
```
ACKs for top commit:
laanwj:
re-ACK 1f05dbd06d
Tree-SHA512: 5aaeb8e2178f1597921f53c12bdfc2f3d5993d10c41658dcd25943e54e8cc2116a411bc71d928f890b33bc0b3761a8ee4449b0532bce41125b6c60692808c8c3
366e3e1f89 fuzz: Add FUZZED_SOCKET_FAKE_LATENCY mode to FuzzedSock to allow for fuzzing timeout logic (practicalswift)
b22d4c1607 fuzz: Add fuzzing harness for Socks5(...) (practicalswift)
Pull request description:
Add [regression fuzz harness](https://twitter.com/kayseesee/status/1205287895923212289) for CVE-2017-18350. This fuzzing harness would have found CVE-2017-18350 within a minute of fuzzing :)
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:
ACK 366e3e1f89
Tree-SHA512: 5d8e1863b635efd10ccb11678b71472ba1523c3ef16affa7f9cd638635c1a9c307e28f432d5b87eb0c9cd1c3c1aeafbb24fa7ae86fe4e5090fda2e20d542b6ca
Recognize also I2P addresses in the form `base32hashofpublickey.b32.i2p`
from `CNetAddr::SetSpecial()`.
This makes `Lookup()` support them, which in turn makes it possible to
manually connect to an I2P node by using
`-proxy=i2p_socks5_proxy:port -addnode=i2p_address.b32.i2p:port`
Co-authored-by: Lucas Ontivero <lucasontivero@gmail.com>
Collects all the orphan handling globals into a single member var in
net_processing, and ensures access is encapuslated into the interface
functions. Also adds doxygen comments for methods.
