af2a145e57 Refactor resource exhaustion test (Troy Giorshev)
5c4648d17b Fix "invalid message size" test (Troy Giorshev)
ff1e7b8844 Move size limits to module-global (Troy Giorshev)
57890abf2c Remove two unneeded tests (Troy Giorshev)
Pull request description:
This PR touches only the p2p_invalid_messages.py functional test module. There are two main goals accomplished here. First, it fixes the "invalid message size" test, which previously made a message that was invalid for multiple reasons. Second, it refactors the file into a single consistent style. This file appears to have originally had two authors, with different styles and some test duplication.
It should now be easier and quicker to understand this module, anticipating the upcoming [BIP324](https://github.com/bitcoin/bitcoin/pull/18242) and [AltNet](https://github.com/bitcoin/bitcoin/issues/18989) changes.
This should probably go in ahead of #19107, but the two are not strictly related.
ACKs for top commit:
jnewbery:
ACK af2a145e57
MarcoFalke:
re-ACK af2a145e57🍦
Tree-SHA512: 9b57561e142c5eaefac5665f7355c8651670400b4db1a89525d2dfdd20e872d6873c4f6175c4222b6f5a8e5210cf5d6a52da69b925b673a2e2ac30a15d670d1c
45eff751c6 Add functional test for P2P eviction logic of inbound peers (Martin Zumsande)
Pull request description:
This adds a functional test for the eviction logic for inbound peers, which is triggered when the number of maximum connections is exceeded.
The functional test covers eviction protection for peers that have sent us blocks or txns recently, or that have faster pings. I couldn't find a way to test the logic of `CConnman::AttemptToEvictConnection` that is based on netgroup (see #14210 for related discussion)
Fixes#16660 (at least partially).
[Edit: Earlier, this PR also contained a unit test, which was removed after the discussion]
ACKs for top commit:
jonatack:
ACK 45eff751c6
naumenkogs:
Tested ACK 45eff75
fjahr:
re-ACK 45eff751c6
andrewtoth:
re-ACK 45eff751c6
Tree-SHA512: 177208ab6f30dc62da1cc5f51e654f7c9770d8c6b42aca6ae7ecb30e29d3096e04d75739578e7d149a0f29dd92652b4a707e93c0f1be8aa7ed315e6ec3ab07a4
fadf6bd04f refactor: Remove unused request.fHelp (MarcoFalke)
fad889cbf0 wallet: Make RPC help compile-time static (MarcoFalke)
Pull request description:
Currently calling `help` on a wallet RPC method will either return `help: unknown command: getnewaddress` or the actual help. This runtime dependency of the help is a bug that complicates any tool that relies on documentation. Also, the code that enables the bug is overly complicated and confusing.
The fix is split into two commits:
* First, a commit that can be reviewed with the `--color-moved=dimmed-zebra` option and tested with the included test.
* Second, a commit that removes the complicated and confusing code.
ACKs for top commit:
achow101:
re-ACK fadf6bd04f
promag:
Tested ACK fadf6bd04f.
Tree-SHA512: 65d4ff400467f57cb8415c30ce30f814dc76c5c157308b7a7409c59ac9db629e65dfba31cd9c389cfe60a008d3d87787ea0a0e0f2671fd65fd190543c915493d
dca73941eb scripted-diff: rename node to peer for mininodes (gzhao408)
0474ea25af [test] fix race conditions and test in p2p_filter (gzhao408)
4ef80f0827 [test] sending invalid msgs to node with bloomfilters=0 causes disconnect (gzhao408)
497a619386 [test] add BIP 37 test for node with fRelay=false (gzhao408)
e8acc60156 [test] add mempool msg test for node with bloomfilter enabled (gzhao408)
Pull request description:
This PR adds a few tests that are bloomfilter-related, including behavior for when bloomfilters are turned _off_:
1. Tests p2p message `msg_mempool`: a node that has `peerbloomfilters` enabled should send its mempool (disabled behavior already tested [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_mempool.py)).
2. Tests that bloomfilter peers with [`fRelay=False`](https://github.com/bitcoin/bips/blob/master/bip-0037.mediawiki#extensions-to-existing-messages) in the `version` message should not receive any invs until they set the filter. The rest is the same as what’s already tested in `p2p_filter.py`.
3. Tests that peers get disconnected if they send `filterload` or `filteradd` p2p messages to a node with bloom filters disabled.
4. Refactor: renames p2p_mempool.py to p2p_nobloomfilter_messages.py.
5. Fixes race conditions in p2p_filter.py
ACKs for top commit:
MarcoFalke:
ACK dca73941eb only changes is restoring accidentally deleted test 🍮
jonatack:
ACK dca73941eb modulo a few nits if you retouch, happy to re-ACK if you take any of them but don't feel obliged to.
Tree-SHA512: 442aeab0755cb8b830251ea170d1d5e6da8ac9029b3276d407a20ee3d588cc61b77b8842368de18c244056316b8c63b911776d6e106bc7c023439ab915b27ad3
3a83a01694 [tests] move generate_wif_key to wallet_util.py (John Newbery)
b216b0b71f [tests] sort imports in rpc_createmultisig.py (John Newbery)
e38081846d Revert "[TESTS] Move base58 to own module to break circular dependency" (John Newbery)
Pull request description:
generate_wif_key is a wallet utility function. Move it from the EC key module to the wallet util module.
This fixes the circular dependency issue in #17977
ACKs for top commit:
MarcoFalke:
ACK 3a83a01694🍪
Tree-SHA512: 24985dffb75202721ccc0c6c5b52f1fa5d1ce7963bccde24389feb913cab4dad0c265274ca67892c46c8b64e6a065a0f23263a89be4fb9134dfefbdbe5c7238a
fa98e10d5e test: Remove leftover comment in mining_basic (MarcoFalke)
faedb50d89 test: pep-8 mining_basic (MarcoFalke)
Pull request description:
Remove an accidental leftover comment from #19082, which no longer applies and thus might be confusing
ACKs for top commit:
adamjonas:
code review ACK fa98e10
Tree-SHA512: c7f7f8f579b3c6e92f45769be0a7af1a421438a3f5524db5278b2269511a9e0e08f44e3836afb26727644035897ee51ff8296d13ce23030549e7403f57b40e40
-A node with bloomfilters disabled should disconnect peers that send
msg_mempool, msg_filterload, or msg_filteradd.
-Renamed the test because it now has a wider scope and msg_mempool's
actual functionality makes more sense for p2p_filter.py.
-msg_mempool is currently only tested with bloomfilter disabled
(node is disconnected) in p2p_mempool.py
-msg_mempool should get mempool txns in response when bloomfilter
is enabled
-edit test that doesn't test msg_mempool as intended
c75de5da5f [TESTS] Move base58 to own module to break circular dependency (Pieter Wuille)
Pull request description:
I encountered difficulties with the test framework in #17977. This fixes them, and I think the change is generally useful.
ACKs for top commit:
laanwj:
Code review ACK c75de5da5f
MarcoFalke:
ACK c75de5da5f according to --color-moved=dimmed-zebra this is a move-only apart from the imports 👒
Tree-SHA512: 9e0493de3e279074f0c70e92c959b73ae30479ad6f2083a3c6bbf4b0191d65ef94854559a5b7c904f5dadc5e93129ed00f6dc0a8ccce6ba7921cd45f7119f74b
34645c4dd0 Test txinwitness is accessible on coinbase vin (Rod Vagg)
3e4421070a Expose txinwitness for coinbase in JSON form (Rod Vagg)
Pull request description:
## Rationale
The CLI can provide you with everything about transactions and blocks that you need to reconstruct the block structure and raw block itself **except** for the witness commitment nonce which is stored in the `scriptWitness` of the coinbase and is not printed. You could manually parse the raw `"hex"` fields for transactions if you really wanted to, but this seems to defeat the point of having a JSONification of the raw block/transaction data.
Without the nonce you can't:
1. calculate and validate the witness commitment yourself, you can generate the witness tx merkle root but you don't have the nonce to combine it with
2. reconstruct the raw block form because you don't have `scriptWitness` stack associated with the coinbase (although you know how big it will be and can guess the common case of `[0x000...000]`)
I'm building some archiving tooling for block data and being able to do a validated two-way conversion is very helpful.
## What
This PR simply makes the `txinwitness` field not dependent on whether we are working with the coinbase or not. So you get it for the coinbase as well as the rest.
## Examples
Common case of a `[0x000...000]` nonce: 00000000000000000000140a7289f3aada855dfd23b0bb13bb5502b0ca60cdd7
```json
"vin": [
{
"coinbase": "0368890904c1fe8d5e2f706f6f6c696e2e636f6d2ffabe6d6d5565843a681160cf7b08b1b74ac90a719e6d6ab28c16d336b924f0dc2fcabdc6010000000000000051bf2ad74af345dbe642154b2658931612a70d195e007add0100ffffffff",
"txinwitness": [
"0000000000000000000000000000000000000000000000000000000000000000"
],
"sequence": 4294967295
}
],
...
```
Novel nonce value: 000000000000000000008c31945b2012258366cc600a3e9a3ee0598e8f797731
```json
"vin": [
{
"coinbase": "031862082cfabe6d6d80c099b5e21f4c186d54eb292e17026932e52b1b807fa1380574c5adc1c843450200000000000000",
"txinwitness": [
"5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d5b5032506f6f6c5d"
],
"sequence": 4294967295
}
],
...
```
## Alternatives
This field could be renamed for the coinbase, `"witnessnonce"` perhaps. It could also be omitted when null/zero (`0x000...000`).
## Tests
This didn't break any tests and I couldn't find an obvious way to include a test for this. If this is desired I'd apreicate some pointers.
ACKs for top commit:
MarcoFalke:
ACK 34645c4dd0
Tree-SHA512: b192facc1dfd210a5ec3f0d5d1ac6d0cae81eb35be15eaa71f60009a538dd6a79ab396f218434e7e998563f7f0df2c396cc925cb91619f6841c5a67806148c85
34e641a564 test: Remove unnecessary disconnect_nodes call in rpc_psbt.py (Danny Lee)
e6e7abd51a test: remove redundant two-way disconnect_nodes calls (Danny Lee)
a9bd1f9adf test: warn if nodes not connected before disconnect_nodes (Danny Lee)
Pull request description:
There's no harm in calling `disconnect_nodes` for nodes that weren't connected (in this case it's a no-op). However, detecting this case and logging a warning can help ensure that tests are behaving as expected.
In addition, since `disconnect_nodes` works bidirectionally, I removed all instances of this pattern:
```
disconnect_nodes(self.nodes[0], 1)
disconnect_nodes(self.nodes[1], 0)
```
ACKs for top commit:
MarcoFalke:
review ACK 34e641a564👔
amitiuttarwar:
ACK 34e641a564. Thanks for this test improvement!
Tree-SHA512: 344855ceb46c012d43c13d7c09f44d32dcb7645706d10ae1e4645d9edca54c6c6c13fee26b79480755cdfcdf39b4b5770b36bb03ce71ba002d5be8a27fe008af
This is a simple refactor of the specified test. It is now brought in
line with the rest of the tests in the module. This should make things
easier to debug, as all of the tests are now grouped together at the
top.
This test originally made a message with an invalid stated length, and
an invalid checksum. This was because only the header was changed, but
the checksum stayed the same. This was fine for now because we check
the header first to see if it has a valid stated size, and we disconnect
if it does not, so we never end up checking for the checksum. If this
behavior was to change, this test would become a problem. (Indeed I
discovered this when playing around with this behavior). By instead
creating a message with an oversized payload from the start, we create a
message with an invalid stated length but a valid checksum, as intended.
Additionally, this takes advantage to the newly module-global
VALID_DATA_LIMIT as opposed to the magic 0x02000000. Yes, 4MB < 32MiB,
but at the moment when receiving a message we check both, so this makes
the test tighter.
As well, this renames those variables to match PEP8 and this clears up
the comment relating to VALID_DATA_LIMIT.
Admittedly, this commit is mainly to make the following ones cleaner.
Test 1 is a duplicate of test_size() later in the file. Inexplicably,
this test does not work on macOS, whereas test_size() does.
Test 2 is problematic for two reasons. First, it always fails with an
invalid checksum, which is probably not what was intended. Second, it's
not defined at this layer what the behavior should be. Hypothetically,
if this test was fixed so that it gave messages with valid checksums,
then the message would pass successfully thought the network layer and
fail only in the processing layer. A priori the network layer has no
idea what the size of a message "actually" is.
The "Why does behavior change at 78 bytes" is because of the following:
print(len(node.p2p.build_message(msg))) # 125
=> Payload size = 125 - 24 = 101
If we take 77 bytes, then there are 101 - 77 = 24 left
That's exactly the size of a header
So, bitcoind deserializes the header and rejects it for some other reason
(Almost always an invalid size (too large))
But, if we take 78 bytes, then there are 101 - 78 = 23 left
That's not enough to fill a header, so the socket stays open waiting for
more data. That's why we sometimes have to push additional data in
order for the peer to disconnect.
Additionally, both of these tests use the "conn" variable. For fun, go
look at where it's declared. (Hint: test_large_inv(). Don't we all
love python's idea of scope?)
fa9604c46f doc: noban precludes maxuploadtarget disconnects (MarcoFalke)
fa3999fe35 net: Reformat excessively long if condition into multiple lines (MarcoFalke)
Pull request description:
Whitelisting has been replaced by permission flags, so properly document this. See also #10131
ACKs for top commit:
hebasto:
ACK fa9604c46f, I have reviewed the code and it looks OK, I agree it can be merged.
ariard:
ACK fa9604c
Tree-SHA512: 5aee917ab9817719f01ec155487542118e17fa3d145ae7e4bc0e872b2cec39cde9e7fbdee2ae77e9a52700dd8bcc366de4224152e08e709d44d08e0d2f19c613
5d77549d8b doc: Add mypy to test dependencies (Hennadii Stepanov)
7dda912e1c test: Do not swallow flake8 exit code (Hennadii Stepanov)
Pull request description:
After #18210 the `flake8` exit code in `test/lint/lint-python.sh` just not used that makes the linter broken.
This PR:
- combines exit codes of `flake8` and `mypy` into the `test/lint/lint-python.sh` exit code
- documents `mypy` as the test dependency
ACKs for top commit:
MarcoFalke:
Approach ACK 5d77549d8b, fine with me
practicalswift:
ACK 5d77549d8b
Tree-SHA512: e948ba04dc4d73393967ebf3c6a26c40d428d33766382a0310fc64746cb7972e027bd62e7ea76898b742a656cf7d0fcda2fdd61560a21bfd7be249cea27f3d41
fa7e002d52 ci: tsan with wallet (MarcoFalke)
Pull request description:
ACKs for top commit:
practicalswift:
ACK fa7e002d52 -- patch looks correct and Travis is happy
hebasto:
ACK fa7e002d52, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 1138459bbef72f402f32dae1e28d96f174901d4248d959b538b973747c8b06f221ecd81a386a1f915c0a2faaefb9fb8f11e3be39e6c5e2468bf9ae43d9f97757
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
7daffc6a90 [test] CScriptNum Decode Check as Unit Tests (Gillian Chu)
Pull request description:
The CScriptNum test (#14816) is a roundtrip test of the test framework. Thus, it would be better suited as a unit test. This is now possible with the introduction of the unit test module for the functional tests. See #18576.
This PR:
1. Refactors the CScriptNum tests into 2 unit tests, one in script.py and one in blocktools.py.
2. Extends the script.py CScriptNum test to trial larger numbers.
ACKs for top commit:
laanwj:
ACK 7daffc6a90
Tree-SHA512: 17a04a4bfff1b1817bfc167824c679455d9e06e6e0164c00a7e44f8aa5041c5f5080adcc1452fd80ba1a6d8409f976c982bc481d686c434edf97a5893a32a436
fa1433ac1b rpc: Remove special case for unknown service flags (MarcoFalke)
Pull request description:
The special case to return a bit as an integer is clumsy and undocumented. Probably also irrelevant because there shouldn't currently be a non-misbehaving client that connects to Bitcoin Core and advertises an unknown service flag.
Thus, simply remove the code.
ACKs for top commit:
laanwj:
ACK fa1433ac1b
Tree-SHA512: 942de6a577a9ee076ce12c92be121617640d53ee8c3424064c45a30a7ff789555d3722a4203670768faf81da2a40adfed3ec5cdeb5da06f04be81ddb53b9db7e
eea8114657 build: Enable unreachable-code-loop-increment (Jonathan Schoeller)
d15db4b1fc refactor: Fix unreachable code in init arg checks (Jonathan Schoeller)
Pull request description:
Closes: #19017
In #19015 it's been suggested that we add some new compiler warnings to our build. Some of these, such as `-Wunreachable-code-loop-increment`, generate warnings. We'll likely want to fix these up if we're going to turn these warnings on.
```shell
init.cpp:969:5: warning: loop will run at most once (loop increment never executed) [-Wunreachable-code-loop-increment]
for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) {
^~~
1 warning generated.
```
aa8d76806c/src/init.cpp (L968-L972)
To fix this, collect all errors, and output them in a single error message after the loop completes. This resolves the unreachable code warning, and avoids popup hell that could result from outputting a seperate message for each error or warning one by one.
ACKs for top commit:
laanwj:
Code review ACK eea8114657
hebasto:
re-ACK eea8114657, only suggested changes applied since the [previous](https://github.com/bitcoin/bitcoin/pull/19131#pullrequestreview-421772387) review.
Tree-SHA512: 2aa3ceb7fab581b6ba2580900668388d8eba1c3001c8ff9c11c1f4a9a10fbc37f30e590249862676858446e3f4950140a252953ba1643ba3bfd772f8eae20583
bd7e530f01 This PR adds initial support for type hints checking in python scripts. (Kiminuo)
Pull request description:
This PR adds initial support for type hints checking in python scripts.
Support for type hints was introduced in Python 3.5. Type hints make it easier to read and review code in my opinion. Also an IDE may discover a potential bug sooner. Yet, as PEP 484 says: "It should also be emphasized that Python will remain a dynamically typed language, and the authors have no desire to ever make type hints mandatory, even by convention."
[Mypy](https://mypy.readthedocs.io/en/latest/index.html) is used in `lint-python.sh` to do the type checking. The package is standard so there is little chance that it will be abandoned. Mypy checks that type hints in source code are correct when they are not, it fails with an error.
**Notes:**
* [--ignore-missing-imports](https://mypy.readthedocs.io/en/latest/command_line.html#cmdoption-mypy-ignore-missing-imports) switch is passed on to `mypy` checker for now. The effect of this is that one does not need `# type: ignore` for `import zmq`. More information about import processing can be found [here](https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports). This can be changed in a follow-up PR, if it is deemed useful.
* We are stuck with Python 3.5 until 04/2021 (see https://packages.ubuntu.com/xenial/python3). When Python version is bumped to 3.6+, one can change:
```python
_opcode_instances = [] # type: List[CScriptOp]
```
to
```python
_opcode_instances:List[CScriptOp] = []
```
for type hints that are **not** function parameters and function return types.
**Useful resources:**
* https://docs.python.org/3.5/library/typing.html
* https://www.python.org/dev/peps/pep-0484/
ACKs for top commit:
fanquake:
ACK bd7e530f01 - the type checking is not the most robust (there are things it fails to detect), but I think this is worth adopting (in a limited capacity while we maintain 3.5 compat).
MarcoFalke:
ACK bd7e530f01 fine with me
Tree-SHA512: 21ef213915fb1dec6012f59ef17484e6c9e0abf542a316b63d5f21a7778ad5ebabf8961ef5fc8e5414726c2ee9c6ae07c7353fb4dd337f8fcef5791199c8987a
