ceec6808d331fa082407a734cd5f3c2f1c7d11b3 test: `-whitebind` and `-bind` with `-listen=0` should throw an error (brunoerg)
Pull request description:
This PR adds test coverage for the following init error:
b9122e95f0/src/init.cpp (L872-L875)
ACKs for top commit:
laanwj:
Code review ACK ceec6808d331fa082407a734cd5f3c2f1c7d11b3
Tree-SHA512: 03068abe7199b1235f029871ab87a3dd4943738c592ad62d82cdcd3e0201e627624960bd3ea1fc6fc1e7da4b8e215ba3393d1cb8130e1108049f764e51dc75c0
d841301010914203fb5ef02627c76fad99cb11f1 test: Add docstring to wait_until() in util.py to warn about its usage (Seleme Topuz)
1343c86c7cc1fc896696b3ed87c12039e4ef3a0c test: Update wait_until usage in tests not to use the one from utils (Seleme Topuz)
Pull request description:
Replace global (from [test_framework/util.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test_framework/util.py#L228)) `wait_until()` usages with the ones provided by `BitcoinTestFramework` and `P2PInterface` classes.
The motivation behind this change is that the `util.wait_until()` expects a timeout, timeout_factor and lock and it is not aware of the context of the test framework. `BitcoinTestFramework` offers a `wait_until()` which has an understandable amount of default `timeout` and a shared `timeout_factor`. Moreover, on top of these, `mininode.wait_until()` also has a shared lock.
closes#19080
ACKs for top commit:
MarcoFalke:
ACK d841301010914203fb5ef02627c76fad99cb11f1 🦆
kallewoof:
utACK d841301010914203fb5ef02627c76fad99cb11f1
Tree-SHA512: 81604f4cfa87fed98071a80e4afe940b3897fe65cf680a69619a93e97d45f25b313c12227de7040e19517fa9c003291b232f1b40b2567aba0148f22c23c47a88
d5800da5199527a366024bc80cad7fcca17d5c4a [test] Remove final references to mininode (John Newbery)
5e8df3312e47a73e747ee892face55ed9ababeea test: resort imports (John Newbery)
85165d4332b0f72d30e0c584b476249b542338e6 scripted-diff: Rename mininode to p2p (John Newbery)
9e2897d020b114a10c860f90c5405be029afddba scripted-diff: Rename mininode_lock to p2p_lock (John Newbery)
Pull request description:
New contributors are often confused by the terminology in the test framework, and what the difference between a _node_ and a _peer_ is. To summarize:
- a 'node' is a bitcoind instance. This is the thing whose behavior is being tested. Each bitcoind node is managed by a python `TestNode` object which is used to start/stop the node, manage the node's data directory, read state about the node (eg process status, log file), and interact with the node over different interfaces.
- one of the interfaces that we can use to interact with the node is the p2p interface. Each connection to a node using this interface is managed by a python `P2PInterface` or derived object (which is owned by the `TestNode` object). We can open zero, one or many p2p connections to each bitcoind node. The node sees these connections as 'peers'.
For historic reasons, the word 'mininode' has been used to refer to those p2p interface objects that we use to connect to the bitcoind node (the code was originally taken from the 'mini-node' branch of https://github.com/jgarzik/pynode/tree/mini-node). However that name has proved to be confusing for new contributors, so rename the remaining references.
ACKs for top commit:
amitiuttarwar:
ACK d5800da519
MarcoFalke:
ACK d5800da5199527a366024bc80cad7fcca17d5c4a 🚞
Tree-SHA512: 2c46c2ac3c4278b6e3c647cfd8108428a41e80788fc4f0e386e5b0c47675bc687d94779496c09a3e5ea1319617295be10c422adeeff2d2bd68378e00e0eeb5de
fa4632c41714dfaa699bacc6a947d72668a4deef test: Move boost/stdlib includes last (MarcoFalke)
fa488f131fd4f5bab0d01376c5a5013306f1abcd scripted-diff: Bump copyright headers (MarcoFalke)
fac5c373006a9e4bcbb56843bb85f1aca4d87599 scripted-diff: Sort test includes (MarcoFalke)
Pull request description:
When writing tests, often includes need to be added or removed. Currently the list of includes is not sorted, so developers that write tests and have `clang-format` installed will either have an unrelated change (sorting) included in their commit or they will have to manually undo the sort.
This pull preempts both issues by just sorting all includes in one commit.
Please be aware that this is **NOT** a change to policy to enforce clang-format or any other developer guideline or process. Developers are free to use whatever tool they want, see also #18651.
Edit: Also includes a commit to bump the copyright headers, so that the touched files don't need to be touched again for that.
ACKs for top commit:
practicalswift:
ACK fa4632c41714dfaa699bacc6a947d72668a4deef
jonatack:
ACK fa4632c41714dfaa, light review and sanity checks with gcc build and clang fuzz build
Tree-SHA512: 130a8d073a379ba556b1e64104d37c46b671425c0aef0ed725fd60156a95e8dc83fb6f0b5330b2f8152cf5daaf3983b4aca5e75812598f2626c39fd12b88b180
fab558612278909df93bdf88f5727b04f13aef0f doc: Use precise permission flags where possible (MarcoFalke)
Pull request description:
Instead of mentioning the all-encompassing `-whitelist*` settings, change the docs to mention the exact permission flag that will influence the behaviour.
This is needed because in the future, the too-broad `-whitelist*` settings (they either include *all* permission flags or apply to *all* peers) might be deprecated to require the permission flags to be enumerated.
Alternatively, in the future there could be an RPC to set the net permission flags on an existing connection, in which case the `-whitelist*` terminology is of no help.
ACKs for top commit:
jnewbery:
reACK fab558612278909df93bdf88f5727b04f13aef0f
fjahr:
Code review ACK fab558612278909df93bdf88f5727b04f13aef0f
jonatack:
ACK fab558612278909df93bdf88f5727b04f13aef0f
Tree-SHA512: c7dea3e577d90103bb2b0ffab7b7c8640b388932a3a880f69e2b70747fc9213dc1f437085671fd54c902ec2a578458b8a2fae6dbe076642fb88efbf9fa9e679c
facb71576cd4d2e90fd03e09d29b42fa3d730e8c net: Remove forcerelay of rejected txs (MarcoFalke)
Pull request description:
This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:
* While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See 4a07233076/src/net_processing.cpp (L3862-L3866)
* Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)
The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574
This feature was (intentionally or accidentally) removed in 4d8993b346, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.
ACKs for top commit:
hebasto:
ACK facb71576cd4d2e90fd03e09d29b42fa3d730e8c, locally running the unit and functional tests.
Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5
aaaae4d0ebd5ef34d81997a73ab9839ba7b4b9e4 test: Add p2p test for forcerelay permission (MarcoFalke)
fa6b57bcaaf4dc65d78316353033b03d171a3beb test: Fix whitespace in p2p_permissions.py (MarcoFalke)
faf40810d7b7f42f3588bfa8a663095aa24001b1 test: Make msg_tx a witness tx (MarcoFalke)
Pull request description:
The commit `test: Make msg_tx a witness tx` is needed so that the python mininode does not strip the witness from transactions before sending them over p2p. The commit should also be done to keep symmetry with msg_block. See:
* tests: Make msg_block a witness block #15982
ACKs for top commit:
laanwj:
ACK aaaae4d0ebd5ef34d81997a73ab9839ba7b4b9e4
Tree-SHA512: b4b546c88f7f0576cb512f0872bc6bef9d4df65783803f226986e56175937f418aa1ed906417ac909f27f1fd521d64629621fda83250fa925c46ef9513db0e4c
3b05f0f70fbaee5b5eaa0d1b6f3b9d32f44410bb Reformat p2p_permissions.py (nicolas.dorier)
ce7eac3cb0e7d301db75de24e9a7b0af93c61311 [Fix] The default whitelistrelay should be true (nicolas.dorier)
Pull request description:
I thought `whitelistrelay` default was `false` when it is `true`.
The root of the issue come from the fact that all references to `DEFAULT_` are not in the scope of this file, so hard coding of default values are used everywhere in `net.cpp`. I think that in a separate PR we should fix that more fundamentally everywhere.
ACKs for top commit:
promag:
ACK 3b05f0f70fbaee5b5eaa0d1b6f3b9d32f44410bb.
Sjors:
re-ACK 3b05f0f70fbaee5b5eaa0d1b6f3b9d32f44410bb
Tree-SHA512: f4a75f986fa2adf1a5f1c91605e0d261f7ac5ac8535fb05437d83b8392dbcf5cc1a47d755adcf8ad8dc67a88de28060187200fd3ce06545261a5c7ec0fea831a
c5b404e8f1973afe071a07c63ba1038eefe13f0f Add functional tests for flexible whitebind/list (nicolas.dorier)
d541fa391844f658bd7035659b5b16695733dd56 Replace the use of fWhitelisted by permission checks (nicolas.dorier)
ecd5cf7ea4c3644a30092100ffc399e30e193275 Do not disconnect peer for asking mempool if it has NO_BAN permission (nicolas.dorier)
e5b26deaaa6842f7dd7c4537ede000f965ea0189 Make whitebind/whitelist permissions more flexible (nicolas.dorier)
Pull request description:
# Motivation
In 0.19, bloom filter will be disabled by default. I tried to make [a PR](https://github.com/bitcoin/bitcoin/pull/16176) to enable bloom filter for whitelisted peers regardless of `-peerbloomfilters`.
Bloom filter have non existent privacy and server can omit filter's matches. However, both problems are completely irrelevant when you connect to your own node. If you connect to your own node, bloom filters are the most bandwidth efficient way to synchronize your light client without the need of some middleware like Electrum.
It is also a superior alternative to BIP157 as it does not require to maintain an additional index and it would work well on pruned nodes.
When I attempted to allow bloom filters for whitelisted peer, my proposal has been NACKed in favor of [a more flexible approach](https://github.com/bitcoin/bitcoin/pull/16176#issuecomment-500762907) which should allow node operator to set fine grained permissions instead of a global `whitelisted` attribute.
Doing so will also make follow up idea very easy to implement in a backward compatible way.
# Implementation details
The PR propose a new format for `--white{list,bind}`. I added a way to specify permissions granted to inbound connection matching `white{list,bind}`.
The following permissions exists:
* ForceRelay
* Relay
* NoBan
* BloomFilter
* Mempool
Example:
* `-whitelist=bloomfilter@127.0.0.1/32`.
* `-whitebind=bloomfilter,relay,noban@127.0.0.1:10020`.
If no permissions are specified, `NoBan | Mempool` is assumed. (making this PR backward compatible)
When we receive an inbound connection, we calculate the effective permissions for this peer by fetching the permissions granted from `whitelist` and add to it the permissions granted from `whitebind`.
To keep backward compatibility, if no permissions are specified in `white{list,bind}` (e.g. `--whitelist=127.0.0.1`) then parameters `-whitelistforcerelay` and `-whiterelay` will add the permissions `ForceRelay` and `Relay` to the inbound node.
`-whitelistforcerelay` and `-whiterelay` are ignored if the permissions flags are explicitly set in `white{bind,list}`.
# Follow up idea
Based on this PR, other changes become quite easy to code in a trivially review-able, backward compatible way:
* Changing `connect` at rpc and config file level to understand the permissions flags.
* Changing the permissions of a peer at RPC level.
ACKs for top commit:
laanwj:
re-ACK c5b404e8f1973afe071a07c63ba1038eefe13f0f
Tree-SHA512: adfefb373d09e68cae401247c8fc64034e305694cdef104bdcdacb9f1704277bd53b18f52a2427a5cffdbc77bda410d221aed252bc2ece698ffbb9cf1b830577
