590bda79e8 scripted-diff: Remove setup_clean_chain if default is not changed (Fabian Jahr)
98892f39e3 doc: Improve setup_clean_chain documentation (Fabian Jahr)
Pull request description:
The first commit improves documentation on setup_clean_chain which is misunderstood quite frequently. Most importantly it fixes the TestShell docs which are simply incorrect.
The second commit removes the instances of `setup_clean_clain` in functional tests where it is not changing the default.
This used to be part of #19168 which also sought to rename`setup_clean_chain`.
ACKs for top commit:
jonatack:
ACK 590bda79e8
Tree-SHA512: a7881186e65d31160b8f84107fb185973b37c6e50f190a85c6e2906a13a7472bb4efa9440bd37fe0a9ac5cd2d1e8559870a7e4380632d9a249eca8980b945f3e
fa61b9d1a6 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke)
7777105a24 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke)
fa06bce4ac test: Add tests (MarcoFalke)
fac05ccdad wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke)
Pull request description:
This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937Fixes: #20902
ACKs for top commit:
ajtowns:
ACK fa61b9d1a6
fjahr:
Code review ACK fa61b9d1a6
Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
bff7c66e67 Add documentation to contrib folder (Troy Giorshev)
381f77be85 Add Message Capture Test (Troy Giorshev)
e4f378a505 Add capture parser (Troy Giorshev)
4d1a582549 Call CaptureMessage at appropriate locations (Troy Giorshev)
f2a77ff97b Add CaptureMessage (Troy Giorshev)
dbf779d5de Clean PushMessage and ProcessMessages (Troy Giorshev)
Pull request description:
This PR introduces per-peer message capture into Bitcoin Core. 📓
## Purpose
The purpose and scope of this feature is intentionally limited. It answers a question anyone new to Bitcoin's P2P protocol has had: "Can I see what messages my node is sending and receiving?".
## Functionality
When a new debug-only command line argument `capturemessages` is set, any message that the node receives or sends is captured. The capture occurs in the MessageHandler thread. When receiving a message, it is captured as soon as the MessageHandler thread takes the message off of the vProcessMsg queue. When sending, the message is captured just before the message is pushed onto the vSendMsg queue.
The message capture is as minimal as possible to reduce the performance impact on the node. Messages are captured to a new `message_capture` folder in the datadir. Each node has their own subfolder named with their IP address and port. Inside, received and sent messages are captured into two binary files, msgs_recv.dat and msgs_sent.dat, like so:
```
message_capture/203.0.113.7:56072/msgs_recv.dat
message_capture/203.0.113.7:56072/msgs_sent.dat
```
Because the messages are raw binary dumps, included in this PR is a Python parsing tool to convert the binary files into human-readable JSON. This script has been placed on its own and out of the way in the new `contrib/message-capture` folder. Its usage is simple and easily discovered by the autogenerated `-h` option.
## Future Maintenance
I sympathize greatly with anyone who says "the best code is no code".
The future maintenance of this feature will be minimal. The logic to deserialize the payload of the p2p messages exists in our testing framework. As long as our testing framework works, so will this tool.
Additionally, I hope that the simplicity of this tool will mean that it gets used frequently, so that problems will be discovered and solved when they are small.
## FAQ
"Why not just use Wireshark"
Yes, Wireshark has the ability to filter and decode Bitcoin messages. However, the purpose of the message capture added in this PR is to assist with debugging, primarily for new developers looking to improve their knowledge of the Bitcoin Protocol. This drives the design in a different direction than Wireshark, in two different ways. First, this tool must be convenient and simple to use. Using an external tool, like Wireshark, requires setup and interpretation of the results. To a new user who doesn't necessarily know what to expect, this is unnecessary difficulty. This tool, on the other hand, "just works". Turn on the command line flag, run your node, run the script, read the JSON. Second, because this tool is being used for debugging, we want it to be as close to the true behavior of the node as possible. A lot can happen in the SocketHandler thread that would be missed by Wireshark.
Additionally, if we are to use Wireshark, we are at the mercy of whoever it maintaining the protocol in Wireshark, both as to it being accurate and recent. As can be seen by the **many** previous attempts to include Bitcoin in Wireshark (google "bitcoin dissector") this is easier said than done.
Lastly, I truly believe that this tool will be used significantly more by being included in the codebase. It's just that much more discoverable.
ACKs for top commit:
MarcoFalke:
re-ACK bff7c66e67 only some minor changes: 👚
jnewbery:
utACK bff7c66e67
theStack:
re-ACK bff7c66e67
Tree-SHA512: e59e3160422269221f70f98720b47842775781c247c064071d546c24fa7a35a0e5534e8baa4b4591a750d7eb16de6b4ecf54cbee6d193b261f4f104e28c15f47
dc8be12510 refactor: remove boost::thread_group usage (fanquake)
Pull request description:
Post #18710, there isn't much left using `boost::thread_group`, so should just be able to replace it with the standard library. This also removes the last use of `boost::thread_interrupted`.
After this change, last piece of Boost Thread we'd be using is `boost::shared_mutex`. See the commentary [here](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) as to why it may be non-trivial to swap that for `std::shared_mutex` in the near future.
Closes#17307
ACKs for top commit:
laanwj:
Code review re-ACK dc8be12510
MarcoFalke:
review ACK dc8be12510🔁
jonatack:
Non-expert code review ACK dc8be12510, also checked range-diff since last review and that local debug build is clean with gcc 10.2.1-6 on Debian
Tree-SHA512: 5510e2d760cce824234207dc86b1551ca8f21cbf3a2ce753c0254a0d03ffd83c94e449aec202fb7bd76e6fc64df783a6b70a736b0add9ece3734bb9c8ce8fc2f
5353b0c64d Change type definitions for "chain" and "setup_clean_chain" from type comments to Python 3.6+ types. Additionally, set type for "nodes". (Kiminuo)
Pull request description:
### Motivation
When I wanted to understand better https://github.com/bitcoin/bitcoin/pull/19145/files#diff-4bebbd3b112dc222ea7e75ef051838ceffcee63b9e9234a98a4cc7251d34451b test, I noticed that navigation in PyCharm/VS Code did not work for `nodes` variable. I think this is frustrating, especially for newcomers.
### Summary
* This PR modifies Python 3.5 [type comments](https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html#variables) to Python 3.6+ types and adds a proper type for `nodes` [instance attribute](https://mypy.readthedocs.io/en/stable/class_basics.html#instance-and-class-attributes).
* This PR does not change behavior.
* This PR is intentionally very small, if the concept is accepted, a follow-up PRs can be more ambitious.
### End result
1. Open `test/functional/feature_abortnode.py`
2. Move your caret to: `self.nodes[0].generate[caret here](3)`
3. Use "Go to definition" [F12] should work now.
I have tested this on PyCharm (Windows, Ubuntu) and VS Code (Windows, Ubuntu).
Note: Some `TestNode` methods (e.g. `self.nodes[0].getblock(...)` ) use `__call__` mechanism and navigation does not work for them even with this PR.
ACKs for top commit:
laanwj:
ACK 5353b0c64d
theStack:
ACK 5353b0c64d
Tree-SHA512: 821773f052ab9b2889dc357d38c59407a4af09e3b86d7134fcca7d78e5edf3a5ede9bfb37595ea97caf9ebfcbda372bcf73763b7f89b0677670f21b3e396a12b
48c8a9b964 net_processing: log txrelay flag from version message (Anthony Towns)
98fab37ca0 net: use peer=N instead of from=N in debug log (Anthony Towns)
12302105bb net_processing: additional debug logging for ignored messages (Anthony Towns)
f7edea3b7c net: make debug logging conditional on -debug=net (Anthony Towns)
a410ae8cb0 net, net_processing: log disconnect reasons with -debug=net (Anthony Towns)
Pull request description:
A few changes to -debug=net logging:
* always log when disconnecting a peer
* only log various connection errors when -debug=net is enabled, since errors from random untrusted peers is completely expected
* log when ignoring a message due to violating protocol (primarily to make it easier to debug other implementations)
* use "peer=123" rather than "from 123" to make grepping logs a bit easier
* log the value of the bip-37 `fRelay` field in version messages both when sending and receiving a version message
ACKs for top commit:
jnewbery:
ACK 48c8a9b964
MarcoFalke:
re-ACK 48c8a9b964 only change is rebase 🚓
practicalswift:
re-ACK 48c8a9b964
Tree-SHA512: 6ac530d883dffc4fd7fe20b1dc5ebb5394374c9b499aa7a253eb4a3a660d8901edd72e5ad21ce4a2bf71df25e8f142087755f9756f3497f564ef453a7e9246c1
faff3991a9 ci: Fuzz with integer sanitizer (MarcoFalke)
Pull request description:
Otherwise the suppressions file will go out of sync
ACKs for top commit:
practicalswift:
cr ACK faff3991a9: patch looks correct
Tree-SHA512: 349216d071a2c5ccf24565fe0c52d7a570ec148d515d085616a284f1ab9992ce10ff82eb17962dddbcda765bbd3a9b15e8b25f34bdbed99fc36922d4161d307c
647b81b709 wallet, rpc: add listdescriptors command (Ivan Metlushko)
Pull request description:
Looking for concept ACKs
**Rationale**: allow users to inspect the contents of their newly created descriptor wallets.
Currently the command only returns xpubs which is not very useful in itself, but there are multiples ways to extend it:
* add an option to export xprv
* with #19136 it'll be possible to return normalised descriptors suitable for a watch-only purposes
The output is compatible with `importdescriptors` command so it could be easily used for backup/recover purposes.
**Output example:**
```json
[
{
"desc": "wpkh(tpubD6NzVbkrYhZ4WW6E2ZETFyNfq2hfF23SKxqSGFvUpPAY58jmmuBybwqwFihAyQPk9KnwTt5516NDZRJ7k5QPeKjy7wuVd5WvXNxwwAs5tUD/*)#nhavpr5h",
"timestamp": 1296688602,
"active": false,
"range": [
0,
999
],
"next": 0
}
]
```
ACKs for top commit:
jonatack:
re-ACK 647b81b709 rebased to master, debug builds cleanly, reviewed diff since last review, tested with a descriptor wallet (and with a legacy wallet)
achow101:
re-ACK 647b81b
Tree-SHA512: 51a3620bb17c836c52cecb066d4fa9d5ff418af56809046eaee0528c4dc240a4e90fff5711ba96e399c6664e00b9ee8194e33852b1b9e75af18061296e19a8a7
Determines whether descriptors should be used based on whether the
--descriptor or --legacy-wallet option is set,
and the compiled support. If no option is set and both BDB and SQLite
are available, it defaults to legacy.
This is used to switch descriptor agnostic tests between descriptors and
legacy wallet.
a6739cc868 rpc: Add specific error code for "wallet already loaded" (Wladimir J. van der Laan)
Pull request description:
Add a separate RPC error code for "wallet already loaded" to avoid having to match on message to detect this.
Requested by shesek for rust-bitcoinrpc.
If concept ACKed needs:
- [ ] Release note
- [x] A functional test (updated the existing test to make it pass, I think this is enough)
ACKs for top commit:
jonasschnelli:
Code Review ACK a6739cc868
promag:
Code review ACK a6739cc868.
Tree-SHA512: 9091872e6ea148aec733705d6af330f72a02f23b936b892ac28f9023da7430af6332418048adbee6014305b812316391812039e9180f7f3362d11f206c13b7d0
f0f8b1a076 fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer (practicalswift)
58232e3ffb fuzz: Avoid -fsanitize=integer warnings in fuzzing harnesses (practicalswift)
Pull request description:
Add UBSan suppressions needed for fuzz tests to not warn under `-fsanitize=integer`.
Avoid `-fsanitize=integer` warnings in fuzzing harnesses.
Suppressed warnings (excluding warnings from `src/crypto/` and `src/test/`):
```
addrman.cpp:306:24: runtime error: implicit conversion from type 'long' of value 5190149478 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 895182182 (32-bit, unsigned)
addrman.h:446:43: runtime error: implicit conversion from type 'int' of value -22 (32-bit, signed) to type 'const uint8_t' (aka 'const unsigned char') changed the value to 234 (8-bit, unsigned)
arith_uint256.cpp:32:35: runtime error: left shift of 1712128 by 24 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
arith_uint256.cpp:47:39: runtime error: left shift of 4294966784 by 31 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
chain.cpp:151:12: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551615 (64-bit, unsigned)
coins.cpp:114:22: runtime error: unsigned integer overflow: 0 - 96 cannot be represented in type 'unsigned long'
compressor.cpp:162:33: runtime error: unsigned integer overflow: 15617702637291228364 * 10 cannot be represented in type 'unsigned long'
compressor.cpp:188:11: runtime error: unsigned integer overflow: 2265760372865400000 * 10 cannot be represented in type 'unsigned long'
hash.cpp:13:15: runtime error: left shift of 1692305888 by 15 places cannot be represented in type 'uint32_t' (aka 'unsigned int')
pubkey.h:152:23: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned int'
streams.h:570:31: runtime error: left shift of 350879 by 52 places cannot be represented in type 'uint64_t' (aka 'unsigned long')
util/bip32.cpp:57:36: runtime error: left shift of 3241096244 by 1 places cannot be represented in type 'unsigned int'
util/strencodings.cpp:562:38: runtime error: implicit conversion from type 'unsigned char' of value 255 (8-bit, unsigned) to type 'char' changed the value to -1 (8-bit, signed)
util/strencodings.h:164:24: runtime error: implicit conversion from type 'int' of value -74 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551542 (64-bit, unsigned)
```
The warnings above happen here:
32b191fb66/src/addrman.cpp (L306)32b191fb66/src/addrman.h (L446)32b191fb66/src/arith_uint256.cpp (L32)32b191fb66/src/arith_uint256.cpp (L47)32b191fb66/src/chain.cpp (L151)32b191fb66/src/coins.cpp (L114)32b191fb66/src/compressor.cpp (L162)32b191fb66/src/compressor.cpp (L188)32b191fb66/src/hash.cpp (L13)32b191fb66/src/pubkey.h (L152)32b191fb66/src/streams.h (L570)32b191fb66/src/util/bip32.cpp (L57)32b191fb66/src/util/strencodings.cpp (L562)32b191fb66/src/util/strencodings.h (L164)
ACKs for top commit:
MarcoFalke:
review ACK f0f8b1a076🤚
Tree-SHA512: a8f04f7cc055d03653161de1d9d14d106a6280cea1e86a1243abcd57cf8e61dcf5f731d0ab0da5b390790e816022ff7a70759a641463bc7e3303076b8667009f
fa39c8a3e8 test: Work around libFuzzer deadlock (MarcoFalke)
Pull request description:
Only required part is `symbolize=0`, but the other changes shouldn't hurt
ACKs for top commit:
practicalswift:
cr ACK fa39c8a3e8: patch looks correct
Tree-SHA512: 9cddf1de46ad12aea9b8be2c1acb86ba0e07ffdb52f8155d943edf970955551c7cb049a3a6c027846b45dab0dc0966dec42999476ebde50aa761a08dbb751eae
8f0b64fb51 Better error messages for invalid addresses (Bezdrighin)
Pull request description:
This PR addresses #20809.
We add more detailed error messages in case an invalid address is provided inside the 'validateaddress' and 'getaddressinfo' RPC calls. This also covers the case when a user provides an address from a wrong network.
We also add a functional test to test the new error messages.
ACKs for top commit:
kristapsk:
ACK 8f0b64fb51
meshcollider:
Code review ACK 8f0b64fb51
Tree-SHA512: ca0f806ab573e96b79e98d9f8c810b81fa99c638d9b5e4d99dc18c8bd2568e6a802ec305fdfb2983574a97a19a46fd53b77645f8078fb77e9deb24ad2a22cf93
bb6fcc75d1 refactor: Drop boost::thread stuff in CCheckQueue (Hennadii Stepanov)
6784ac471b bench: Use CCheckQueue local thread pool (Hennadii Stepanov)
dba30695fc test: Use CCheckQueue local thread pool (Hennadii Stepanov)
01511776ac Add local thread pool to CCheckQueue (Hennadii Stepanov)
0ef938685b refactor: Use member initializers in CCheckQueue (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of `boost::thread_group` in the `CCheckQueue` class
- allows thread safety annotation usage in the `CCheckQueue` class
- is alternative to #14464 (https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-616618525, https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-617291612)
Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938.
Related: #17307
ACKs for top commit:
laanwj:
Code review ACK bb6fcc75d1
LarryRuane:
ACK bb6fcc75d1
jonatack:
Code review ACK bb6fcc75d1 and verified rebase to master builds cleanly with unit/functional tests green
Tree-SHA512: fddeb720d5a391b48bb4c6fa58ed34ccc3f57862fdb8e641745c021841c8340e35c5126338271446cbd98f40bd5484f27926aa6c3e76fa478ba1efafe72e73c1
This commit addresses #20809.
We add an additional 'error' property in the result of 'validateaddress' in case the address is not valid that gives a short description of why the address in invalid. We also change the error message returned by 'getaddressinfo' in case the address is invalid.
Add a functional test for CaptureMessage. This connects and then
disconnects a peer so that the handshake can be used to check if capture
is being done correctly.
Included in a docstring in the test is the following:
From the data file we'll only check the structure.
We won't care about things like:
- Deserializing the payload of the message
- This is managed by the deserialize methods in
test_framework.messages
- The order of the messages
- There's no reason why we can't, say, change the order of the
messages in the handshake
- Message Type
- We can add new message types
We're ignoring these because they're simply too brittle to test here.
This commit adds contrib/message-capture/message-capture-parser.py, a python
script to be used alongside -capturemessages to parse the captured
messages.
It is complete with arguments and will parse any file given, sorting the
messages in the files when creating the output. If an output file is
specified with -o or --output, it will dump the messages in json format
to that file, otherwise it will print to stdout.
The small change to the unused msg_generic is to bring it in line with
the other message classes, purely to avoid a bug in the future.
ff44cae279 test: Change feature_config_args.py not to rely on strange regtest=0 behavior (Russell Yanofsky)
Pull request description:
Update test to simply generate a normal mainnet configuration file instead of using a crazy setup where a regtest=1 config file using an includeconf in the [regtest] section includes another config file that specifies regtest=0, retroactively switching the network to mainnet.
This setup was fragile and only worked because the triggered InitError happened early enough that none of the ignored [regtest] options mattered (only affecting log output).
This change was originally made as part of #17493
Top commit has no ACKs.
Tree-SHA512: 3f77305454f04438493dfc2abd78a00434b30869454d1c3f54587b9c1f63239c49c90fb3b4d3a777ad130f2184e0f2dac87cee4cd23c50f1b3496a375943da01
4efb6c2d3b zmq test: deduplicate test setup code (node restart, topics subscription) (Sebastian Falbesoner)
Pull request description:
This PR deduplicates common setup code for the ZMQ functional test. The following steps, previously duplicated in each sub-test, are put into a new method `setup_zmq_test(...)`:
- create subscriber sockets (`zmq.SUB`) for each topic with the specified timeout (default 60s)
- restart node0 with specified zmq notifications enabled (`-zmqpub...=tcp://127.0.0.1:...`...)
- if desired, connect node0 with node1 (note done by default)
- connect all susbcriber sockets to publisher (running on node0)
- wait a bit (currently 200ms), to _"Relax so that the subscribers are ready before publishing zmq messages"_
Note that the last point should be repaced by a more robust method, as this test is still flaky, see #20934 (also #20590 and #20538).
ACKs for top commit:
instagibbs:
ACK 4efb6c2d3b
laanwj:
Code review ACK 4efb6c2d3b
Tree-SHA512: d49626756a9c669f1133f1b73ce273994b58c760ce0d6a4bdaa384f043a74149dc2b9fa66fe990413d9105f9c3b6ea973e099669e8e02f2902a5b84fa995028c
1fca9811e1 lint: Skip whitespace lint for guix patches (Carl Dong)
a91c46c57d guix: Make nsis reproducible by respecting SOURCE-DATE-EPOCH (Carl Dong)
Pull request description:
```
When building nsis, if VERSION is not specified, it defaults to
cvs_version which is non-deterministic as it includes the current date.
This patches nsis to default to SOURCE_DATE_EPOCH if it exists so that
nsis is reproducible.
Upstream change: https://github.com/kichik/nsis/pull/13
```
Sidenote: also a good demonstration of how Guix allows us to flexibly patch our tools!
Note to reviewers: if you want to compare hashes, please build after Jan 16th 2021 without my substitute server enabled!
ACKs for top commit:
fanquake:
ACK 1fca9811e1
Tree-SHA512: b800e0ce5f73827ad353739effb9167ec3a6bdb362c725ae20dd3f025ce78660f85c70ce1d75cd0896facf1e8fe38a9e058459ed13dec71ab3a2fe41e20eaa5d
ea0a7ec949 Remove deprecated bumpfee behavior (Andrew Chow)
Pull request description:
Removes the deprecation message, behavior, and test.
This was marked for removal in 22.0.
ACKs for top commit:
promag:
ACK ea0a7ec949, maybe add need release notes tag.
Tree-SHA512: d1626906849f6ee37213c32e5f8c1433ad8fb7beabcd88f5801b1964b322171a2341bdfbd9a3a5ab39b2fd9d9c6a05f73298583423a73cab1275653105c03e8e
fa0aa87071 rpc: Return wtxid from testmempoolaccept (MarcoFalke)
Pull request description:
It would be nice if `testmempoolaccept` returned the unique wtxid directly to avoid a costly `decoderawtransaction` roundtrip
ACKs for top commit:
mjdietzx:
utACK fa0aa87071
stackman27:
utACK [`fa0aa87`](fa0aa87071)
glozow:
cr ACK fa0aa87071
Tree-SHA512: 05dbaf46d93e47e9eedb725c2f57175e6d4e1722da0531fe4f80e74fc2518911da87634f25f61fa2bc8d87a3017e82fd0684b09a0a9706d71deed970035c2e7a
b4dd2ef800 [test] Test the add_outbound_p2p_connection functionality (Amiti Uttarwar)
602e69e427 [test] P2PBlocksOnly - Test block-relay-only connections. (Amiti Uttarwar)
8bb6beacb1 [test/refactor] P2PBlocksOnly - Extract transaction violation test into helper. (Amiti Uttarwar)
99791e7560 [test/refactor] P2PBlocksOnly - simplify transaction creation using blocktool helper. (Amiti Uttarwar)
3997ab9154 [test] Add test framework support to create outbound connections. (Amiti Uttarwar)
5bc04e8837 [rpc/net] Introduce addconnection to test outbounds & blockrelay (Amiti Uttarwar)
Pull request description:
The existing functional test framework uses the `addnode` RPC to spin up manual connections between bitcoind nodes. This limits our ability to add integration tests for our networking code, which often executes different code paths for different connection types.
**This PR enables creating `outbound` & `block-relay-only` P2P connections in the functional tests.** This allows us to increase our p2p test coverage, since we can now verify expectations around these connection types.
This builds out the [prototype](https://github.com/bitcoin/bitcoin/issues/14210#issuecomment-527421978) proposed by ajtowns in #14210. 🙌🏽
An overview of this branch:
- introduces a new test-only RPC function `addconnection` which initiates opening an `outbound` or `block-relay-only` connection. (conceptually similar to `addnode` but for different connection types & restricted to regtest)
- adds `test_framework` support so a mininode can open an `outbound`/`block-relay-only` connection to a `P2PInterface`/`P2PConnection`.
- updates `p2p_blocksonly` tests to create a `block-relay-only` connection & verify expectations around transaction relay.
- introduces `p2p_add_connections` test that checks the behaviors of the newly introduced `add_outbound_p2p_connection` test framework function.
With these changes, there are many more behaviors that we can add integration tests for. The blocksonly updates is just one example.
Huge props to ajtowns for conceiving the approach & providing me feedback as I've built out this branch. Also thank you to jnewbery for lots of thoughtful input along the way.
ACKs for top commit:
troygiorshev:
reACK b4dd2ef800
jnewbery:
utACK b4dd2ef800
MarcoFalke:
Approach ACK b4dd2ef800🍢
Tree-SHA512: d1cba768c19c9c80e6a38b1c340cc86a90701b14772c4a0791c458f9097f6a4574b4a4acc7d13d6790c7b1f1f197e2c3d87996270f177402145f084ef8519a6b
39b43298d9 test: add test for banning of non-IP addresses (Vasil Dimov)
94d335da7f net: allow CSubNet of non-IP networks (Vasil Dimov)
Pull request description:
Allow creation of valid `CSubNet` objects of non-IP networks and only
match the single address they were created from (like /32 for IPv4 or
/128 for IPv6).
This fixes a deficiency in `CConnman::DisconnectNode(const CNetAddr& addr)`
and in `BanMan` which assume that creating a subnet from any address
using the `CSubNet(CNetAddr)` constructor would later match that address
only. Before this change a non-IP subnet would be invalid and would not
match any address.
ACKs for top commit:
jonatack:
Code review re-ACK 39b43298d9 per `git diff 5e95ce6 39b4329`; only change since last review is improvements to the functional test; verified the test fails on master @ 616eace0 where expected (`assert(self.is_banned(node, tor_addr))` fails and unban unfails)
laanwj:
code review ACK 39b43298d9
Tree-SHA512: 3239b26d0f2fa2d1388b4fdbc1d05ce4ac1980be699c6ec46049409baefcb2006b1e72b889871e2210e897f6725c48e873f68457eea7e6e4958ab4f959d20297
faabc26a61 test: Replace getmempoolentry with testmempoolaccept in MiniWallet (MarcoFalke)
Pull request description:
This is a refactor to not use the return value of `sendrawtransaction` and `getmempoolentry` with the goal that submitting the tx to the mempool will become optional.
ACKs for top commit:
mjdietzx:
ACK faabc26a61
Tree-SHA512: 4260a59e65fed1c807530dad23f1996ba6e881bcce91995f5b498a0be6001f67b3e0251507c8801750fe105410147c68bb2f393ebe678c6a3a4d045e5d72fc19
faa8f68943 Replace boost::variant with std::variant (MarcoFalke)
Pull request description:
Now that we can use std::variant from the vanilla standard library, drop the third-party boost variant dependency
ACKs for top commit:
fjahr:
Code review ACK faa8f68943
fanquake:
ACK faa8f68943
Tree-SHA512: 6e3aecd33b00c2e31a763f999247944d5b2ce5e3018f1965c516c1000cd08ff6703a8d50fb0be64883153da2925ae72986b8a6b96586db74057bd05d6f4986e6
a7599c80eb test: run mempool_compatibility.py even with wallet disabled (Michael Dietz)
Pull request description:
Another functional test rewritten as proposed in https://github.com/bitcoin/bitcoin/issues/20078
ACKs for top commit:
MarcoFalke:
review ACK a7599c80eb didn't test
Tree-SHA512: cc7a617e5489ed27bbdbdee85a82fa08525375061f7f4524577a6b8ecb340396adac88419b51f513be22ca53edd0a3bd5d572d9f43ffc2c18550b0ef9069d238
a191e23b8e doc: Add release notes (Hennadii Stepanov)
ae749d12dd doc: Add libnatpmp stuff (Hennadii Stepanov)
e28f9be87a ci: Add libnatpmp-dev package to some builds (Hennadii Stepanov)
5a0185b6c9 gui: Add NAT-PMP network option (Hennadii Stepanov)
a39f7336a3 net: Add -natpmp command line option (Hennadii Stepanov)
28acffd9d5 net: Add NAT-PMP to port mapping loop (Hennadii Stepanov)
a8d9f275d0 net: Add libnatpmp support (Hennadii Stepanov)
58e8364dcd gui: Apply port mapping changes on dialog exit (Hennadii Stepanov)
cf151cc68c scripted-diff: Rename UPnP stuff (Hennadii Stepanov)
4e91b1e24d net: Add flags for port mapping protocols (Hennadii Stepanov)
8b50d1b5bb net: Keep trying to use UPnP when -upnp=1 (Hennadii Stepanov)
28e2961fd6 refactor: Replace magic number with named constant (Hennadii Stepanov)
02ccf69dd6 refactor: Move port mapping code to its own module (Hennadii Stepanov)
Pull request description:
Close#11902
This PR is an alternative to:
- #12288
- #15717
To compile with NAT-PMP support on Ubuntu [`libnatpmp-dev`](https://packages.ubuntu.com/source/bionic/libnatpmp) should be available.
Log excerpt:
```
2020-02-05T20:12:28Z [mapport] NAT-PMP: public address = 95.164.65.194
2020-02-05T20:12:28Z [mapport] AddLocal(95.164.65.194:18333,3)
2020-02-05T20:12:28Z [mapport] NAT-PMP: port mapping successful.
```
See: [`libnatpmp`](https://miniupnp.tuxfamily.org/libnatpmp.html)
---
Some follow-ups are out of this PR's scope:
- mention NAT-PMP library in the version message
- ~integrate NAT-PMP into the GUI~ (already [added](https://github.com/bitcoin/bitcoin/pull/18077#issuecomment-589405068))
ACKs for top commit:
laanwj:
Tested and code review ACK a191e23b8e
Tree-SHA512: 10e19267c21bf30f20ff1abfc882d526049f0e790b95e12f109dc2bed7c0aef45de03eaf967f4e667e7509be04f1873a5c508087393d947205f3aab2ad6d7cf1
Open max number of full-relay and block-relay-only connections from a
functional test with different sorts of behaviors to ensure it behaves as
expected.
In the interest of increasing our P2P test coverage, add support to create
full-relay or block-relay-only connections. To support this, a P2P connection
spins up a listening thread & uses a callback to trigger the node initiating
the connection.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
9815332d51 test: Change MuHash Python implementation to match cpp version again (Fabian Jahr)
01297fb3ca fuzz: Add MuHash consistency fuzz test (Fabian Jahr)
b111410914 test: Add MuHash3072 fuzz test (Fabian Jahr)
c122527385 bench: Add Muhash benchmarks (Fabian Jahr)
7b1242229d test: Add MuHash3072 unit tests (Fabian Jahr)
adc708c98d crypto: Add MuHash3072 implementation (Fabian Jahr)
0b4d290bf5 crypto: Add Num3072 implementation (Fabian Jahr)
589f958662 build: Check for 128 bit integer support (Fabian Jahr)
Pull request description:
This is the first split of #18000 which implements the Muhash algorithm and uses it to calculate the UTXO set hash in `gettxoutsetinfo`.
ACKs for top commit:
laanwj:
Code review ACK 9815332d51
Tree-SHA512: 4bc090738f0e3d80b74bdd8122e24a8ce80121120fd37c7e4335a73e7ba4fcd7643f2a2d559e2eebf54b8e3a3bd5f12cfb27ba61ded135fda210a07a233eae45
fa6c114ae6 test: Add sanitizer suppressions for AMD EPYC CPUs (MarcoFalke)
Pull request description:
Currently the ci system only runs on intel cpus (and some arm devices), but it won't run on CPUs `Using the 'shani(1way,2way)' SHA256 implementation` (excerpt from debug log).
For reference, google cloud CPUs (which is what Cirrus CI uses) print `Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation`
The traceback I got:
```
crypto/sha256_shani.cpp:87:18: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' (aka 'unsigned long')
#0 0x55c0000e95ec in sha256_shani::Transform(unsigned int*, unsigned char const*, unsigned long) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256_shani.cpp:87:18
#1 0x55bfffb926f8 in (anonymous namespace)::SelfTest() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:517:9
#2 0x55bfffb906ed in SHA256AutoDetect[abi:cxx11]() /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/crypto/sha256.cpp:626:5
#3 0x55bfff87ab97 in BasicTestingSetup::BasicTestingSetup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<char const*, std::allocator<char const*> > const&) /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/util/setup_common.cpp:104:5
#4 0x55bffe885877 in main /root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_main.cpp:52:27
#5 0x7f20c3bf60b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
#6 0x55bffe7a5f6d in _start (/root/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/qt/test/test_bitcoin-qt+0x1d00f6d)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow crypto/sha256_shani.cpp:87:18 in
ACKs for top commit:
laanwj:
Anyhow ACK fa6c114ae6
Tree-SHA512: 968a1d28eedec58c337b1323862f583cb1bcd78c5f03396940b9ab53ded12f8c6652877909aba05ee5586532137418fd817ff979bd7bef6e07856094f9d7f9b1
1112035d32 doc: fix various typos (Ikko Ashimine)
e8640849c7 doc: Use https URLs where possible (Sawyer Billings)
Pull request description:
Consolidates / fixes the changes from #20762, #20836, #20810. There is no output when `test/lint/lint-all.sh` is run.
Closes#20807.
ACKs for top commit:
MarcoFalke:
ACK 1112035d32
Tree-SHA512: 22ca824688758281a74e5ebc6a84a358142351434e34c88c6b36045d2d241ab95fd0958565fd2060f98317e62e683323b5320cc7ec13592bf340e6922294ed78
fa749fbea3 rpc: Replace boost::variant with std::variant for RPCArg.m_fallback (MarcoFalke)
Pull request description:
Now that we can use std::variant from the vanilla standard library, drop the third-party boost variant dependency.
Patch is split out from #20480. A step-by-step replacement is possible because we don't have our own `Variant` wrapper and the source code specifies `boost::variant` explicitly.
I think a step-by-step replacement should be preferred, because it simplifies review.
ACKs for top commit:
fjahr:
re-ACK fa749fbea3
Sjors:
re-ACK fa749fbea3
Tree-SHA512: 5e3c12b7d535f73065b4afa8df0a488f78fb25d2234f5ecbf740e624db03a34c35fea100eb7d37e84741721310e6450b7fb4296a2207a7ed1fa24485b3650981
454a4088a8 [doc] Add release notes for removed getpeerinfo fields. (Amiti Uttarwar)
b1a936d4ae [rpc] Remove deprecated "whitelisted" field from getpeerinfo (Amiti Uttarwar)
094c3beaa4 [rpc] Remove deprecated "banscore" field from getpeerinfo (Amiti Uttarwar)
537053336f [rpc] Remove deprecated "addnode" field from getpeerinfo (Amiti Uttarwar)
Pull request description:
This PR removes support for 3 fields on the `getpeerinfo` RPC that were deprecated in v0.21- `addnode`, `banscore` & `whitelisted`.
ACKs for top commit:
sipa:
utACK 454a4088a8
jnewbery:
ACK 454a4088a8.
Tree-SHA512: ccc0e90c0763eeb8529cf0c46162dbaca3f7773981b3b52d9925166ea7421aed086795d56b320e16c9340f68862388785f52a9b78314865070917b33180d7cd6
40fdb2a212 test: Fix Comment Typo in BitcoinTestFramework (Joel Klabo)
Pull request description:
Missing "override" in comment describing use of set_test_params
ACKs for top commit:
michaelfolkson:
ACK 40fdb2a212
Tree-SHA512: bf893a0d5f8dc86a3ec2eaf48cd7c0f0f832f3b3d254b3d99953336db7e294571b1d2c8686030bf8a27cbe67b1a85a54e53ebefb2e57d6d8d6ac864a15dce4e7
