| Age | Commit message (Collapse) | Author |
|---|
|
16bd283b3ad05daa41259a062aee0fc05b463fa6 Reapply "test: p2p: check that connecting to ourself leads to disconnect" (Sebastian Falbesoner)
0dbcd4c14855fe2cba15a32245572b693dc18c4e net: prevent sending messages in `NetEventsInterface::InitializeNode` (Sebastian Falbesoner)
66673f1c1302c986e344c7f44bb0b352213d5dc8 net: fix race condition in self-connect detection (Sebastian Falbesoner)
Pull request description:
This PR fixes a recently discovered race condition in the self-connect detection (see #30362 and #30368).
Initiating an outbound network connection currently involves the following steps after the socket connection is established (see [`CConnman::OpenNetworkConnection`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net.cpp#L2923-L2930) method):
1. set up node state
2. queue VERSION message (both steps 1 and 2 happen in [`InitializeNode`](https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/net_processing.cpp#L1662-L1683))
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message (step 2) is received and processed locally *before* the node object is added to the connection manager's `m_nodes` vector (step 3). In this case, the self-connect remains undiscovered, as the detection doesn't find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion` call out of `InitializeNode` and doing that in the `SendMessages` method instead, which is only called for `CNode` instances in `m_nodes`.
The temporarily reverted test introduced in #30362 is readded. Fixes #30368.
Thanks go to vasild, mzumsande and dergoegge for suggestions on how to fix this (see https://github.com/bitcoin/bitcoin/issues/30368#issuecomment-2200625017 ff. and https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789).
ACKs for top commit:
naiyoma:
tested ACK [https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6](https://github.com/bitcoin/bitcoin/pull/30394/commits/16bd283b3ad05daa41259a062aee0fc05b463fa6), built and tested locally, test passes successfully.
mzumsande:
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
tdb3:
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
glozow:
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
dergoegge:
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6
Tree-SHA512: 5b8aced6cda8deb38d4cd3fe4980b8af505d37ffa0925afaa734c5d81efe9d490dc48a42e1d0d45dd2961c0e1172a3d5b6582ae9a2d642f2592a17fbdc184445
|
|
Initiating an outbound network connection currently involves the
following steps after the socket connection is established (see
`CConnman::OpenNetworkConnection` method):
1. set up node state
2. queue VERSION message
3. add new node to vector `m_nodes`
If we connect to ourself, it can happen that the sent VERSION message
(step 2) is received and processed locally *before* the node object
is added to the connection manager's `m_nodes` vector (step 3). In this
case, the self-connect remains undiscovered, as the detection doesn't
find the outbound peer in `m_nodes` yet (see `CConnman::CheckIncomingNonce`).
Fix this by swapping the order of 2. and 3., by taking the `PushNodeVersion`
call out of `InitializeNode` and doing that in the `SendMessages` method
instead, which is only called for `CNode` instances in `m_nodes`.
Thanks go to vasild, mzumsande, dergoegge and sipa for suggestions on
how to fix this.
|
|
Block was recently touched (e2d1f84858485650ff743753ffa5c679f210a992) and the codebase recently switched to C++20 which allows this to improve robustness.
|
|
The existing code uses GetRand(nMax), with a default value for nMax, where nMax is the
range of values (not the maximum!) that the output is allowed to take. This will always
miss the last possible value (e.g. GetRand<uint32_t>() will never return 0xffffffff).
Fix this, by moving the functionality largely in RandomMixin, and also adding a
separate RandomMixin::rand function, which returns a value in the entire (non-negative)
range of an integer.
|
|
The nVersion field is unused, so remove it.
This is also required for future commits.
Also, add PushMessage aliases in PeerManagerImpl to make calling code
less verbose.
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
|
|
that are otherwise private:
- CConnman::m_nodes
- CConnman::ConnectNodes()
- CConnman::AlreadyConnectedToAddress()
and update the #include headers per iwyu.
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/WithParams(\(CAddress::V[12]_[A-Z]*\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's/WithParams(\(CNetAddr::V[12]\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's@\(CNetAddr::V1.CService{}.*\) //@\1 //@' src/test/util/net.cpp
-END VERIFY SCRIPT-
|
|
Before this commit, there are only two possibly outcomes for the "more" prediction
in Transport::GetBytesToSend():
* true: the transport itself has more to send, so the answer is certainly yes.
* false: the transport has nothing further to send, but if vSendMsg has more message(s)
left, that still will result in more wire bytes after the next
SetMessageToSend().
For the BIP324 v2 transport, there will arguably be a third state:
* definitely not: the transport has nothing further to send, but even if vSendMsg has
more messages left, they can't be sent (right now). This happens
before the handshake is complete.
To implement this, we move the entire decision logic to the Transport, by adding a
boolean to GetBytesToSend(), called have_next_message, which informs the transport
whether more messages are available. The return values are still true and false, but
they mean "definitely yes" and "definitely no", rather than "yes" and "maybe".
|
|
This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
|
|
This furthers transport abstraction by removing the assumption that a message
can always immediately be converted to wire bytes. This assumption does not hold
for the v2 transport proposed by BIP324, as no messages can be sent before the
handshake completes.
This is done by only keeping (complete) CSerializedNetMsg objects in vSendMsg,
rather than the resulting bytes (for header and payload) that need to be sent.
In SocketSendData, these objects are handed to the transport as permitted by it,
and sending out the bytes the transport tells us to send. This also removes the
nSendOffset member variable in CNode, as keeping track of how much has been sent
is now a responsability of the transport.
This is not a pure refactor, and has the following effects even for the current
v1 transport:
* Checksum calculation now happens in SocketSendData rather than PushMessage.
For non-optimistic-send messages, that means this computation now happens in
the network thread rather than the message handler thread (generally a good
thing, as the message handler thread is more of a computational bottleneck).
* Checksum calculation now happens while holding the cs_vSend lock. This is
technically unnecessary for the v1 transport, as messages are encoded
independent from one another, but is untenable for the v2 transport anyway.
* Statistics updates about per-message sent bytes now happen when those bytes
are actually handed to the OS, rather than at PushMessage time.
|
|
This makes the sending side of P2P transports mirror the receiver side: caller provides
message (consisting of type and payload) to be sent, and then asks what bytes must be
sent. Once the message has been fully sent, a new message can be provided.
This removes the assumption that P2P serialization of messages follows a strict structure
of header (a function of type and payload), followed by (unmodified) payload, and instead
lets transports decide the structure themselves.
It also removes the assumption that a message must always be sent at once, or that no
bytes are even sent on the wire when there is no message. This opens the door for
supporting traffic shaping mechanisms in the future.
|
|
This allows state that is shared between both directions to be encapsulated
into a single object. Specifically the v2 transport protocol introduced by
BIP324 has sending state (the encryption keys) that depends on received
messages (the DH key exchange). Having a single object for both means it can
hide logic from callers related to that key exchange and other interactions.
|
|
cd0c8eeb0940790b6ba83786d1c9e362d4dc4829 [net] Pass nRecvFloodSize to CNode (dergoegge)
860402ef2ed728ef096dda4e65e77d566782209f [net] Remove trivial GetConnectionType() getter (dergoegge)
b5a85b365a4abd98176b0935015dbb502cc3e6f6 [net] Delete CNetMessage copy constructor/assignment op (dergoegge)
Pull request description:
Follow-up PR for #27257
* Deletes the copy constructor/assignment operator of `CNetMessage`
* Removes trivial getter for the connection type
* Avoids passing `nRecvFloodSize` to CNode methods by passing it to `CNode` on creation
ACKs for top commit:
jnewbery:
utACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829
theStack:
ACK cd0c8eeb0940790b6ba83786d1c9e362d4dc4829
Tree-SHA512: 673a758668617f69fba77e61f0eaa1538da27a4849c82c98742436692baa2d7f001129af3e7a66b160e599d12109dac08137a146f10ff9b9ebdc5c2237311d41
|
|
|
|
related fixes
03ec5b6f9ca3af28c9ce25cf2393e28ae852d808 clang-tidy: Exclude `performance-*` checks rather including them (Hennadii Stepanov)
24004372302adfc0e7cb36f8db6830694bf050e9 clang-tidy: Add `performance-type-promotion-in-math-fn` check (Hennadii Stepanov)
7e975e6cf86617346c1d8e2568f74a0252c03857 clang-tidy: Add `performance-inefficient-vector-operation` check (Hennadii Stepanov)
516b75f66ec3ba7495fc028c750937bd66cc9bba clang-tidy: Add `performance-faster-string-find` check (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
martinus:
ACK 03ec5b6f9ca3af28c9ce25cf2393e28ae852d808
TheCharlatan:
re-ACK [03ec5b6](https://github.com/bitcoin/bitcoin/pull/26642/commits/03ec5b6f9ca3af28c9ce25cf2393e28ae852d808)
Tree-SHA512: 2dfa52f9131da88826f32583bfd534a56a998477db9804b7333c0e7ac0b6b36141009755c7163b9f95d0ecbf5c2cb63f8a69ce4b114bb83423faed21b50cec67
|
|
https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html
|
|
|
|
At the time when
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it);
```
is called, `it` is certainly `pnode->vRecvMsg.end()` which makes the
call equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), pnode->vRecvMsg.end());
```
which is equivalent to:
```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg);
```
Thus, use the latter. Further, maybe irrelevant, but the latter has
constant complexity while the original code is `O(length of vRecvMsg)`.
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
SendMessages() is now protected g_msgproc_mutex; so this additional
per-node mutex is redundant.
|
|
|
|
|
|
Use Peer::m_their_services instead
|
|
Track services offered by us and the peer in the Peer object.
|
|
|
|
|
|
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }
ren cs_filter m_bloom_filter_mutex
ren fRelayTxes m_relay_txs
ren pfilter m_bloom_filter
ren cs_tx_inventory m_tx_inventory_mutex
ren filterInventoryKnown m_tx_inventory_known_filter
ren setInventoryTxToSend m_tx_inventory_to_send
ren fSendMempool m_send_mempool
ren nNextInvSend m_next_inv_send_time
ren minFeeFilter m_fee_filter_received
ren lastSentFeeFilter m_fee_filter_sent
-END VERIFY SCRIPT-
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
* 2020: fa0074e2d82928016a43ca408717154a1c70a4db
* 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
-BEGIN VERIFY SCRIPT-
ren() { sed -i "s/\<$1\>/$2/g" $( git grep -l "$1" ./src/ ) ; }
ren nLastBlockTime m_last_block_time
ren nLastTXTime m_last_tx_time
ren nTimeConnected m_connected
-END VERIFY SCRIPT-
|
|
|
|
-BEGIN VERIFY SCRIPT-
perl -0777 -pi -e 's:((\(|\{|,)(\n| )*)\/\* ?([^=* ]+) ?\*\/ ?:\1/*\4=*/:g' $( git ls-files ./src/test ./src/wallet/test )
-END VERIFY SCRIPT-
|
|
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
|
|
|
|
|
|
|
