| Age | Commit message (Collapse) | Author |
|---|
|
-onlynet but are unreachable
68209a7b5c0326e14508d9cf749771605bd6ffe7 rpc: make addpeeraddress work with cjdns addresses (Martin Zumsande)
a8a9ed67cc447d204304ccfd844c45fd76486c6a init: Abort if i2p/cjdns are chosen via -onlynet but unreachable (Martin Zumsande)
Pull request description:
If the networks i2p / cjdns are chosen via `-onlynet` but the user forgot to provide `-i2psam` / `-cjdnsreachable`, no outbound connections will be made - it would be nice to inform the user about that.
The solution proposed here mimics existing behavior for `-onlynet=onion` and non-specified `-onion`/`-proxy` where we already abort with an InitError - if reviewers would prefer to just print a warning, please say so.
The second commit adds CJDNS support to the debug-only `addpeeraddress` RPC allowing to add CJDNS addresses to addrman for testing and debug purposes. (if `-cjdnsreachable=1`)
This is the result of an [IRC discussion](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2022-09-01#848066;) with vasild.
ACKs for top commit:
vasild:
ACK 68209a7b5c0326e14508d9cf749771605bd6ffe7
dergoegge:
ACK 68209a7b5c0326e14508d9cf749771605bd6ffe7
Tree-SHA512: 6db9787f01820190f14f90a0b39e4206603421eb7521f792879094d8bbf4d4d0bfd70665eadcc40994ac7941a15ab5a8d65c4779fba5634c0e6fa66eb0972b8d
|
|
This allows us to add cjdns addresses to addrman for
testing and debug purposes (if -cjdnsreachable is true)
|
|
SendMessages() is now protected g_msgproc_mutex; so this additional
per-node mutex is redundant.
|
|
There are many cases where we assume message processing is
single-threaded in order for how we access node-related memory to be
safe. Add an explicit mutex that we can use to document this, which allows
the compiler to catch any cases where we try to access that memory from
other threads and break that assumption.
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/permissionFlags/permission_flags/g' $(git grep -l permissionFlags)
-END VERIFY SCRIPT-
|
|
|
|
|
|
|
|
m_permissionFlags and m_prefer_evict are treated as const -- they're
only set immediately after construction before any other thread has
access to the object, and not changed again afterwards. As such they
don't need to be marked atomic or guarded by a mutex; though it would
probably be better to actually mark them as const...
|
|
Dereferencing a unique_ptr is not necessarily thread safe. The reason
these are safe is because their values are set at construction and do
not change later; so mark them as const and set them via the initializer
list to guarantee that.
|
|
The (V1)TransportSerializer instance CNode::m_serializer is used from
multiple threads via PushMessage without protection by a mutex. This
is only thread safe because the class does not have any mutable state,
so document that by marking the methods and the object as "const".
|
|
|
|
If not accepting I2P connections, then do not create
`CConnman::m_i2p_sam_session`.
When opening a new outbound I2P connection either use
`CConnman::m_i2p_sam_session` like before or create a temporary one and
store it in `CNode` for destruction later.
|
|
and destroy it when `CNode::m_sock` is closed.
I2P transient sessions are created per connection (i.e. per `CNode`) and
should be destroyed when the connection is closed. Storing the session
in `CNode` is a convenient way to destroy it together with the connection
socket (`CNode::m_sock`).
An alternative approach would be to store a list of all I2P sessions in
`CConnman` and from `CNode::CloseSocketDisconnect()` to somehow ask the
`CConnman` to destroy the relevant session.
|
|
|
|
|
|
|
|
Use Peer::m_their_services instead
|
|
fClient is replaced by CanServeBlocks(), and m_limited_node is replaced
by IsLimitedPeer().
|
|
Track services offered by us and the peer in the Peer object.
|
|
|
|
|
|
|
|
|
|
mockable Sock::WaitMany()
6e68ccbefea6509c61fc4405a391a517c6057bb0 net: use Sock::WaitMany() instead of CConnman::SocketEvents() (Vasil Dimov)
ae263460bab9e6aa112dc99790c8ef06a56ec838 net: introduce Sock::WaitMany() (Vasil Dimov)
cc74459768063a923fb6220a4f420eaf211aee7b net: also wait for exceptional events in Sock::Wait() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
`Sock::Wait()` waits for IO events on one socket. Introduce a similar `virtual` method `WaitMany()` that waits simultaneously for IO events on more than one socket.
Use `WaitMany()` instead of `CConnman::SocketEvents()` (and ditch the latter). Given that the former is a `virtual` method, it can be mocked by unit and fuzz tests. This will help to make bigger parts of `CConnman` testable (unit and fuzz).
ACKs for top commit:
laanwj:
Code review ACK 6e68ccbefea6509c61fc4405a391a517c6057bb0
jonatack:
re-ACK 6e68ccbefea6509c61fc4405a391a517c6057bb0 per `git range-diff e18fd47 6747729 6e68ccb`, and verified rebase to master and debug build
Tree-SHA512: 917fb6ad880d64d3af1ebb301c06fbd01afd8ff043f49e4055a088ebed6affb7ffe1dcf59292d822f10de5f323b6d52d557cb081dd7434634995f9148efcf08f
|
|
Rename `GenerateSelectSet()` to `GenerateWaitSockets()` and adapt it to
generate a wait data suitable for `Sock::WaitMany()`. Then call it from
`CConnman::SocketHandler()` and feed the generated data to
`Sock::WaitMany()`.
This way `CConnman::SocketHandler()` can be unit tested because
`Sock::WaitMany()` is mockable, so the usage of real sockets can be
avoided.
Resolves https://github.com/bitcoin/bitcoin/issues/21744
|
|
-BEGIN VERIFY SCRIPT-
sed -i -E -e '/^([a-z]+ )?Mutex [a-z]/ s/Mutex/GlobalMutex/' $(git grep -lE '^([a-z]+ )?Mutex [a-z]')
-END VERIFY SCRIPT-
|
|
436ce0233c276e263dcb441255dc0b881cb39cfb sync.h: strengthen AssertLockNotHeld assertion (Anthony Towns)
7d73f58e9cea8f4b0bc16512983898fddde3d764 Increase threadsafety annotation coverage (Anthony Towns)
Pull request description:
This changes `AssertLockNotHeld` so that it is annotated with the negative capability for the mutex it refers to. clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.
Note that this can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex. At present, the only global mutexes that use `AssertLockNotHeld` are `RecursiveMutex` so we treat that as an exception in order to avoid having to add an excessive number of negative annotations.
ACKs for top commit:
vasild:
ACK 436ce0233c276e263dcb441255dc0b881cb39cfb
MarcoFalke:
review ACK 436ce0233c276e263dcb441255dc0b881cb39cfb 🌺
Tree-SHA512: 5f16d098790a36b5277324d5ee89cdc87033c19b11c7943c2f630a41c2e3998eb39d356a763e857f4d8fefb6c0c02291f720bb6769bcbdf5e2cd765bf266ab8c
|
|
where all the other logging actions in src/net.{h,cpp} are located.
StartExtraBlockRelayPeers() does not appear to be a hotspot that needs to be
inlined for performance, as it is called from CheckForStaleTipAndEvictPeers(),
called in turn from StartScheduledTasks() with a scheduleEvery delta of 45
seconds, called at the end of AppInitMain() on bitcoind startup.
This allows dropping `#include <logging.h>` from net.h, which can improve
compile time/speed. Currently, none of the other includes in net.h use
logging.h, except src/sync.h if DEBUG_LOCKCONTENTION is defined.
|
|
|
|
and src/rpc/net.cpp
e71c51b27d420fbd6cc0a36f62e63e190e13473a refactor: rename command -> message type in comments in the src/net* files (Shashwat)
2b09593bddb0a93aebf84e5f43cdb4d5282c7984 scripted-diff: Rename message command to message type (Shashwat)
Pull request description:
This PR is a follow-up to #24078.
> a message is not a command, but simply a message of some type
The first commit covers the message_command variable name and comments not addressed in the original PR in `src/net*` files.
The second commit goes beyond the original `src/net*` limit of #24078 and does similar changes in the `src/rpc/net.cpp` file.
ACKs for top commit:
MarcoFalke:
review ACK e71c51b27d420fbd6cc0a36f62e63e190e13473a 💥
Tree-SHA512: 24015d132c00f15239e5d3dc7aedae904ae3103a90920bb09e984ff57723402763f697d886322f78e42a0cb46808cb6bc9d4905561dc6ddee9961168f8324b05
|
|
with Mutex and rename it
709af67add93f6674fb80e3ae8e3f175653a62f0 p2p: replace RecursiveMutex `m_total_bytes_sent_mutex` with Mutex (w0xlt)
8be75fd0f0039eeea5f9af7c1eb17c584ed9f507 p2p: add assertions and negative TS annotations for `m_total_bytes_sent_mutex` (w0xlt)
a237a065cc2c6337e3797cc30a0f84c56c6d2f3b scripted-diff: rename cs_totalBytesSent -> m_total_bytes_sent_mutex (w0xlt)
Pull request description:
Related to #19303, this PR gets rid of the RecursiveMutex `cs_totalBytesSent` and also adds `AssertLockNotHeld` macros combined with `LOCKS_EXCLUDED` thread safety annotations to avoid recursive locking.
ACKs for top commit:
jonatack:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0 per `git range-diff 7a4ac71 eff7918 709af67`, rebase to master, clang 15 debug build, and build with -Wthread-safety-negative
vasild:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0
hebasto:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0, tested on Ubuntu 22.04.
Tree-SHA512: 560b4e6c92b1511911d69185207df6ee809db09b96d97f96430d8d2595dc05c98cc691aaec8a58ef87cf2ab0a98675c210b8ce0be3dedb81e31114bbbfdfd8be
|
|
|
|
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/cs_totalBytesSent/m_total_bytes_sent_mutex/g' -- $(git grep --files-with-matches 'cs_totalBytesSent')
-END VERIFY SCRIPT-
|
|
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
|
|
-BEGIN VERIFY SCRIPT-
s1() { sed -i "s/$1/$2/g" $(git grep -l "$1" ./); }
s1 'NET_MESSAGE_COMMAND_OTHER' 'NET_MESSAGE_TYPE_OTHER'
s1 'mapMsgCmdSize' 'mapMsgTypeSize'
s1 'mapRecvBytesPerMsgCmd' 'mapRecvBytesPerMsgType'
s1 'mapSendBytesPerMsgCmd' 'mapSendBytesPerMsgType'
s1 'recvPerMsgCmd' 'recvPerMsgType'
s1 'sendPerMsgCmd' 'sendPerMsgType'
-END VERIFY SCRIPT-
|
|
|
|
net_processing
1066d10f71e6800c78012d789ff6ae19df0243fe scripted-diff: rename TxRelay members (John Newbery)
575bbd0dea6d12510fdf3220d0f0e47d969da6e9 [net processing] Move tx relay data to Peer (John Newbery)
785f55f7eeab0dedbeb8e0d0b459f3bdc538b621 [net processing] Move m_wtxid_relay to Peer (John Newbery)
36346703f8558d6781c079c29ddece5a97477beb [net] Add CNode.m_relays_txs and CNode.m_bloom_filter_loaded (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing, by moving all tx data into the new Peer object added in #19607.
For motivation, see #19398.
ACKs for top commit:
dergoegge:
ACK 1066d10f71e6800c78012d789ff6ae19df0243fe - This is a good layer separation improvement with no behavior changes.
glozow:
utACK 1066d10f71e6800c78012d789ff6ae19df0243fe
Tree-SHA512: 0c9d6b8a0a05e2d816b6d6588b7df133842ec960ae67667813422aa7bd8eb5308599c714f3822a98ddbdf364ffab9050b055079277ba4aff24092557ff99ebcc
|
|
This makes code that uses the helper less verbose.
Moreover, this makes net_processing C++20 compliant. Otherwise, it would
lead to a compile error (see below). C++20 disables aggregate
initialization when any constructor is declared. See
http://open-std.org/JTC1/SC22/WG21/docs/papers/2018/p1008r1.pdf
net_processing.cpp:1627:42: error: no matching constructor for initialization of 'CSerializedNetMsg'
m_connman.PushMessage(pnode, CSerializedNetMsg{ser_cmpctblock.data, ser_cmpctblock.m_type});
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
|
-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-
|
|
|
|
We'll move the transaction relay data into Peer in subsequent commits,
but the inbound eviction logic needs to know if the peer is relaying
txs and if the peer has loaded a bloom filter.
This is currently redundant information with m_tx_relay->fRelayTxes,
but when m_tx_relay is moved into net_processing, then we'll need these
separate fields in CNode.
|
|
If `-bind=` is provided then we would bind only to a particular address
and should not add all the other addresses of the machine to the list of
local addresses.
Fixes https://github.com/bitcoin/bitcoin/issues/20184 (case 4.)
|
|
Span is lightweight and need not be passed by const reference.
|
|
Rename `CaptureMessage()` to `CaptureMessageToFile()` and introduce a
`std::function` variable called `CaptureMessage` whose value can be
changed by unit tests, should they need to inspect message contents.
|
|
network to CJDNS peers
b7be28cac50046b9f2ddfe63ecafccc80649a36c test: add combined CJDNS/I2P/localhost/onion eviction protection tests (Jon Atack)
0a1bb84770b403ab5cbd9d5474c76f91ce58e8f6 test: add tests for inbound eviction protection of CJDNS peers (Jon Atack)
0c00c0c981fc0b6cec101e68e8c1aeda1ccf33bb test: fix off-by-one logic in an eviction protection test (Jon Atack)
f7b8094d611531c6b41a94715dbc01f56257ccd2 p2p: extend inbound eviction protection by network to CJDNS peers (Jon Atack)
Pull request description:
Extend inbound eviction protection for peers connected over CJDNS, as is the case for peers connected via onion, localhost, and I2P since #21261 and #20197. CJDNS peers seem to have better min ping latency than onion and I2P peers but still higher than that of unencrypted IPv4/6 peers and can be disadvantaged under our eviction criteria. They are also very few in number, which is a further reason to protect them, as the goal of this logic is to favorise the diversity of our peer connections. CJDNS support was added in #23077 for the upcoming v23 release.
ACKs for top commit:
laanwj:
Concept and code review ACK b7be28cac50046b9f2ddfe63ecafccc80649a36c
w0xlt:
tACK b7be28c
Tree-SHA512: 89ebdd217602e16ae14b9bd0d5a25fc09f9b2384c951f820bc0f5a6d8452bbc9042065db817d5d5296c0ad22988491a83fc5b9a611e660c40ebd4f03448c4061
|
|
ef5014d256638735b292672c774446db4003f03b style: wrap long lines in CNode creation and add some comments (Vasil Dimov)
b68349164827f14c472201cad54c4e19a3321261 scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex (Vasil Dimov)
c41a1162ac4da437c5d755e8fe2bf636bed22b0f net: use Sock in CNode (Vasil Dimov)
c5dd72e146dd8fa77d29c8689a42322a4d1ec780 fuzz: move FuzzedSock earlier in src/test/fuzz/util.h (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Change `CNode` to use a pointer to `Sock` instead of a bare `SOCKET`.
This will help mocking / testing / fuzzing more code.
ACKs for top commit:
jonatack:
re-ACK ef5014d256638735b292672c774446db4003f03b changes since last review are the removal of an unneeded dtor and the addition of a style commit
w0xlt:
reACK ef5014d
PastaPastaPasta:
utACK ef5014d256638735b292672c774446db4003f03b, I have reviewed the code, and believe it makes sense to merge
theStack:
Cod-review ACK ef5014d256638735b292672c774446db4003f03b
Tree-SHA512: 7f5414dd339cd2f16f7cbdc5fcec238d68b6d50072934aea10b901f409da28ff1ece6db6e899196616aa8127b8b25ab5b86d000bdcee58b4cadd7a3c1cf560c5
|
|
-BEGIN VERIFY SCRIPT-
sed -i -e 's/cs_hSocket/m_sock_mutex/g' $(git grep -l cs_hSocket)
-END VERIFY SCRIPT-
|
