Now that we require Python 3.6+, we should be using variable type
annotations directly rather than # type: comments.
Also takes care of the discarded value issue in p2p_message_capture.py.
See: https://github.com/bitcoin/bitcoin/pull/19509/files#r571674446.
5e0cd25e29 fix the unreachable code at feature_taproot (Bruno Garcia)
Pull request description:
This PR removes the unnecessary return statement at the beginning of the function that makes the rest of the function unreachable.
ACKs for top commit:
practicalswift:
cr ACK 5e0cd25e29: patch looks correct!
sipa:
ACK 5e0cd25e29.
theStack:
Tested ACK 5e0cd25e29🏔️
sanket1729:
tACK 5e0cd25e29. I noted this a while ago while fixing feature_taproot.py for elements. Verified that the extreme ranges of CScriptNum are correct and the overflow case for `CHECKSIGADD` works as intended. Adding 1 to 2^31 - 1 results in an overflow, but the interpreter puts a `vch` of corresponding to 2^31 on stack. Even though it cannot be converted to CscriptNum(restricted to 4 bytes), it's result can still be compared by OP_EQUAL.
Tree-SHA512: fff9be3be94f4b3f3ccf24bf588d96e84d14806f82692dccd31631b0e5c79a7575a96c308cb5a4f610ab02e2f854b899f374437c33ecf6d52055d333f2de9b27
49797c3ccf tests: Disable bdb dump test when no bdb (Andrew Chow)
1194cf9269 Fix wallet_send.py wallet setup to work with descriptors (Andrew Chow)
fbaea7bfe4 Require legacy wallet for wallet_upgradewallet.py (Andrew Chow)
b1b679e0ab Explicitly mark legacy wallet tests as such (Andrew Chow)
09514e1bef Setup wallets for interface_zmq.py (Andrew Chow)
4d03ef9a73 Use MiniWallet in rpc_net.py (Andrew Chow)
4de23824b0 Setup wallets for interface_bitcoin_cli.py (Andrew Chow)
7c71c627d2 Setup wallets with descriptors for feature_notifications (Andrew Chow)
1f1bef8dba Have feature_filelock.py test both bdb and sqlite, depending on compiled (Andrew Chow)
c77975abc0 Disable upgrades tests that require BDB if BDB is not compiled (Andrew Chow)
1f20cac9d4 Disable wallet_descriptor.py bdb format check if BDB is not compiled (Andrew Chow)
3641597d7e tests: Don't make any wallets unless wallet is required (Andrew Chow)
b9b88f57a9 Skip legacy wallet reliant tests if BDB is not compiled (Andrew Chow)
6f36242389 tests: Set descriptors default based on compilation (Andrew Chow)
Pull request description:
This PR fixes tests for when BDB is not compiled. Tests which rely on or test legacy wallet behavior are disabled and skipped when BDB is not compiled. For the components of some tests that are for legacy wallet things, those parts of the tests are skipped.
For the majority of tests, changes are made so that they can be run with either legacy wallets or descriptor wallets without materially effecting the test. Most tests only need the wallet for balance and transactions, so the type of wallet is not an important part of those tests. Additionally, some tests are wallet agnostic and modified to instead use the test framework's MiniWallet.
ACKs for top commit:
laanwj:
ACK 49797c3ccf
ryanofsky:
Code review ACK 49797c3ccf. Only change since last review is dropping last commit. Previous review w/ suggestions for future followup is https://github.com/bitcoin/bitcoin/pull/20267#pullrequestreview-581508843
Tree-SHA512: 69659f8a81fb437ecbca962f4082c12835282dbf1fba7d9952f727a49e01981d749af9b09feda1c8ca737516c7d7a08ef17e782795df3fa69892d5021b41c1ed
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
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
Affects the following RPCs:
- getblockheader
- getblock
Also adds trivial tests on genesis block (should not contain
"previousblockhash") and best block (should not contain
"nextblockhash").
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
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
