-Waiting is important to avoid race conditions,
especially if testing peer info through rpc later.
-Wait for mininodes to be disconnected only, even
though it's more complex, because we may still want
to be connected to test nodes.
-Use peer to refer to mininodes instead of node
because they are not bitcoind nodes.
-Use log.debug for logs that give helpful but
not super necessary information.
-Adhere to style guidelines (newlines, capitalization).
5527be06277647dffe7cda587c4bbfbec2a5c8ca refactor: Add AbortError alias (Hennadii Stepanov)
d924f2a596c8f37deb2dd94069c578244823c31f Drop MSG_NOPREFIX flag (Hennadii Stepanov)
083daf7fbaf02de61f8d197ef6a8df98c1a57f7b Pass bilingual_str argument to AbortNode() (Hennadii Stepanov)
d1cca129b4b5b8e4830e442ebaee55dd0660b48a refactor: Use bilingual_str::empty() (Hennadii Stepanov)
Pull request description:
This PR is a [followup](https://github.com/bitcoin/bitcoin/issues/16218#issuecomment-625919724) of #16224, and it adds `bilingual_str` type argument support to the `AbortNode()` functions.
ACKs for top commit:
MarcoFalke:
ACK 5527be06277647dffe7cda587c4bbfbec2a5c8ca 👟
Tree-SHA512: bf8b15b14912b1f672e6e588fffa1e6eb6f00b4b23d15d0ced7f18fbdf76919244427feb7217007fe29617049308e13def893a03a87358db819cca9692f59905
16d4b3fd6d5aad18ebb731a5006a15180d3661ef test: mempool.dat compatibility between versions (Ivan Metlushko)
Pull request description:
Rationale: Verify mempool.dat compatibility between versions
The format of mempool.dat has been changed in #18038
The tests verifies the fix made in #18807 and ensures that the file format is compatible between current version and v0.19.1
The test verifies both backward and forward compatibility.
This PR also adds a log when we fail to add a tx loaded from mempool.dat.
It was useful when debugging this test and could be potentially useful to debug other scenarios as well.
Closes#19037
ACKs for top commit:
Sjors:
tACK 16d4b3fd6d5aad18ebb731a5006a15180d3661ef
Tree-SHA512: 00a38bf528c6478cb0da467af216488f83c1e3ca4d9166c109202ea8284023e99d87a3d6e252c4d88d08d9b5ed1a730b3e1970d6e5c0aef526fa7ced40de7490
Every `return false` is preceeded by a detailed debug log message to
explain that a disconnect or misbehavior happened. Logging another
generic "FAILED" message seems redundant.
Also, the size of the message and the message type has already been
logged and is thus redundant as well.
Finally, claiming that message processing FAILED seems odd, because the
message was fully processed to the point where it was concluded that the
peer should be either disconnected or marked as misbehaving.
62068381a3b9c065d81300be79abba7aecfdb41b [tests] Make mininode_lock non-reentrant (John Newbery)
c67c1f2c032a8efa141d776a7e5be58f052159ea [tests] Don't call super twice in P2PTxInvStore.on_inv() (John Newbery)
9d80762fa0931fe553fad241e95bcc1515ef0e95 [tests] Don't acquire mininode_lock twice in wait_for_broadcast() (John Newbery)
edae6075aa3b1169c84b65e76fd48d68242a294e [tests] Only acquire lock once in p2p_compactblocks.py (John Newbery)
Pull request description:
There's no need for mininode_lock to be reentrant.
Use a simpler non-recursive lock.
ACKs for top commit:
MarcoFalke:
ACK 62068381a3b9c065d81300be79abba7aecfdb41b 😃
jonatack:
ACK 62068381a3b9c0
Tree-SHA512: dcbc19e6c986970051705789be0ff7bec70c69cf76d5b468c2ba4cb732883ad512b1de5c3206c2eca41fa3f1c4806999df4cabbf67fc3c463bb817458e59a19c
3a10d935ac8ebabdfd336569d943f042ff84b13e [p2p/refactor] move disconnect logic and remove misbehaving (gzhao408)
ff8c430c6589ea72b9e169455cf6437c8623cc52 [test] test disconnect for filterclear (gzhao408)
1c6b787e0319c44f0e0bede3f4a77ac7c2089db2 [netprocessing] disconnect node that sends filterclear (gzhao408)
Pull request description:
Nodes that don't have bloomfilters turned on (i.e. no `NODE_BLOOM` service) should disconnect peers that send them `filterclear` P2P messages.
Non-bloomfilter nodes already disconnect peers for [`filteradd` and `filterload`](19e919217e/src/net_processing.cpp (L2218)), but #8709 removed `filterclear` so it could be used to reset tx relay. This isn't needed now because using `feefilter` message is much better for this purpose (See #19204).
Also refactors existing disconnect logic for `filteradd` and `filterload` into respective message handlers and removes banning for them.
ACKs for top commit:
jnewbery:
Code review ACK 3a10d935ac8ebabdfd336569d943f042ff84b13e
naumenkogs:
utACK 3a10d93
gillichu:
tested ACK: quick test_runner on macOS [`3a10d93`](3a10d935ac)
MarcoFalke:
re-ACK 3a10d935ac only change is replacing false with true 🚝
Tree-SHA512: 7aad8b3c0b0e776a47ad52544f0c1250feb242320f9a2962542f5905042f77e297a1486f8cdc3bf0fb93cd00c1ab66a67b2ec426eb6da3fe4cda56b5e623620f
In Python integers should be compared for equality (`i == j`), not identity (`i is j`). Recent versions of CPython 3.x emit a SyntaxWarning when they encounter this incorrect usage, e.g.
```
$ python3 base58.py
base58.py:110: SyntaxWarning: "is" with a literal. Did you mean "=="?
assert get_bcaddress_version('15VjRaDX9zpbA8LVnbrCAFzrVzN7ixHNsC') is 0
Tests passed
```
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.
fa7166759789c1282609ff3ab2e80d4f70910a9f ci: Move travis workarounds to .travis.yml (MarcoFalke)
Pull request description:
It seems odd to have travis related workarounds in the general ci config files. Fix that oddity by moving the travis related workarounds to the travis yaml file.
For unexplained reasons, this should also work around and thus close#19171
ACKs for top commit:
hebasto:
ACK fa7166759789c1282609ff3ab2e80d4f70910a9f, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: b4419d38e2b41f6e4d6e6b7658f1d972c40c390a49fe78808f8640d28efd84cc6668ce292d45b7c539e65b9e2ecbad10e796cb8f9329a0f1e7d0132ce962d226
0f8f51544558d204a4e9d0607b0f375150529637 RPC: Rephrase generatetoaddress help, and use PACKAGE_NAME (Luke Dashjr)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 357c6e0bd1b144213ca6cf0bfd649c7a482c2d6d5e98a254d20c8365d228dc71ae1b78aca4918fdbf065f8894ef82f8a475902d605204275bb99fe77d4b42fae
-Increasing the banscore and/or banning is too harsh,
just disconnecting is enough.
-Return true from ProcessMessage because we already log
receipt of filterclear and disconnect.
-nodes not serving bloomfilters should disconnect peers
that send filterclear, just like filteradd and filterload
-nodes that want to enable/disable txrelay should use
feefilter
49236be099c5e8b3cadbc98d5216313e7e1a5a45 [tests] Don't import asyncio to test magic bytes (John Newbery)
Pull request description:
Simplify the test for invalid start bytes. No need to import asyncio and the Network thread.
ACKs for top commit:
MarcoFalke:
review ACK 49236be099c5e8b3cadbc98d5216313e7e1a5a45
jonatack:
ACK 49236be099c5e8b3cadbc98d5216313e7e1a5a45
troygiorshev:
ACK 49236be. +0.1 on the additional `cut_len` reformat.
Tree-SHA512: 75cb695603cdc1be7035d7b5117dbef2a1fdb29fd4414a73d75b53d563d6fa800c31bfa9475004622c8bdea4978b51b2055d6fa7be0fe47c7ae34ccc2b0e89a0
ccf1f6ea24905876f35e685204cb2293cf083e97 refactor: Drop ::HasWallets() (João Barbosa)
Pull request description:
Minor follow-up of #19250. The global `HasWallets()` is used only once and at the call site there's already a way to know if any wallet is loaded.
ACKs for top commit:
MarcoFalke:
ACK ccf1f6ea24905876f35e685204cb2293cf083e97
hebasto:
ACK ccf1f6ea24905876f35e685204cb2293cf083e97, I have reviewed the changes and they look OK, I agree they can be merged.
Tree-SHA512: fb902c045cbd331eaf71716c04734520f2ce7f2b317db510c4ce140162bbc683327b5a40ac860f6cde5add37e069065274d39dfa147fac2091eedec505f2f7eb
8a26848c460160e1279f26bc413f693a34e33b9d build: Fix m4 escaping (Hennadii Stepanov)
9123ec15db104397998f5084afc69403d2f9e4b8 build: Remove extra tokens warning (Hennadii Stepanov)
fded4f48c33742d7c790335c8de59c15b80d94e6 build: Remove duplicated QT_STATICPLUGIN define (Hennadii Stepanov)
05a93d5d96101b45d87571af5b772c7a1e82fd27 build: Fix indentation in bitcoin_qt.m4 (Hennadii Stepanov)
ddbb41931019ed4226af3df37874c7eb7cf570f1 build: Use pkg-config in BITCOIN_QT_CONFIGURE for all hosts (Hennadii Stepanov)
492971de35bab26346545f68365872211f458b00 build: Fix mingw pkgconfig file and dependency naming (Hennadii Stepanov)
Pull request description:
This PR makes `bitcoin_qt.m4` to use `pkg-config` for all hosts and removes non-pkg-config paths from it. This is a step towards the idea which was clear [stated](https://github.com/bitcoin/bitcoin/pull/8314#issue-76644643) by Cory Fields:
> I believe the consensus is to treat Windows like the others and require pkg-config across the board. We can drop all of the non-pkg-config paths, and simply AC_REQUIRE(PKG_PROG_PKG_CONFIG)
There are two unsolved problems with this PR. If depends is built with `DEBUG=1` the `configure` script fails to pickup Qt:
- for macOS host (similar to, but not the same as #16391)
- for Windows host (regression)
The fix is ~on its way~ submitted in #18298 (as a followup).
Also this PR picks some small improvements from #17820.
ACKs for top commit:
theuni:
Code review ACK 8a26848c460160e1279f26bc413f693a34e33b9d
dongcarl:
Code Review ACK 8a26848c460160e1279f26bc413f693a34e33b9d
laanwj:
Code review ACK 8a26848c460160e1279f26bc413f693a34e33b9d
Tree-SHA512: 3b25990934b939121983df7707997b31d61063b1207d909f539d69494c7cb85212f353092956d09ecffebb9fef28b869914dd1216a596d102fcb9744bb5487f7
af2a145e575f23c64909e6cf1fb323c603bda7ca Refactor resource exhaustion test (Troy Giorshev)
5c4648d17ba18e4194959963994cc6b37053f127 Fix "invalid message size" test (Troy Giorshev)
ff1e7b884447a5ba10553b2d964625f94e255bdc Move size limits to module-global (Troy Giorshev)
57890abf2c7919eddfec36178b1136cd44ffe883 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 af2a145e575f23c64909e6cf1fb323c603bda7ca
MarcoFalke:
re-ACK af2a145e57 🍦
Tree-SHA512: 9b57561e142c5eaefac5665f7355c8651670400b4db1a89525d2dfdd20e872d6873c4f6175c4222b6f5a8e5210cf5d6a52da69b925b673a2e2ac30a15d670d1c
45eff751c6d07007dabc365dc4c0e6c63e3fe5cf 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 45eff751c6d07007dabc365dc4c0e6c63e3fe5cf
naumenkogs:
Tested ACK 45eff75
fjahr:
re-ACK 45eff751c6d07007dabc365dc4c0e6c63e3fe5cf
andrewtoth:
re-ACK 45eff751c6d07007dabc365dc4c0e6c63e3fe5cf
Tree-SHA512: 177208ab6f30dc62da1cc5f51e654f7c9770d8c6b42aca6ae7ecb30e29d3096e04d75739578e7d149a0f29dd92652b4a707e93c0f1be8aa7ed315e6ec3ab07a4
fadf6bd04f002d05aaff8eba74015e25a41966bc refactor: Remove unused request.fHelp (MarcoFalke)
fad889cbf0b6c46da2e110b73cbea55e4ff7951e 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 fadf6bd04f002d05aaff8eba74015e25a41966bc
promag:
Tested ACK fadf6bd04f002d05aaff8eba74015e25a41966bc.
Tree-SHA512: 65d4ff400467f57cb8415c30ce30f814dc76c5c157308b7a7409c59ac9db629e65dfba31cd9c389cfe60a008d3d87787ea0a0e0f2671fd65fd190543c915493d
