| Age | Commit message (Collapse) | Author |
|---|
|
The addpeeraddress calls can fail due to collisions. As we are using a
deteministic addrman, they won't fail with the current bucket/position
calculation. However, if the calculation is changed, they might collide
and fail silently causing tests using `seed_addrman()` to fail.
Assert that the addpeeraddress calls are successful.
|
|
current check to make sure that detailed help for hidden RPC
is displayed won't work because the assertion isn't sufficient.
Even if unknown RPCs are passed, RPC names would still be present
in node.help().
|
|
Drops the mocktime added in fa4c6836c9366c3cc575cb386a397840d5f1aa57.
Setting the mocktime in test_addpeeraddress() isn't needed
anymore as it doesn't leak into test_getrawaddrman() anymore
(since 2cc8ca19f4185490f30a49516c890b2289fbab71).
test_getrawaddrman() clear's the addrman and sets it's own
mocktime.
|
|
When trying to add an address to the IP address manager tried table,
it's first added to the new table and then moved to the tried table.
Previously, adding a conflicting address to the address manager's
tried table with test-only `addpeeraddress tried=true` RPC would
return `{ "success": true }`. However, the address would not be added
to the tried table, but would remain in the new table. This caused,
e.g., issue 28964.
This is fixed by returning `{ "success": false, "error":
"failed-adding-to-tried" }` for failed tried table additions. Since
the address remaining in the new table can't be removed (the address
manager interface does not support removing addresses at the moment
and adding this seems to be a bigger effort), an error message is
returned. This indicates to a user why the RPC failed and allows
accounting for the extra address in the new table.
Also:
To check the number of addresses in each addrman table,
the addrman checks were re-run and the log output of this check
was asserted. Ideally, logs shouldn't be used as an interface
in automated tests. To avoid asserting the logs, use the getaddrmaninfo
and getrawaddrman RPCs (which weren't implemented when the test was added).
Removing the "getnodeaddress" calls would also remove the addrman checks
from the test, which could reduce the test coverage. To avoid this,
these are kept.
|
|
The nKey of the addrman is generated the first time the node is
started. Therefore, restarting a node or turning it off and on
again won't make a previously non-deterministic addrman
deterministic.
Co-authored-by: 0xb10c <b10c@b10c.me>
|
|
functional tests
2cc8ca19f4185490f30a49516c890b2289fbab71 [test] Use deterministic addrman in addrman info tests (stratospher)
a8978661093500df8d8dcf2a9c90837afacd0aab [test] Restart a node with empty addrman (stratospher)
71c19915c0c716d6f8a539dd92b8ad41e8c447ee [test] Use deterministic addrman in addpeeraddress test (stratospher)
7b868e6b678502e86571976d696c0e3cb72c0884 Revert "test: avoid non-determinism in asmap-addrman test" (stratospher)
69e091f3e1f65b12cf1804e7d39651f55a01827a [init] Create deterministic addrman in tests using -test=addrman (stratospher)
be25ac3092b7755e26e1ec6c33a27cd0e3dd9eac [init] Remove -addrmantest command line arg (stratospher)
802e6e128bba5ffa6d4ec53ff45acccb7cb28f21 [init] Add new command line arg for use only in functional tests (stratospher)
Pull request description:
An address is placed in a `[bucket,position]` in the addrman table (new table or tried table) using the `addpeeraddress` RPC. This `[bucket,position]` is calculated using `nKey`(and other metrics) for the addrman which is chosen randomly during every run.
Supposing there are 2 addresses to be placed in an addrman table. During every test run, a different `[bucket,position]` would be calculated for each address.These calculated `[bucket,position]` could even be the same for the 2 addresses in some test runs and result in collisions in the addrman. We wouldn't be able to predict when the collisions are going to happen because we can't predict the `nKey` value which is chosen at random. This can cause flaky tests.
Because of these non deterministic collisions, we are limited in what we can do to test addrman functionality. Currently in our tests don't add a second address to prevent these collisions from happening - we only keep 1 address in the new table and 1 address in the tried table. See https://github.com/bitcoin/bitcoin/pull/26988#discussion_r1091145647, https://github.com/bitcoin/bitcoin/pull/23084, [#22831(comment)](https://github.com/bitcoin/bitcoin/pull/22831/files#r708302639).
This PR lets us create a deterministic addrman with fixed `nKey` so that we can know the `[bucket,position]` collisions beforehand, safely add more addresses in an addrman table and write more extensive tests.
ACKs for top commit:
amitiuttarwar:
ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
achow101:
ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
0xB10C:
ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
mzumsande:
Code Review ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
Tree-SHA512: 8acd9bdfe7de1eb44d22373bf13533d8ecf602df966fdd5b8b78afcd8cc35a286c95d2712f67a89473a0d68dded7d38f5599f6e4bf95a6589475444545bfb189
|
|
Make sure that v2 handshake is complete before comparing getpeerinfo
outputs so that `transport_protocol_type` isn't stuck at 'detecting'.
- on the python side, this is ensured by default
`wait_for_handshake = True` inside `add_p2p_connection()`.
- on the c++ side, add a wait_until statement till
`transport_protocol_type = v2` so that v2 handshake is complete.
Co-Authored-By: Martin Zumsande <mzumsande@gmail.com>
|
|
This changes the default behavior, individual tests can overwrite this option.
As a result, it is possible to run the entire test suite with
--v2transport, and all connections to the python p2p will then use it.
Also adjust several tests that are already running with --v2transport in the
test runner (although they actually made v1 connection before this change).
This is done in the same commit so that there isn't an
intermediate commit in which the CI fails.
|
|
bc9283c4415a932ec1eeb70ca2aa4399c80437b3 [test] Add functional test to test early key response behaviour in BIP 324 (stratospher)
ffe6a56d75c0b47d0729e4e0b7225a827b43ad89 [test] Check whether v2 TestNode performs downgrading (stratospher)
ba737358a37438c18f0fba723eab10ccfd9aae9b [test] Add functional tests to test v2 P2P behaviour (stratospher)
4115cf995647d1a513caecb54a4ff3f51927aa8e [test] Ignore BIP324 decoy messages (stratospher)
8c054aa04d33b247744b3747cd5bf3005a013e90 [test] Allow inbound and outbound connections supporting v2 P2P protocol (stratospher)
382894c3acd2dbf3e4198814f547c75b6fb17706 [test] Reconnect using v1 P2P when v2 P2P terminates due to magic byte mismatch (stratospher)
a94e350ac0e5b65ef23a84b05fb10d1204c98c97 [test] Build v2 P2P messages (stratospher)
bb7bffed799dc5ad8b606768164fce46d4cbf9d0 [test] Use lock for sending P2P messages in test framework (stratospher)
5b91fb14aba7d7fe45c9ac364526815bec742356 [test] Read v2 P2P messages (stratospher)
05bddb20f5cc9036fd680500bde8ece70dbf0646 [test] Perform initial v2 handshake (stratospher)
a049d1bd08c8cdb3b693520f24f8a82572dcaab1 [test] Introduce EncryptedP2PState object in P2PConnection (stratospher)
b89fa59e715a185d9fa7fce089dad4273d3b1532 [test] Construct class to handle v2 P2P protocol functions (stratospher)
8d6c848a48530893ca40be5c1285541b3e7a94f3 [test] Move MAGIC_BYTES to messages.py (stratospher)
595ad4b16880ae1f23463ca9985381c8eae945d8 [test/crypto] Add ECDH (stratospher)
4487b8051797173c7ab432e75efa370afb03b529 [rpc/net] Allow v2 p2p support in addconnection (stratospher)
Pull request description:
This PR introduces support for v2 P2P encryption(BIP 324) in the existing functional test framework and adds functional tests for the same.
### commits overview
1. introduces a new class `EncryptedP2PState` to store the keys, functions for performing the initial v2 handshake and encryption/decryption.
3. this class is used by `P2PConnection` in inbound/outbound connections to perform the initial v2 handshake before the v1 version handshake. Only after the initial v2 handshake is performed do application layer P2P messages(version, verack etc..) get exchanged. (in a v2 connection)
- `v2_state` is the object of class `EncryptedP2PState` in `P2PConnection` used to store its keys, session-id etc.
- a node [advertising](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#advertising-to-support-v2-p2p) support for v2 P2P is different from a node actually [supporting v2 P2P](https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md#supporting-v2-p2p) (differ when false advertisement of services occur)
- introduce a boolean variable `supports_v2_p2p` in `P2PConnection` to denote if it supports v2 P2P.
- introduce a boolean variable `advertises_v2_p2p` to denote whether `P2PConnection` which mimics peer behaviour advertises V2 P2P support. Default option is `False`.
- In the test framework, you can create Inbound and Outbound connections to `TestNode`
1. During **Inbound Connections**, `P2PConnection` is the initiator [`TestNode` <--------- `P2PConnection`]
- Case 1:
- if the `TestNode` advertises/signals v2 P2P support (means `self.nodes[i]` set up with `"-v2transport=1"`), different behaviour will be exhibited based on whether:
1. `P2PConnection` supports v2 P2P
2. `P2PConnection` does not support v2 P2P
- In a real world scenario, the initiator node would intrinsically know if they support v2 P2P based on whatever code they choose to run. However, in the test scenario where we mimic peer behaviour, we have no way of knowing if `P2PConnection` should support v2 P2P or not. So `supports_v2_p2p` boolean variable is used as an option to enable support for v2 P2P in `P2PConnection`.
- Since the `TestNode` advertises v2 P2P support (using "-v2transport=1"), our initiator `P2PConnection` would send:
1. (if the `P2PConnection` supports v2 P2P) ellswift + garbage bytes to initiate the connection
2. (if the `P2PConnection` does not support v2 P2P) version message to initiate the connection
- Case 2:
- if the `TestNode` doesn't signal v2 P2P support; `P2PConnection` being the initiator would send version message to initiate a connection.
2. During **Outbound Connections** [TestNode --------> P2PConnection]
- initiator `TestNode` would send:
- (if the `P2PConnection` advertises v2 P2P) ellswift + garbage bytes to initiate the connection
- (if the `P2PConnection` advertises v2 P2P) version message to initiate the connection
- Suppose `P2PConnection` advertises v2 P2P support when it actually doesn't support v2 P2P (false advertisement scenario)
- `TestNode` sends ellswift + garbage bytes
- `P2PConnection` receives but can't process it and disconnects.
- `TestNode` then tries using v1 P2P and sends version message
- `P2PConnection` receives/processes this successfully and they communicate on v1 P2P
4. the encrypted P2P messages follow a different format - 3 byte length + 1-13 byte message_type + payload + 16 byte MAC
5. includes support for testing decoy messages and v2 connection downgrade(using false advertisement - when a v2 node makes an outbound connection to a node which doesn't support v2 but is advertised as v2 by some malicious
intermediary)
### run the tests
* functional test - `test/functional/p2p_v2_encrypted.py` `test/functional/p2p_v2_earlykeyresponse.py`
I'm also super grateful to @ dhruv for his really valuable feedback on this branch.
Also written a more elaborate explanation here - https://github.com/stratospher/blogosphere/blob/main/integration_test_bip324.md
ACKs for top commit:
naumenkogs:
ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
mzumsande:
Code Review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
theStack:
Code-review ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
glozow:
ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3
Tree-SHA512: 9b54ed27e925e1775e0e0d35e959cdbf2a9a1aab7bcf5d027e66f8b59780bdd0458a7a4311ddc7dd67657a4a2a2cd5034ead75524420d58a83f642a8304c9811
|
|
- Add an optional `supports_v2_p2p` parameter to specify if the inbound
and outbound connections support v2 P2P protocol.
- In the `addconnection_callback` which gets called when creating
outbound connections, call the `addconnection` RPC with v2 P2P protocol
support enabled.
|
|
The nodes are restarted with an empty addrman and populated
with addresses from different networks using a helper function.
We can safely add multiple addresses to addrman tables without
worrying about unpredictable collisions since bucket:position
is fixed in a deterministic addrman.
|
|
this test inserts 1 address into the new table and 1 address into
the tried table so that no collisions can happen in either table
if a second address is added. this setup does not need to be
maintained anymore since we can use a deterministic addrman and
safely add many addresses in both tables without collisions. Remove
comment explaining why previous setup needed to be maintained.
|
|
ea00f982d21aab51001d422225f00626a74db298 test: fix intermittent error in rpc_net.py (#29030) (Sebastian Falbesoner)
Pull request description:
Asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following getpeerinfo() call, as the debug message is written in the CNode ctor, which means it hasn't necessarily been added to CConnman.m_nodes at this point.
Solve this by using the recently introduced `wait_for_new_peer` helper (see #29006, commit 00e0658e77f66103ebdeb29def99dc9f937c049d), which is more robust.
Fixes #29030.
ACKs for top commit:
maflcko:
lgtm ACK ea00f982d21aab51001d422225f00626a74db298
Tree-SHA512: dda307949a466fb3b24408a8c213d307e0af2155f2e8b4e52c836a22397f9d218bf9d8c54ca55bae62a96d7566f27167db9311dd8801785c327234783af5ed00
|
|
Asserting for the debug log message "Added connection peer=" is
insufficient for ensuring that this new connection will show up in a
following getpeerinfo() call, as the debug message is written in the
CNode ctor, which means it hasn't necessarily been added to
CConnman.m_nodes at this point.
Solve this by using the recently introduced `wait_for_new_peer`
helper, which is more robust.
Fixes #29030.
|
|
This is the functional test counterpart of PR #28891 /
commit 007d6f0e85bc329040bb405ef6016339518caa66.
|
|
with --v2transport
35fb9930adb3501b29d3ad20d2e74c0114f2bcbe test: enable v2 transport for p2p_timeouts.py (Martin Zumsande)
2c1669c37a9759e15ff5f4e340aeaa8778a81b9a test: enable v2 transport for rpc_net.py (Sebastian Falbesoner)
cc961c26956859850202f56191981b0306a65fcf test: enable v2 transport for p2p_node_network_limited.py (Sebastian Falbesoner)
3598a1b5c932634dc7ccb991cc83df5e1a1dcaa9 test: enable --v2transport in combination with --usecli (Martin Zumsande)
68a90017519874793e34e3b439a63e5aa3a6f6a7 test: persist -v2transport over restarts and respect -v2transport=0 (Martin Zumsande)
Pull request description:
This makes the functional test suite compatible with BIP324, so that
`python3 test_runner.py --v2transport`
should succeed (currently, 12 tests fail for me on master).
Includes two commits by TheStack I found in an old discussion https://github.com/bitcoin/bitcoin/pull/28331#discussion_r1326714164
Note that even though all tests should pass, the python `p2p.py` module will do v2 connections only after the merge of #24748, so that for now only connections between two full nodes will actually run v2.
Some of the fixed tests were added with `--v2transport` to the test runner. Though after #24748 we might also want to consider running the entire suite with `--v2transport` in some CI.
ACKs for top commit:
sipa:
utACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe. Thanks for taking care of this.
achow101:
ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe
theStack:
ACK 35fb9930adb3501b29d3ad20d2e74c0114f2bcbe
stratospher:
ACK 35fb993.
Tree-SHA512: 80dc0bf211fa525ff1d092043aea9f222f14c02e5832a548fb8b83b9ede1fcee03c5e8ade0d05c331bdaa492af9c1cf3d0f0b15b846673c6eacea82dd4cefbc3
|
|
- "transport_protocol_type" of inbound peer before version handshake
is "detecting" on p2p v2 nodes (as opposed to "v1" for p2p v1)
- size of a ping/pong message is 29 bytes (as opposed to 32 for p2p v1)
- for the sendmsgtopeer RPC sub-test, enforce p2p v1 connection to
have a peer id of zero
|
|
0420f99f429ce2382057e101859067f40de47be0 Create net_peer_connection unit tests (Jon Atack)
4b834f649921aceb44d3e0b5a2ffd7847903f9f7 Allow unit tests to access additional CConnman members (Jon Atack)
34b9ef443bc2655a85c8802edc5d5d48d792a286 net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura)
94e8882d820969ddc83f24f4cbe1515a886da4ea rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura)
2574b7e177ef045e64f1dd48cb000640ff5103d3 net/rpc: Check all resolved addresses in ConnectNode rather than just one (Sergi Delgado Segura)
Pull request description:
## Rationale
Currently, `addnode` has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to `m_added_nodes`, hence making Bitcoin iterate through useless data on a regular basis.
### Connecting to the same node more than once
In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via `addnode` in two different ways:
1. Calling `addnode` more than once in a short time period, using two equivalent but distinct addresses
2. Calling `addnode add` using an IP, and `addnode onetry` after with an address that resolved to the same IP
For the former, the issue boils down to `CConnman::ThreadOpenAddedConnections` calling `CConnman::GetAddedNodeInfo` once, and iterating over the result to open connections (`CConman::OpenNetworkConnection`) on the same loop for all addresses.`CConnman::ConnectNode` only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.
An example to test this would be calling:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" add
```
And check how it allows us to perform both connections some times, and some times it fails.
The latter boils down to the same issue, but takes advantage of `onetry` bypassing the `CConnman::ThreadOpenAddedConnections` logic and calling `CConnman::OpenNetworkConnection` straightaway. A way to test this would be:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" onetry
```
### Adding the same peer with two different, yet equivalent, addresses
The current implementation of `addnode` is pretty naive when checking what data is added to `m_added_nodes`. Given the collection stores strings, the checks at `CConnman::AddNode()` basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.
Two examples would be `127.0.0.1` being equal to `127.1` and `[::1]` being equal to `[0:0:0:0:0:0:0:1]`. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by `getaddednodeinfo`, given they map to the same `CService`.
This is less severe than the previous issue, since even tough both nodes are reported as connected by `getaddednodeinfo`, there is only a single connection to them (as properly reported by `getpeerinfo`). However, this adds redundant data to `m_added_nodes`, which is undesirable.
### Parametrize `CConnman::GetAddedNodeInfo`
Finally, this PR also parametrizes `CConnman::GetAddedNodeInfo` so it returns either all added nodes info, or only info about the nodes we are **not** connected to. This method is used both for `rpc`, in `getaddednodeinfo`, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in `CConnman::ThreadOpenAddedConnections`, in which we are currently returning more data than needed and then actively filtering using `CService.fConnected()`
ACKs for top commit:
jonatack:
re-ACK 0420f99f429ce2382057e101859067f40de47be0
kashifs:
> > tACK [0420f9](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0)
sr-gi:
> > > tACK [0420f9](https://github.com/bitcoin/bitcoin/commit/0420f99f429ce2382057e101859067f40de47be0)
mzumsande:
Tested ACK 0420f99f429ce2382057e101859067f40de47be0
Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
|
|
Currently it is possible to add the same node twice when formatting IPs in
different, yet equivalent, manner. This applies to both ipv4 and ipv6, e.g:
127.0.0.1 = 127.1 | [::1] = [0:0:0:0:0:0:0:1]
`addnode` will accept both and display both as connected (given they translate to
the same IP). This will not result in multiple connections to the same node, but
will report redundant info when querying `getaddednodeinfo` and populate `m_added_nodes`
with redundant data.
This can be avoided performing comparing the contents of `m_added_addr` and the address
to be added as `CServices` instead of as strings.
|
|
|
|
- make `getaddrmaninfo` RPC public since it's not for development
purposes only and regular users might find it useful
- add missing `all_networks` key to RPC help
- use clang format spacing
|
|
addrman table entries
352d5eb2a9e89cff4a2815d94a9d81fcc20c4b2c test: getrawaddrman RPC (0xb10c)
da384a286bd84a97e7ebe7a64654c5be20ab2df1 rpc: getrawaddrman for addrman entries (0xb10c)
Pull request description:
Inspired by `getaddrmaninfo` (#27511), this adds a hidden/test-only `getrawaddrman` RPC. The RPC returns information on all addresses in the address manager new and tried tables. Addrman table contents can be used in tests and during development.
The RPC result encodes the `bucket` and `position`, the internal location of addresses in the tables, in the address object's string key. This allows users to choose to consume or to ignore the location information. If the internals of the address manager implementation change, the location encoding might change too.
```
getrawaddrman
EXPERIMENTAL warning: this call may be changed in future releases.
Returns information on all address manager entries for the new and tried tables.
Result:
{ (json object)
"table" : { (json object) buckets with addresses in the address manager table ( new, tried )
"bucket/position" : { (json object) the location in the address manager table (<bucket>/<position>)
"address" : "str", (string) The address of the node
"port" : n, (numeric) The port number of the node
"network" : "str", (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the address
"services" : n, (numeric) The services offered by the node
"time" : xxx, (numeric) The UNIX epoch time when the node was last seen
"source" : "str", (string) The address that relayed the address to us
"source_network" : "str" (string) The network (ipv4, ipv6, onion, i2p, cjdns) of the source address
},
...
},
...
}
Examples:
> bitcoin-cli getrawaddrman
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getrawaddrman", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
```
ACKs for top commit:
willcl-ark:
reACK 352d5eb2a9
amitiuttarwar:
reACK 352d5eb2a9e
stratospher:
reACK 352d5eb.
achow101:
ACK 352d5eb2a9e89cff4a2815d94a9d81fcc20c4b2c
Tree-SHA512: cc462666b5c709617c66b0e3e9a17c4c81e9e295f91bdd9572492d1cb6466fc9b6d48ee805ebe82f9f16010798370effe5c8f4db15065b8c7c0d8637675d615e
|
|
Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
|
|
Test that the getrawaddrman returns the addresses in the new and tried
tables. We can't check the buckets and positions as these are not
deterministic (yet).
|
|
for invalid command
f52cb02f700b58bca921a7aa24bfeee04760262b doc: make it clear that `node` in `addnode` refers to the node's address (brunoerg)
effd1efefb53c58f0e43fec4f019a19f97795553 test: `addnode` with an invalid command should throw an error (brunoerg)
56b27b84877376ffc32b3bad09f1047b23de4ba1 rpc, refactor: clean-up `addnode` (brunoerg)
Pull request description:
This PR:
- Adds test coverage for an invalid `command` in `addnode`.
- Rename `test_getaddednodeinfo` to `test_addnode_getaddednodeinfo` and its log since this function also tests `addnode` and it doesn't worth to split into 2 ones.
- Makes it clear in docs that `node` in `addnode` refers to the node's address. It seemed a little weird for me "The node (see getpeerinfo for nodes)", it could mean a lot of things e.g. the node id.
- Some small improv/clean-up: use `const` where possible, rename some vars, and remove the check for nullance for `command` since it's a non-optional field.
ACKs for top commit:
achow101:
ACK f52cb02f700b58bca921a7aa24bfeee04760262b
jonatack:
ACK f52cb02f700b58bca921a7aa24bfeee04760262b
theStack:
re-ACK f52cb02f700b58bca921a7aa24bfeee04760262b
Tree-SHA512: e4a69e58b784e233463945b4d55a401957f9fe4562c129f59216a44f44fb3221d3449ac578fb35e665ca654c6ade2e741b72c3df78040f7527229c77b6c5b82e
|
|
|
|
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
|
|
|
|
The wait in disconnect_p2ps checked for subver, which
is unavailable for the peer that didn't send a version msg.
|
|
|
|
- Ensures port sanitization in `addpeeraddress()`
- Adds test to check for invalid port values
|
|
changes
fac23c211407a77af82bb1491c48c8d37022c8b3 scripted-diff: Bump copyright headers (MarcoFalke)
fa974f1f1417a536636347072e86bcb54a4c909c scripted-diff: Remove redundant sync_all and sync_blocks (MarcoFalke)
fad13991aea6463ecf07dd907de1c1b23837d7e7 test: Properly set sync_fun in NodeNetworkLimitedTest (MarcoFalke)
faeff577093c4de9eec9491486a2c3766d46dae6 test: Use 4 spaces for indentation (MarcoFalke)
Pull request description:
Some cleanups after commit 94db963de501e4aba6e5d8150a01ceb85753dee1
ACKs for top commit:
fanquake:
ACK fac23c211407a77af82bb1491c48c8d37022c8b3
Tree-SHA512: 5acfd5bb9679b41969d0fc6fc85801ccadcd6530ea692bac6352668e06fc7a9b0e1db3fd6fba435e84afe983d2eb07bd0a47c8364462bb7110004bd3d102b698
|
|
in milliseconds
22b44fc696dc1078c40d17e2d497c74c7b4ae750 p2p: improve checkaddrman logging with duration in milliseconds (Jon Atack)
ec65bed00ee2e403e39b3c5977caf4abd31ccc87 log, timer: add LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE macro (Jon Atack)
325da75a5396f3161a6eade74b349105ed5722ab log, timer: allow not repeating log message on completion (Jon Atack)
Pull request description:
This patch:
- updates the `logging/timer.h::Timer` class to allow not repeating the log message on completion
- adds a `LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE` macro that prints the descriptive message when logging the start but not when logging the completion
- updates the checkaddrman logging to log the duration, and renames the function like the `-checkaddrman` configuration option in order to prefix every log message with `CheckAddrman` instead of the longer, less pleasant, and different-from-checkaddrman `ForceCheckAddrman` (the Doxygen documentation on the function already makes clear that it is unaffected by `m_consistency_check_ratio`).
before
```
2021-09-21T18:42:50Z [opencon] Addrman checks started: new 64864, tried 1690, total 66554
2021-09-21T18:42:50Z [opencon] Addrman checks completed successfully
```
after
```
2021-09-21T18:42:50Z [opencon] CheckAddrman: new 64864, tried 1690, total 66554 started
2021-09-21T18:42:50Z [opencon] CheckAddrman: completed (76.21ms)
```
To test, build and run bitcoind with `-debug=addrman -checkaddrman=<n>` for a value of `n` in the range of, say, 10 to 40.
ACKs for top commit:
laanwj:
Code review ACK 22b44fc696dc1078c40d17e2d497c74c7b4ae750
Tree-SHA512: 658c0dfaaa9d07092e2418f2d05007c58cc35be6593f22b3c592ce793334a885dd92dacc46bdeddc9d37939cf11174660a094c07c0fa117fbb282953aa45a94d
|
|
The previous diff touched most files in ./test/, so bump the headers to
avoid having to touch them again for a bump later.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
|
|
The sync calls are redundant after a call to generate, because generate
already syncs itself.
-BEGIN VERIFY SCRIPT-
perl -0777 -pi -e 's/(generate[^\n]*\)[^\n]*)(\n|\s)+self.sync_(all|blocks)\([^\)]*\)\n/\1\n/g' $(git grep -l generate ./test)
-END VERIFY SCRIPT-
|
|
|
|
This allows to use "cjdns" as an argument to the `getnodeaddresses` RPC
and to the `-onlynet=` parameter.
|
|
and update the function name to CheckAddrman (drop "Force") for
nicer log output as it is prefixed to each of these log messages:
2021-09-21T18:42:50Z [opencon] CheckAddrman: new 64864, tried 1690, total 66554 started
2021-09-21T18:42:50Z [opencon] CheckAddrman: completed (76.21ms)
The existing Doxygen documentation on the function already makes
clear that it is unaffected by m_consistency_check_ratio.
|
|
constant in functional tests
b69a106bcd8ddefdb810df6ebb3625c430197e04 test: use test_framework.p2p P2P_SERVICES in functional tests (Jon Atack)
Pull request description:
`P2P_SERVICES` is defined in `test/functional/test_framework/p2p.py`, so we can use it as a single definition for our functional tests. It may also be a tiny bit more efficient to use the constant rather than calculating `NODE_NETWORK | NODE_WITNESS` every time we need it in the tests.
ACKs for top commit:
laanwj:
Code review ACK b69a106bcd8ddefdb810df6ebb3625c430197e04
klementtan:
crACK b69a106bcd8ddefdb810df6ebb3625c430197e04
fanquake:
ACK b69a106bcd8ddefdb810df6ebb3625c430197e04 - didn't look at the formatting changes.
Tree-SHA512: f83e593663a69182986325d9ba2b4b787b87896d6648973f4f802f191a2573201b9e7d7e10e69662ef1965fa63268845726ed1aa5742a2e38dcccf4aebc6a961
|
|
checks on restart with asmap
cdaab90662a54e331de0e49a89596bbb94a8ac45 Add test for addrman consistency check on restart with asmap (Jon Atack)
869f136816c6900ce84bc4b5a9c93c0deab85193 Add test for rpc addpeeraddress with "tried" argument (Jon Atack)
ef242f52137f2a79a739447251d7759bd4705be0 Allow passing "tried" to rpc addpeeraddress to call CAddrMan::Good() (Jon Atack)
Pull request description:
This pull adds a `tried` argument to RPC addpeeraddress and a regression test for the recent addrman/asmap changes and issue.
PR #22697 introduced a reproducible bug in commit 181a1207 that fails addrman consistency checks and causes it to significantly lose peer entries when the `-asmap` configuration option is used.
The issue occurs upon bitcoind restart due to an initialization order change in `src/init.cpp` in that commit, whereby CAddrman asmap is set after deserializing `peers.dat`, rather than before.
Issue reported on the `#bitcoin-core-dev` IRC channel starting at https://www.erisian.com.au/bitcoin-core-dev/log-2021-08-23.html#l-263.
```
addrman lost 22813 new and 2 tried addresses due to collisions or invalid addresses
ADDRMAN CONSISTENCY CHECK FAILED!!! err=-17 bitcoind: ./addrman.h:707: void CAddrMan::Check() const: Assertion `false' failed. Aborted
```
How to reproduce:
- `git checkout 181a1207`, build, and launch bitcoind with the `-asmap` and `-checkaddrman=1` configuration options enabled
- restart bitcoind
- bitcoind aborts on the second call to the addrman consistency checks in `CAddrMan::Check()`
How to test this pull:
- `git checkout 181a1207`, cherry pick the first commit of this branch, build, git checkout this branch, run `test/functional/rpc_net.py`, which should pass, and then run `test/functional/feature_asmap.py`, which should fail with the following output:
```
AssertionError: Unexpected stderr bitcoind: ./addrman.h:739: void CAddrMan::Check() const: Assertion `false' failed.
```
ACKs for top commit:
jnewbery:
utACK cdaab90662a54e331de0e49a89596bbb94a8ac45
mzumsande:
re-ACK cdaab90662a54e331de0e49a89596bbb94a8ac45 (based on code review of diff to d586817)
vasild:
ACK cdaab90662a54e331de0e49a89596bbb94a8ac45
Tree-SHA512: 0251a18fea629b62486fc907d7ab0e96c6df6fadb9e4d62cff018bc681afb6ac31e0e7258809c0a88f91e4a36c4fb0b16ed294ce47ef30585217de89c3342399
|
|
|
|
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Co-authored-by: John Newbery <john@johnnewbery.com>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
-BEGIN VERIFY SCRIPT-
sed --regexp-extended -i \
's/((self\.)?(nodes\[[^]]+\]|[a-z_]*(wallet|node)[0-9a-z_]*))\.(generate(|toaddress|block|todescriptor)(\(|, ))/self.\5\1, /g' \
$(git grep -l generate ./test | grep -v 'test_framework/' | grep -v 'feature_rbf')
-END VERIFY SCRIPT-
|
|
bfa9309ad606102f24c9bd3c33dfe78949f09418 Use COINBASE_MATURITY constant in functional tests. (Kiminuo)
525448df9dc2ab6b7e960ff138956ae3e2efdf60 Move COINBASE_MATURITY from `feature_nulldummy` test to `blocktools`. (Kiminuo)
Pull request description:
`COINBASE_MATURITY` constant was added to `feature_nulldummy` test in #21373. This PR moves the constant to `blocktools.py` file and uses the constant in more tests as suggested [here](https://github.com/bitcoin/bitcoin/pull/21373#discussion_r605418462).
Edit: Goal of this PR is to replace integer constants with `COINBASE_MATURITY` but not necessarily in *all* cases because that would mean to read and fully understand all tests. That's out of my time constraints. Any reports where `COINBASE_MATURITY` should be used are welcome though!
ACKs for top commit:
theStack:
ACK bfa9309ad606102f24c9bd3c33dfe78949f09418 🌇
Tree-SHA512: 01f04645f05a39028681f355cf3d42dd63ea3303f76d93c430e0fdce441934358a2d847a54e6068d61932f1b75e1d406f51859b057b3e4b569f7083915cb317f
|
|
|
|
|
