5730a43703 test: Add functional test for AddrFetch connections (Martin Zumsande)
c34ad3309f net, rpc: Enable AddrFetch connections for functional testing (Martin Zumsande)
533500d907 p2p: Add timeout for AddrFetch peers (Martin Zumsande)
b6c5d1e450 p2p: AddrFetch - don't disconnect on self-announcements (Martin Zumsande)
Pull request description:
AddrFetch connections (old name: oneshots) are intended to be short-lived connections on which we ask a peer for addresses via `getaddr` and disconnect after receiving them.
This is done by disconnecting after receiving the first `addr`. However, it is no longer working as intended, because nowadays, the first `addr` a typical bitcoin core node sends is its self-announcement.
So we'll disconnect before the peer gets a chance to answer our `getaddr`.
I checked that this affects both `-seednode` peers specified manually, and DNS seeds when AddrFetch is used as a fallback if DNS doesn't work for us.
The current behavior of getting peers via AddrFetch when starting with an empty addrman would be to connect to the peer, receive its self-announcement and add it to addrman, disconnect, reconnect to the same peer again as a full outbound (no other addresses in addrman) and then receive more `addr`. This is silly and not in line with AddrFetch peer being intended to be short-lived peers.
Fix this by only disconnecting after receiving an `addr` message of size > 1.
[Edit] As per review discussion, this PR now also adds a timeout after which we disconnect if we haven't received any suitable `addr`, and a functional test.
ACKs for top commit:
amitiuttarwar:
reACK 5730a43703
naumenkogs:
ACK 5730a43703
jnewbery:
ACK 5730a43703
Tree-SHA512: 8a81234f37e827705138eb254223f7f3b3bf44a06cb02126fc7990b0d231b9bd8f07d38d185cc30d55bf35548a6fdc286b69602498d875b937e7c58332158bf9
ce6bca88e8 doc: release note for getnodeaddresses by network (Jon Atack)
3f89c0e990 test: improve getnodeaddresses coverage, test by network (Jon Atack)
6c98c09991 rpc: enable filtering getnodeaddresses by network (Jon Atack)
80ba294854 p2p: allow CConnman::GetAddresses() by network, add doxygen (Jon Atack)
a49f3ddbba p2p: allow CAddrMan::GetAddr() by network, add doxygen (Jon Atack)
c38981e748 p2p: pull time call out of loop in CAddrMan::GetAddr_() (João Barbosa)
d35ddca91e p2p: enable CAddrMan::GetAddr_() by network, add doxygen (Jon Atack)
Pull request description:
This patch allows passing a network argument to CAddrMan::GetAddr(), CConnman::GetAddresses(), and rpc getnodeaddresses to return only addresses of that network.
It also contains a performance optimisation by promag.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK ce6bca88e8
vasild:
ACK ce6bca88e8
Tree-SHA512: 40e700d97091248429c73cbc0639a1f03ab7288e636a7b9026ad253e9708253c6b2ec98e7d9fb2d56136c0f762313dd648915ac98d723ee330d713813a43f99d
6e2eb0d63b rpc/wallet: use OMITTED_NAMED_ARG instead of Default(VNULL) (Karl-Johan Alm)
4983f4cba4 rpc/createwallet: omitted named arguments (Karl-Johan Alm)
dc4db23b30 rpc: address:amount dictionaries are OBJ_USER_KEYS (Karl-Johan Alm)
c8cf0a3d51 rpc/getpeerinfo: bytesrecv_per_msg is a dynamic dictionary (Karl-Johan Alm)
eb4fb7e507 rpc/gettxoutsetinfo: hash_or_height is a named argument (Karl-Johan Alm)
Pull request description:
This is a follow-up to #21897, and I believe covers the remaining cases, at least that I could find.
Edited to remove unrelated information about a side project.
ACKs for top commit:
laanwj:
Documentation diff ACK 6e2eb0d63b
promag:
Code review ACK 6e2eb0d63b.
Tree-SHA512: d26f6e074e13d64bbca2a114a0adc7f905d47d238c4e9bc49f70ca0b775afbebf9879fc3794ab29dc316a6dbd00ba8cbeb01197e236ee4ab2e9854db25f23f04
fae196147b doc: Clarify that feerates are per virtual size (MarcoFalke)
fa83e95ac6 scripted-diff: Clarify that feerates are per virtual size (MarcoFalke)
Pull request description:
By implementing segwit, it is already clear that all feerates in Bitcoin Core are denoted in (amount/virtual size). Though, there is inconsistency, as some places use kvB, some use kB. Thus, replace all with "kvB".
See also commit 6da3afbaee, which did the replacement for wallet RPCs.
ACKs for top commit:
ryanofsky:
Code review ACK fae196147b. Checked instances where units were being added in the second commit and they all looked right.
Tree-SHA512: ab70d13cde7d55c1ac931bddc2b45aa218fc75ef46cb6ea9e5a30b1d4dbf27889c2b6357299a6c5427912443a46ec3592a4809dae335e03162bd2120a0f7f8ad
b4fcbcfb49 doc: update -maxconnections config option help (Jon Atack)
79685a8992 doc: update -addnode config option help (Jon Atack)
2896c6c4cc doc: update addnode rpc help (Jon Atack)
Pull request description:
Since #9319 proposed by Gregory Maxwell and released in v0.14, peers manually added through the `-addnode` config option or using the `addnode` RPC have their own separate limit of 8 connections that does not compete with other inbound or outbound connection usage and is not subject to the limitation imposed by the `-maxconnections` option.
This PR updates the `-addnode` and `-maxconnections` config options and the `addnode` RPC help docs with this information.
`-addnode` config option help
```
$ bitcoind -h | grep -A5 addnode=
-addnode=<ip>
Add a node to connect to and attempt to keep the connection open (see
the addnode RPC help for more info). This option can be specified
multiple times to add multiple nodes; connections are limited to
8 at a time and are counted separately from the -maxconnections
limit.
$ bitcoind -h | grep -A3 maxconnections=
-maxconnections=<n>
Maintain at most <n> connections to peers (default: 125). This limit
does not apply to connections manually added via -addnode or the
addnode RPC, which have a separate limit of 8.
```
`addnode` rpc help
```
$ bitcoin-cli help addnode
addnode "node" "command"
Attempts to add or remove a node from the addnode list.
Or try a connection to a node once.
Nodes added using addnode (or -connect) are protected from DoS disconnection and are not required to be
full nodes/support SegWit as other outbound peers are (though such peers will not be synced from).
Addnode connections are limited to 8 at a time and are counted separately from the -maxconnections limit.
```
ACKs for top commit:
prayank23:
ACK b4fcbcfb49
jarolrod:
ACK b4fcbcfb49
Tree-SHA512: b6d69baa6cbf6d53f91bac5b39b549d49db6c95f92ea1bdd3588a6432794a25ac2c8b3c89e2c72bb9097e61f2717c8b5ecc404745d5992b88e523db03200898f
fafb68add5 refactor: Add and use EnsureConnman in rpc code (MarcoFalke)
faabeb854a refactor: Mark member functions const (MarcoFalke)
Pull request description:
This removes the 10 occurrences of `throw JSONRPCError(RPC_CLIENT_P2P_DISABLED, "Error: Peer-to-peer functionality missing or disabled");` and replaces them with `EnsureConnman`.
ACKs for top commit:
jarolrod:
re-ACK fafb68add5
theStack:
ACK fafb68add5
ryanofsky:
Code review ACK fafb68add5
Tree-SHA512: 84c63cfe31e548645d906f7191a3526c7bea99ed0d54c2a75c2041452a44fe149ede343d8e1943b0e7770816c828bb047dfec8bc541a1f2b89920a126ee54d68
bee56c78e9 rpc: Check default value type againts argument type (João Barbosa)
f81ef4303e rpc: Keep default argument value in correct type (João Barbosa)
Pull request description:
Store default values of RPC arguments in the corresponding type instead of a string. The value is then serialized when the help output is needed. This change simplifies #20017.
The following examples illustrates how to use the new `RPCArg::Default` and `RPCArg::DefaultHint`:
```diff
- {"verbose", RPCArg::Type::BOOL, /* default */ "false", "True for a json object, false for array of transaction ids"}
+ {"verbose", RPCArg::Type::BOOL, RPCArg::Default(false), "True for a json object, false for array of transaction ids"}
```
```diff
- {"nblocks", RPCArg::Type::NUM, /* default */ "one month", "Size of the window in number of blocks"}
+ {"nblocks", RPCArg::Type::NUM, RPCArg::DefaultHint("one month"), "Size of the window in number of blocks"}
```
No behavior change is expected.
ACKs for top commit:
LarryRuane:
ACK bee56c78e9
MarcoFalke:
ACK bee56c78e9🦅
Tree-SHA512: c47d78c918e996d36631d4ad3c933b270a34c5b446b8d736be94cf4a0a7b8c0e33d954149ec786cf9550639865b79deb6a130ad044de6030f95aac33f524293a
586190f0b4 rpc/rest: Take and reuse local Chain/ChainState obj (Carl Dong)
bc3bd36902 rpc: style: Improve BuriedForkDescPushBack signature (Carl Dong)
f99913969f rpc: Remove unnecessary casting of block height (Carl Dong)
6a3d192020 rpc: Tidy up local references (see commit message) (Carl Dong)
038854f31e rest/rpc: Remove now-unused old Ensure functions (Carl Dong)
6fb65b49f4 scripted-diff: rest/rpc: Use renamed EnsureAny*() (Carl Dong)
1570c7ee98 rpc: Add renamed EnsureAny*() functions (Carl Dong)
306b1cd3ee rpc: Add alt Ensure* functions acepting NodeContext (Carl Dong)
d7824acdb9 rest: Use existing NodeContext (Carl Dong)
3f08934799 rest: Pass in NodeContext to rest_block (Carl Dong)
7be0671b95 rpc/rawtx: Use existing NodeContext (Carl Dong)
60dc05afc6 rpc/mining: Use existing NodeContext (Carl Dong)
d485e815e2 rpc/blockchain: Use existing NodeContext (Carl Dong)
d0abf0bf42 rpc/*,rest: Add review-only assertion to EnsureChainman (Carl Dong)
cced0f46c9 miner: Pass in previous CBlockIndex to RegenerateCommitments (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Based on:
- [x] #21270 | [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules
- [x] #21525 | [Bundle 4.5/n] Followup fixups to bundle 4
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
ryanofsky:
Code review ACK 586190f0b4. Since last review, no changes to existing commits, just some simple new commits added: three new commits renaming std::any Ensure functions (scripted diff commit and manual pre/post commits), and one new commit factoring out a repeated `ActiveChain()` call made in a loop. Thanks for the updates!
jnewbery:
utACK 586190f0b4
MarcoFalke:
review ACK 586190f0b4🍯
Tree-SHA512: 64b677fb50141805b55c3f1afe68fcd298f9a071a359bdcd63256d52e334f83e462f31fb3ebee9b630da8f1d912a03a128cfc38179e7aaec29a055744a98478c
d3b0b08b0f doc: release notes for new listbanned fields (Jarol Rodriguez)
60290d3f5e test: increase listbanned unit test coverage (Jon Atack)
3e978d1a5d rpc: add time_remaining field to listbanned (Jarol Rodriguez)
5456b34531 rpc: add ban_duration field to listbanned (Jarol Rodriguez)
c95c61657a doc: improve listbanned help (Jarol Rodriguez)
dd3c8eaa33 rpc: swap position of banned_until and ban_created fields (Jarol Rodriguez)
Pull request description:
This PR adds a `ban_duration` and `time_remaining` field to the `listbanned` RPC command. Thanks to jonatack, this PR also expands the `listbanned` test coverage to include these new fields
It's useful to keep track of `ban_duration` as this is another data point on which to sort banned peers. I found this helpful in adding additional context columns to the GUI `bantablemodel` as part of a follow-up PR. As [suggested](https://github.com/bitcoin/bitcoin/pull/21602#issuecomment-813486134) by jonatack, `time_remaining` is another useful user-centric data point.
Since a ban always expires after its created, the `ban_created` field is now placed before the `banned_until` field. This new ordering is more logical.
This PR also improves the `help listbanned` output by providing additional context to the descriptions of the `address`, `ban_created`, and `banned_until` fields.
**Master: listbanned**
```
[
{
"address": "1.2.3.4/32",
"banned_until": 1617691101,
"ban_created": 1617604701
},
{
"address": "135.181.41.129/32",
"banned_until": 1649140716,
"ban_created": 1617604716
}
]
```
**PR: listbanned**
```
[
{
"address": "1.2.3.4/32",
"ban_created": 1617775773,
"banned_until": 1617862173,
"ban_duration": 86400,
"time_remaining": 86392
},
{
"address": "3.114.211.172/32",
"ban_created": 1617753165,
"banned_until": 1618357965,
"ban_duration": 604800,
"time_remaining": 582184
}
]
```
ACKs for top commit:
jonatack:
re-ACK d3b0b08b0f
hebasto:
ACK d3b0b08b0f, tested on Linux Mint 20.1 (x86_64).
MarcoFalke:
review ACK d3b0b08b0f🕙
Tree-SHA512: 5b83ed2483344e546d57e43adc8a1ed7a1fff292124b14c86ca3a1aa2aec8b0f7198212fabff2c5145e7f726ca04ae567fe667b141254c7519df290cf63774e5
Introduce two new options to reach the I2P network:
* `-i2psam=<ip:port>` point to the I2P SAM proxy. If this is set then
the I2P network is considered reachable and we can make outgoing
connections to I2P peers via that proxy. We listen for and accept
incoming connections from I2P peers if the below is set in addition to
`-i2psam=<ip:port>`
* `-i2pacceptincoming` if this is set together with `-i2psam=<ip:port>`
then we accept incoming I2P connections via the I2P SAM proxy.
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
Add a new RPC endpoint to enable opening outbound connections from
the tests. The functional test framework currently uses the addnode RPC, which
has different behavior than general outbound peers. These changes enable
creating both full-relay and block-relay-only connections. The new RPC
endpoint calls through to a newly introduced AddConnection method on
CConnman that ensures we stay within the allocated max.
