| Age | Commit message (Collapse) | Author |
|---|
|
|
|
`AddWhitelistPermissionFlags`
|
|
Simplify ParamsStream usage in ConvertSeeds now that ParamsStream supports
rvalue substream arguments.
|
|
Move parameter argument after stream argument so will be possible to accept
multiple variadic parameter arguments in the following commit.
Also reverse template parameter order for consistency.
|
|
27f260aa6e04f82dad78e9a06d58927546143a27 net: remove now unused global 'g_initial_block_download_completed' (furszy)
aff7d92b1500e2478ce36a7e86ae47df47dda178 test: add coverage for peerman adaptive connections service flags (furszy)
6ed53602ac7c565273b5722de167cb2569a0e381 net: peer manager, dynamically adjust desirable services flag (furszy)
9f36e591c551ec2e58a6496334541bfdae8fdfe5 net: move state dependent peer services flags (furszy)
f9ac96b8d6f4eba23c88f302b22a2c676e351263 net: decouple state independent service flags from desirable ones (furszy)
97df4e38879d2644aeec34c1eef241fed627333e net: store best block tip time inside PeerManager (furszy)
Pull request description:
Derived from #28120 discussion.
By relocating the peer desirable services flags into the peer manager, we
allow the connections acceptance process to handle post-IBD potential
stalling scenarios.
The peer manager will be able to dynamically adjust the services flags
based on the node's proximity to the tip (back and forth). Allowing the node
to recover from the following post-IBD scenario:
Suppose the node has successfully synced the chain, but later experienced
dropped connections and remained inactive for a duration longer than the limited
peers threshold (the timeframe within which limited peers can provide blocks). In
such cases, upon reconnecting to the network, the node might only establish
connections with limited peers, filling up all available outbound slots. Resulting
in an inability to synchronize the chain (because limited peers will not provide
blocks older than the `NODE_NETWORK_LIMITED_MIN_BLOCKS` threshold).
ACKs for top commit:
achow101:
ACK 27f260aa6e04f82dad78e9a06d58927546143a27
vasild:
ACK 27f260aa6e04f82dad78e9a06d58927546143a27
naumenkogs:
ACK 27f260aa6e04f82dad78e9a06d58927546143a27
mzumsande:
Light Code Review ACK 27f260aa6e04f82dad78e9a06d58927546143a27
andrewtoth:
ACK 27f260aa6e04f82dad78e9a06d58927546143a27
Tree-SHA512: 07befb9bcd0b60a4e7c45e4429c02e7b6c66244f0910f4b2ad97c9b98258b6f46c914660a717b5ed4ef4814d0dbfae6e18e6559fe9bec7d0fbc2034109200953
|
|
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
|
|
This test-only RPC is required when a TestNode initiates
an outbound v2 p2p connection. Add a new arg `v2transport`
so that the node can attempt v2 connections.
|
|
No behavior change. Just an intermediate refactoring.
By relocating the peer desirable services flags into the peer
manager, we allow the connections acceptance process to handle
post-IBD potential stalling scenarios.
In the follow-up commit(s), the desirable service flags will be
dynamically adjusted to detect post-IBD stalling scenarios (such
as a +48-hour inactive node that must prefer full node connections
instead of limited peer connections because they cannot provide
historical blocks). Additionally, this encapsulation enable us
to customize the connections decision-making process based on
new user's configurations in the future.
|
|
This former one will be moved to the peer manager class in the
following-up commit.
|
|
connections, add to -netinfo
fb5bfed26a564014b83ccfc96ff00b630930fc61 cli: add transport protcol column to -netinfo (Martin Zumsande)
9eed22e870e650cadf5f65650917da21836d2bb0 net: attempt v2 transport for addrfetch connections if we support it (Martin Zumsande)
770c0311ef5e35444efe4fd26f7bb5782624cf2c net: attempt v2 transport for manual connections if we support it (Martin Zumsande)
Pull request description:
Some preparations before enabling `-v2transport` as the default:
* Use v2 for `-connect`, `-addnode` config arg and `-seednode` if `-v2transport` is enabled.
Our peer may or may not support v2, but I don't think an extra option is necessary for any of these (we have that for the `addnode` rpc), because we have the reconnection mechanism that will try again with `v1` if our peer doesn't support `v2`.
* Add a column for the transport protocol to `-netinfo`. I added it next to the `net` column because I thought it looked nice there, but if people prefer it somewhere else I'm happy to move it.
ACKs for top commit:
sipa:
utACK fb5bfed26a564014b83ccfc96ff00b630930fc61
achow101:
ACK fb5bfed26a564014b83ccfc96ff00b630930fc61
stratospher:
tested ACK fb5bfed. addrfetch + manual connections aren't frequent and it would be useful to have this for transition to v2 one day.
theStack:
ACK fb5bfed26a564014b83ccfc96ff00b630930fc61
kristapsk:
ACK fb5bfed26a564014b83ccfc96ff00b630930fc61
Tree-SHA512: c4575ad11b99613870b342acae369fa08f877ac79e6e04eb62e94ad7a92d528e289183c0963c78aa779ba11cb91e2a6fad7c8b0d813126c46c3e5b54bd962c26
|
|
-addrmantest is only used in `p2p_node_network_limited.py` test to
test if the node self-advertises a hard-coded local address
(which wouldn't be advertised in the tests because it's unroutable
without the test-only code path) to check pruning-related services
are correct in that addr.
Remove -addrmantest because the self advertisement happens because
of hard coded test path logic, and expected services are nominal
due to how easily the test-only code could diverge from mainnet
logic. It's also being used only in 1 test.
|
|
|
|
This affects manual connections made either with -connect, or with
-addnode provided as a bitcoind config arg (the addnode RPC has an
extra option for v2).
We don't necessarily know if our peer supports v2, but will reconnect
with v1 if they don't. In order to do that, improve the reconnection
behavior such that we will reconnect after a sleep of 500ms
(which usually should be enough for our peer to send us their
version message).
|
|
Making the `GenerateRandomKey` helper available to other modules via
key.{h.cpp} allows us to create random private keys directly at
instantiation of CKey, in contrast to the two-step process of creating
the instance and then having to call `MakeNewKey(...)`.
|
|
3ea54e5db7d53da5afa321e1800c29aa269dd3b3 net: Add continuous ASMap health check logging (Fabian Jahr)
28d7e55dff826a69f3f8e58139dbffb611cc5947 test: Add tests for unfiltered GetAddr usage (Fabian Jahr)
b8843d37aed1276ff8527328c956c70c6e02ee13 fuzz: Let fuzzers use filter options in GetAddr/GetAddresses (Fabian Jahr)
e16f420547fc72a5a2902927aa7138e43c0fb7c8 net: Optionally include terrible addresses in GetAddr results (Fabian Jahr)
Pull request description:
There are certain statistics we can collect by running all our known clearnet addresses against the ASMap file. This could show issues with a maliciously manipulated file or with an old file that has decayed with time.
This is just a proof of concept for now. My idea currently is to run the analysis once per day and print the results to logs if an ASMap file is used.
ACKs for top commit:
achow101:
ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
mzumsande:
ACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
brunoerg:
crACK 3ea54e5db7d53da5afa321e1800c29aa269dd3b3
Tree-SHA512: 777acbfac43cc43ce4a0a3612434e4ddbc65f59ae8ffc9e24f21de09011bccb297f0599cbaa82bcf40ef68e5af582c4e98556379db7ceff7d9f97574a1cf8e09
|
|
|
|
version and type
fa79a881ce0537e1d74da296a7513730438d2a02 refactor: P2P transport without serialize version and type (MarcoFalke)
fa9b5f4fe32c0cfe2e477bb11912756f84a52cfe refactor: NetMsg::Make() without nVersion (MarcoFalke)
66669da4a5ca9edf2a40d20879d9a8aaf2b9e2ee Remove unused Make() overload in netmessagemaker.h (MarcoFalke)
fa0ed0794161d937d2d3385963c1aa5624b60d17 refactor: VectorWriter without nVersion (MarcoFalke)
Pull request description:
Now that the serialize framework ignores the serialize version and serialize type, everything related to it can be removed from the code.
This is the first step, removing dead code from the P2P stack. A different pull will remove it from the wallet and other parts.
ACKs for top commit:
ajtowns:
reACK fa79a881ce0537e1d74da296a7513730438d2a02
Tree-SHA512: 785b413580d980f51f0d4f70ea5e0a99ce14cd12cb065393de2f5254891be94a14f4266110c8b87bd2dbc37467676655bce13bdb295ab139749fcd8b61bd5110
|
|
|
|
The field is unused, so remove it.
This is also required for future commits.
|
|
to allocate our limited outbound slots correctly, and to ensure addnode
connections benefit from their intended protections.
Our addnode logic usually connects the addnode peers before the automatic
outbound logic does, but not always, as a connection race can occur. If an
addnode peer disconnects us and if it was the only one from its network, there
can be a race between reconnecting to it with the addnode thread, and it being
picked as automatic network-specific outbound peer. Or our internet connection
or router, or the addnode peer, could be temporarily offline, and then return
online during the automatic outbound thread. Or we could add a new manual peer
using the addnode RPC at that time.
The race can be more apparent when our node doesn't know many peers, or with
networks like cjdns that currently have few bitcoin peers.
When an addnode peer is connected as an automatic outbound peer and is the only
connection we have to a network, it can be protected by our new outbound
eviction logic and persist in the "wrong role".
Examples on mainnet using logging added in the same pull request:
2023-08-12T14:51:05.681743Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic network-specific outbound-full-relay connection
to i2p peer selected for manual (addnode) connection: [geh...odq.b32.i2p]:0
2023-08-13T03:59:28.050853Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic block-relay-only connection to onion peer
selected for manual (addnode) connection: kpg...aid.onion:8333
2023-08-13T16:21:26.979052Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic network-specific outbound-full-relay connection
to cjdns peer selected for manual (addnode) connection: [fcc...8ce]:8333
2023-08-14T20:43:53.401271Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic network-specific outbound-full-relay connection
to cjdns peer selected for manual (addnode) connection: [fc7...59e]:8333
2023-08-15T00:10:01.894147Z [opencon] [net.cpp:1949] [ThreadOpenConnections]
[net:debug] Not making automatic feeler connection to i2p peer selected for
manual (addnode) connection: geh...odq.b32.i2p:8333
Finally, there does not seem to be a reason to make block-relay or short-lived
feeler connections to addnode peers, as the addnode logic will ensure we connect
to them if they are up, within the addnode connection limit.
Fix these issues by checking if the address is an addnode peer in our automatic
outbound connection logic.
|
|
Also remove an assert that is already enforced by the compiler checking
that the length of the std::array matches.
|
|
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
|
|
df69b22f2e3cc03764a582f29a16a36114f67e17 doc: improve documentation around connection limit maximums (Amiti Uttarwar)
adc171edf45ec90857d990b8ec570f3c8c2242b7 scripted-diff: Rename connection limit variables (Amiti Uttarwar)
e9fd9c0225527ec7727d2a7ccbdf028784aadc6c net: add m_max_inbound to connman (Amiti Uttarwar)
c25e0e05550426f29d79571368d90f63fb472b02 net, refactor: move calculations for connection type limits into connman (Amiti Uttarwar)
Pull request description:
This is joint work with amitiuttarwar.
This has the first few commits of #28463. It is not strictly a prerequisite for that, but has changes that in our opinion make sense on their own.
It improves the handling of maximum numbers for different connection types (that are set during init and don’t change after) by:
* moving all calculations into one place, `CConnMan::Init()`. Before, they were dispersed between `Init`, `CConnman::Init` and other parts of `CConnman`, resulting in some duplicated test code.
* removing the possibility of having a negative maximum of inbound connections, which is hard to argue about
* renaming of variables and doc improvements
ACKs for top commit:
amitiuttarwar:
co-author review ACK df69b22f2e3cc03764a582f29a16a36114f67e17
naumenkogs:
ACK df69b22f2e3cc03764a582f29a16a36114f67e17
achow101:
ACK df69b22f2e3cc03764a582f29a16a36114f67e17
Tree-SHA512: 913d56136bc1df739978de50db67302f88bac2a9d34748ae96763288d97093e998fc0f94f9b6eff12867712d7e86225af6128f4170bf2b5b8ab76f024870a22c
|
|
af0fca530e4d8311bcb24a14c416e5ad7c30ff78 netbase: use reliable send() during SOCKS5 handshake (Vasil Dimov)
1b19d1117ca5373a15313227b547ef4392022dbd sock: change Sock::SendComplete() to take Span (Vasil Dimov)
Pull request description:
The `Socks5()` function which does the SOCKS5 handshake with the SOCKS5 proxy sends bytes to the socket without retrying partial writes.
`send(2)` may write only part of the provided data and return. In this case the caller is responsible for retrying the operation with the remaining data. Change `Socks5()` to do that. There is already a method `Sock::SendComplete()` which does exactly that, so use it in `Socks5()`.
A minor complication for this PR is that `Sock::SendComplete()` takes `std::string` argument whereas `Socks5()` has `std::vector<uint8_t>`. Thus the necessity for the first commit. It is possible to do also in other ways - convert the data in `Socks5()` to `std::string` or have just one `Sock::SendComplete()` that takes `void*` and change the callers to pass `str.data(), str.size()` or `vec.data(), vec.size()`.
This came up while testing https://github.com/bitcoin/bitcoin/pull/27375.
ACKs for top commit:
achow101:
ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
jonatack:
ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
pinheadmz:
ACK af0fca530e4d8311bcb24a14c416e5ad7c30ff78
Tree-SHA512: 1d4a53d0628f7607378038ac56dc3b8624ce9322b034c9547a0c3ce052eafb4b18213f258aa3b57bcb4d990a5e0548a37ec70af2bd55f6e8e6399936f1ce047a
|
|
`send(2)` can be interrupted or for another reason it may not fully
complete sending all the bytes. We should be ready to retry the send
with the remaining bytes. This is what `Sock::SendComplete()` does,
thus use it in `Socks5()`.
Since `Sock::SendComplete()` takes a `CThreadInterrupt` argument,
change also the recv part of `Socks5()` to use `CThreadInterrupt`
instead of a boolean.
Easier reviewed with `git show -b` (ignore white-space changes).
|
|
address on request
`CConnman::GetAddedNodeInfo` is used both to get a list of addresses to manually connect to
in `CConnman::ThreadOpenAddedConnections`, and to report about manually added connections in
`getaddednodeinfo`. In both cases, all addresses added to `m_added_nodes` are returned, however
the nodes we are already connected to are only relevant to the latter, in the former they are
actively discarded.
Parametrizes `CConnman::GetAddedNodeInfo` so we can ask for only addresses we are not connected to,
to avoid passing useless information around.
|
|
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.
|
|
The current `addnode` rpc command has some edge cases in where it is possible to
connect to the same node twice by combining ip and address requests. This can happen under two situations:
The two commands are run one right after each other, in which case they will be processed
under the same loop in `CConnman::ThreadOpenAddedConnections` without refreshing `vInfo`, so both
will go trough. An example of this would be:
```
bitcoin-cli addnode "localhost:port" add
```
A node is added by IP using `addnode "add"` while the other is added by name using
`addnode "onetry"` with an address that resolves to multiple IPs. In this case, we currently
only check one of the resolved IPs (picked at random), instead of all the resolved ones, meaning
this will only probabilistically fail/succeed. An example of this would be:
```
bitcoin-cli addnode "127.0.0.1:port" add
[...]
bitcoin-cli addnode "localhost:port" onetry
```
Both cases can be fixed by iterating over all resolved addresses in `CConnman::ConnectNode` instead
of picking one at random
|
|
|
|
destroy the session if we get an unexpected error
5c8e15c451ec870b9dd4eb843ec6ca3ba64cda4f i2p: destroy the session if we get an unexpected error from the I2P router (Vasil Dimov)
762404a68c114e8831cdfae937627174544b55a7 i2p: also sleep after errors in Accept() (Vasil Dimov)
Pull request description:
### Background
In the `i2p::sam::Session` class:
`Listen()` does:
* if the session is not created yet
* create the control socket and on it:
* `HELLO`
* `SESSION CREATE ID=sessid`
* leave the control socked opened
* create a new socket and on it:
* `HELLO`
* `STREAM ACCEPT ID=sessid`
* read reply (`STREAM STATUS`), `Listen()` only succeeds if it contains `RESULT=OK`
Then a wait starts, for a peer to connect. When connected,
`Accept()` does:
* on the socket from `STREAM ACCEPT` from `Listen()`: read the Base64 identification of the connecting peer
### Problem
The I2P router may be in such a state that this happens in a quick succession (many times per second, see https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1609907115): `Listen()`-succeeds, `Accept()`-fails.
`Accept()` fails because the I2P router sends something that is not Base64 on the socket: `STREAM STATUS RESULT=I2P_ERROR MESSAGE="Session was closed"`
We only sleep after failed `Listen()` because the assumption was that if `Accept()` fails then the next `Listen()` will also fail.
### Solution
Avoid filling the log with "Error accepting:" messages and sleep also after a failed `Accept()`.
### Extra changes
* Reset the error waiting time after one successful connection. Otherwise the timer will remain high due to problems that have been solved long time in the past.
* Increment the wait time less aggressively.
* Handle the unexpected "Session was closed" message more gracefully (don't log stupid messages like `Cannot decode Base64: "STREAM STATUS...`) and destroy the session right way.
ACKs for top commit:
achow101:
ACK 5c8e15c451ec870b9dd4eb843ec6ca3ba64cda4f
jonatack:
re-ACK 5c8e15c451ec870b9dd4eb843ec6ca3ba64cda4f
Tree-SHA512: 1d47958c50eeae9eefcb668b8539fd092adead93328e4bf3355267819304b99ab41cbe1b5dbedbc3452c2bc389dc8330c0e27eb5ccb880e33dc46930a1592885
|
|
|
|
It need not be in the `net` module and we need to call it from
`LookupSubNet()`, thus move it to `netbase`.
|
|
`vfLimited`, `IsReachable()`, `SetReachable()` need not be in the `net`
module. Move them to `netbase` because they will be needed in
`LookupSubNet()` to possibly flip the result to CJDNS (if that network
is reachable).
In the process, encapsulate them in a class.
`NET_UNROUTABLE` and `NET_INTERNAL` are no longer ignored when adding
or removing reachable networks. This was unnecessary.
|
|
Background:
`Listen()` does:
* if the session is not created yet
* create the control socket and on it:
* `HELLO`
* `SESSION CREATE ID=sessid`
* leave the control socked opened
* create a new socket and on it:
* `HELLO`
* `STREAM ACCEPT ID=sessid`
* read reply (`STREAM STATUS`)
Then a wait starts, for a peer to connect. When connected,
`Accept()` does:
* on the socket from `STREAM ACCEPT` from `Listen()`: read the
Base64 identification of the connecting peer
Problem:
The I2P router may be in such a state that this happens in a quick
succession (many times per second, see https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1609907115):
`Listen()`-succeeds, `Accept()`-fails.
`Accept()` fails because the I2P router sends something that is
not Base64 on the socket:
STREAM STATUS RESULT=I2P_ERROR MESSAGE="Session was closed"
We only sleep after failed `Listen()` because the assumption was that
if `Accept()` fails then the next `Listen()` will also fail.
Solution:
Avoid filling the log with "Error accepting:" messages and sleep also
after a failed `Accept()`.
Extra changes:
* Reset the error waiting time after one successful connection.
Otherwise the timer will remain high due to problems that have
vanished long time ago.
* Increment the wait time less aggressively.
|
|
|
|
A "version" message in the V1 protocol starts with a fixed 16 bytes:
* The 4-byte network magic
* The 12-byte zero-padded command "version" plus 5 0x00 bytes
The current code detects incoming V1 connections by just looking at the
first 12 bytes (matching an earlier version of BIP324), but 16 bytes is
more precise. This isn't an observable difference right now, as a 12 byte
prefix ought to be negligible already, but it may become observable with
future extensions to the protocol, so make the code match the
specification.
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/nMaxConnections/m_max_automatic_connections/g' src/net.h src/net.cpp
sed -i 's/\.nMaxConnections/\.m_max_automatic_connections/g' src/init.cpp src/test/denialofservice_tests.cpp
sed -i 's/nMaxFeeler/m_max_feeler/g' src/net.h
sed -i 's/nMaxAddnode/m_max_addnode/g' src/net.h src/net.cpp
sed -i 's/m_max_outbound\([^_]\)/m_max_automatic_outbound\1/g' src/net.h src/net.cpp
-END VERIFY SCRIPT-
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
Extract the logic for calculating & maintaining inbound connection limits to be
a member within connman for consistency with other maximum connection limits.
Note that we now limit m_max_inbound to 0 and don't call
AttemptToEvictConnection() when we don't have any inbounds.
Previously, nMaxInbound could become negative if the user ran with a low
-maxconnections, which didn't break any logic but didn't make sense.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
7df450836969b81e98322c9a09c08b35d1095a25 test: improve sock_tests/move_assignment (Vasil Dimov)
5086a99b84367a45706af7197da1016dd966e6d9 net: remove Sock default constructor, it's not necessary (Vasil Dimov)
7829272f7826511241defd34954e6040ea963f07 net: remove now unnecessary Sock::Get() (Vasil Dimov)
944b21b70ae490a5a746bcc1810a5074d74e9d34 net: don't check if the socket is valid in ConnectSocketDirectly() (Vasil Dimov)
aeac68d036e3cff57ce155f1a904d77f98b357d4 net: don't check if the socket is valid in GetBindAddress() (Vasil Dimov)
5ac1a51ee5a57da59f1ff1986b7d9054484d3c80 i2p: avoid using Sock::Get() for checking for a valid socket (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Peeking at the underlying socket file descriptor of `Sock` and checkig if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of testing/mocking/fuzzing.
Instead use an empty `unique_ptr` to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary.
The default constructor `Sock::Sock()` is unnecessary now after recent changes, thus remove it.
ACKs for top commit:
ajtowns:
ACK 7df450836969b81e98322c9a09c08b35d1095a25
jonatack:
ACK 7df450836969b81e98322c9a09c08b35d1095a25
Tree-SHA512: 9742aeeeabe8690530bf74caa6ba296787028c52f4a3342afd193b05dbbb1f6645935c33ba0a5230199a09af01c666bd3c7fb16b48692a0d185356ea59a8ddbf
|
|
Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
|
|
Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
|
|
When an outbound v2 connection is disconnected without receiving anything, but at
least 24 bytes of our pubkey were sent out (enough to constitute an invalid v1
header), add them to a queue of reconnections to be tried.
The reconnections are in a queue rather than performed immediately, because we should
not block the socket handler thread with connection creation (a blocking operation
that can take multiple seconds).
|
|
|
|
Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
|
|
Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
|
|
This matches the behavior for per-type received bytes.
|
|
version in CKeyPool serialize
fac29a0ab19fda457b55d7a0a37c5cd3d9680f82 Remove SER_GETHASH, hard-code client version in CKeyPool serialize (MarcoFalke)
fa72f09d6ff8ee204f331a69d3f5e825223c9e11 Remove CHashWriter type (MarcoFalke)
fa4a9c0f4334678fb80358ead667807bf2a0a153 Remove unused GetType() from OverrideStream, CVectorWriter, SpanReader (MarcoFalke)
Pull request description:
Removes a bunch of redundant, dead or duplicate code.
Uses the idea from and finishes the idea https://github.com/bitcoin/bitcoin/pull/28428 by theuni
ACKs for top commit:
ajtowns:
ACK fac29a0ab19fda457b55d7a0a37c5cd3d9680f82
kevkevinpal:
added one nit but otherwise ACK [fac29a0](https://github.com/bitcoin/bitcoin/pull/28508/commits/fac29a0ab19fda457b55d7a0a37c5cd3d9680f82)
Tree-SHA512: cc805e2f38e73869a6691fdb5da09fa48524506b87fc93f05d32c336ad3033425a2d7608e317decd3141fde3f084403b8de280396c0c39132336fe0f7510af9e
|
|
|
|
See also https://github.com/bitcoin/bips/pull/1498
The benefit is a simpler implementation:
- The protocol state machine does not need separate states for garbage
authentication and version phases.
- The special case of "ignoring the ignore bit" is removed.
- The freedom to choose the contents of the garbage authentication
packet is removed. This simplifies testing.
|
|
helpers over low-level code, use optional
4ecfd3eaf434d868455466e001adae4b68515ab8 Inline short, often-called, rarely-changed basic CNetAddr getters (Jon Atack)
5316ae5dd8d90623f9bb883bb253fa6463ee4d34 Convert GetLocal() to std::optional and remove out-param (Jon Atack)
f1304db136bbeac70b52522b5a4522165bfb3fc0 Use higher-level CNetAddr and CNode helpers in net.cpp (Jon Atack)
07f589158835151a7613e4b2a508c0dd61a18fd7 Add CNetAddr::IsPrivacyNet() and CNode::IsConnectedThroughPrivacyNet() (Jon Atack)
df488563b280c63f5d74d5ac0fcb1a2cad489d55 GetLocal() type-safety, naming, const, and formatting cleanups (stickies-v)
fb4265747c9eb022d80c6f2988e574c689130348 Add and use CNetAddr::HasCJDNSPrefix() helper (Jon Atack)
5ba73cd0ee1e661ec4d041ac8ae7a60cfd31f0c0 Move GetLocal() declaration from header to implementation (Jon Atack)
11426f6557ac09489d5e13bf3b9d94fbd5073c8e Move CaptureMessageToFile() declaration from header to implementation (Jon Atack)
deccf1c4848620cfff2a9642c5a6b5acdfbc33bd Move IsPeerAddrLocalGood() declaration from header to implementation (Jon Atack)
Pull request description:
and other improvements noticed while reviewing #27411.
Addresses https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263969104 and https://github.com/bitcoin/bitcoin/pull/27411#discussion_r1263967598.
See commit messages for details.
ACKs for top commit:
achow101:
ACK 4ecfd3eaf434d868455466e001adae4b68515ab8
vasild:
ACK 4ecfd3eaf434d868455466e001adae4b68515ab8
stickies-v:
ACK 4ecfd3eaf434d868455466e001adae4b68515ab8
Tree-SHA512: d792bb2cb24690aeae9bedf97e92b64fb6fd080c39385a4bdb8ed05f37f3134d85bda99da025490829c03017fd56382afe7951cdd039aede1c08ba98fb1aa7f9
|
