* Make PeerLogicValidation final to prevent deriving from it [1]
* Prevent deletions of NetEventsInterface and CValidationInterface
objects via a base class pointer
[1] silences the following compiler warning (from Clang 7.0.0):
/usr/include/c++/v1/memory:2285:5: error: delete called on non-final 'PeerLogicValidation' that has
virtual functions but non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
delete __ptr;
^
/usr/include/c++/v1/memory:2598:7: note: in instantiation of member function
'std::__1::default_delete<PeerLogicValidation>::operator()' requested here
__ptr_.second()(__tmp);
^
init.cpp:201:15: note: in instantiation of member function 'std::__1::unique_ptr<PeerLogicValidation,
std::__1::default_delete<PeerLogicValidation> >::reset' requested here
peerLogic.reset();
^
eb91835 Add setter for g_initial_block_download_completed (Jonas Schnelli)
3f56df5 [QA] add NODE_NETWORK_LIMITED address relay and sync test (Jonas Schnelli)
158e1a6 [QA] fix mininode CAddress ser/deser (Jonas Schnelli)
fa999af [QA] Allow addrman loopback tests (add debug option -addrmantest) (Jonas Schnelli)
6fe57bd Connect to peers signaling NODE_NETWORK_LIMITED when out-of-IBD (Jonas Schnelli)
31c45a9 Accept addresses with NODE_NETWORK_LIMITED flag (Jonas Schnelli)
Pull request description:
Eventually connect to peers signalling NODE_NETWORK_LIMITED if we are out of IBD.
Accept and relay NODE_NETWORK_LIMITED peers in addrman.
Tree-SHA512: 8a238fc97f767f81cae1866d6cc061390f23a72af4a711d2f7158c77f876017986abb371d213d1c84019eef7be4ca951e8e6f83fda36769c4e1a1d763f787037
Adds HasAllRelevantServices and GetRelevantServices, which check
for NETWORK|WITNESS.
This changes the following:
* Removes nRelevantServices from CConnman, disconnecting it a bit
more from protocol-level logic.
* Replaces our sometimes-connect-to-!WITNESS-nodes logic with
simply always requiring WITNESS|NETWORK for outbound non-feeler
connections (feelers still only require NETWORK).
* This has the added benefit of removing nServicesExpected from
CNode - instead letting net_processing's VERSION message
handling simply check HasAllRelevantServices.
* This implies we believe WITNESS nodes to continue to be a
significant majority of nodes on the network, but also because
we cannot sync properly from !WITNESS nodes, it is strange to
continue using our valuable outbound slots on them.
* In order to prevent this change from preventing connection to
-connect= nodes which have !WITNESS, -connect nodes are now
given the "addnode" flag. This also allows outbound connections
to !NODE_NETWORK nodes for -connect nodes (which was already true
of addnodes).
* Has the (somewhat unintended) consequence of changing one of the
eviction metrics from the same
sometimes-connect-to-!WITNESS-nodes metric to requiring
HasRelevantServices.
This should make NODE_NETWORK_LIMITED much simpler to implement.
Use C++11's better capability of expressing an interface of a non-copyable class by publicly deleting its copy ctor and assignment operator instead of just declaring them private.
There are a few too many edge-cases here to make this a scripted diff.
The following commits will move a few functions into PeerLogicValidation, where
the local connman instance can be used. This change prepares for that usage.
352d582ba Add vConnect to CConnman::Options (Marko Bencun)
Pull request description:
Split the "-connect" argument parsing out of CConnman and put it into
AppInitMain().
Tree-SHA512: f2d3efc4e2c5808ff98696ea20dd96df599bc472ed5afc9c3eea305d94c36a6ab50c632aa05396c7c34d1917d91b1e7ccd725656ff2631e2a36d9eac477455dc
When running test_bitcoin under Valgrind I found the following issue:
```
$ valgrind src/test/test_bitcoin
...
==10465== Use of uninitialised value of size 8
==10465== at 0x6D09B61: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==10465== by 0x6D0B1BB: std::ostreambuf_iterator<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<unsigned long>(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, unsigned long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==10465== by 0x6D0B36C: std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::do_put(std::ostreambuf_iterator<char, std::char_traits<char> >, std::ios_base&, char, unsigned long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==10465== by 0x6D17699: std::ostream& std::ostream::_M_insert<unsigned long>(unsigned long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.21)
==10465== by 0x4CAAD7: operator<< (ostream:171)
==10465== by 0x4CAAD7: formatValue<ServiceFlags> (tinyformat.h:345)
==10465== by 0x4CAAD7: void tinyformat::detail::FormatArg::formatImpl<ServiceFlags>(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:523)
==10465== by 0x1924D4: format (tinyformat.h:510)
==10465== by 0x1924D4: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:803)
==10465== by 0x553A55: vformat (tinyformat.h:947)
==10465== by 0x553A55: format<ServiceFlags> (tinyformat.h:957)
==10465== by 0x553A55: std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<ServiceFlags>(char const*, ServiceFlags const&) (tinyformat.h:966)
==10465== by 0x54C952: getnetworkinfo(JSONRPCRequest const&) (net.cpp:462)
==10465== by 0x28EDB5: CallRPC(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >) (rpc_tests.cpp:31)
==10465== by 0x293947: rpc_tests::rpc_togglenetwork::test_method() (rpc_tests.cpp:88)
==10465== by 0x2950E5: rpc_tests::rpc_togglenetwork_invoker() (rpc_tests.cpp:84)
==10465== by 0x182496: invoke<void (*)()> (callback.hpp:56)
==10465== by 0x182496: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
...
```
The read of the uninitialized variable nLocalServices is triggered by g_connman->GetLocalServices()
in getnetworkinfo(const JSONRPCRequest& request) (net.cpp:462):
```c++
UniValue getnetworkinfo(const JSONRPCRequest& request)
{
...
if(g_connman)
obj.push_back(Pair("localservices", strprintf("%016x", g_connman->GetLocalServices())));
...
}
```
The reason for the uninitialized nLocalServices is that CConnman::Start(...) is not called
by the tests, and hence the initialization normally performed by CConnman::Start(...) is
not done.
This commit adds a method Init(const Options& connOptions) which is called by both the
constructor and CConnman::Start(...). This method initializes nLocalServices and the other
relevant values from the supplied Options object.
Identified with `cppcheck --enable=unusedFunction .`.
- GetSendBufferSize()'s last use removed in
991955ee81
- SetPort()'s last use removed in
7e195e8459
- GetfLargeWorkInvalidChainFound() was introduced in
e3ba0ef956 and never used
Part of a series of changes to clean up the instantiation of connman
by decoupling the command line arguments.
We also now abort with an error when explicit binds are set with
-listen=0.
This adds the listening address on which incoming connections were received to the
CNode and CNodeStats structures.
The address is reported in `getpeerinfo`.
This can be useful for distinguishing connections received on different listening ports
(e.g. when using a different listening port for Tor hidden service connections)
or different networks.
381a46e Consensus: Policy: MOVEONLY: Move CFeeRate out of the consensus module (Jorge Timón)
330bb5a Consensus: Minimal way to move dust out of consensus (Jorge Timón)
Tree-SHA512: 19a2ea8169afd5a9d3f940d8974e34cfaead153e3ff3068ac82fccdb8694d19d9b45938904ec9e8cd095bd5ca3a0080364da29372f6aaf56b11a6c2ccd6c7a4d
These are (afaik) all long-standing races or concurrent accesses. Going
forward, we can clean these up so that they're not all individual atomic
accesses.
- Reintroduce cs_vRecv to guard receive-specific vars
- Lock vRecv/vSend for CNodeStats
- Make some vars atomic.
- Only set the connection time in CNode's constructor so that it doesn't change
0729102 Net: pass interruptMsgProc as const where possible (Jorge Timón)
fc7f2ff Net: Make CNetMsgMaker more const (Jorge Timón)
d45955f Net: CConnman: Make some methods const (Jorge Timón)
Since ForEach* are can be used to send messages to all nodes, the caller may
end up sending a message before the version handshake is complete. To limit
this, filter out these nodes. While we're at it, may as well filter out
disconnected nodes as well.
Delete unused methods rather than updating them.
This avoids having some vars set if the version negotiation fails.
Also copy it all into CNode at the same site. nVersion and
fSuccessfullyConnected are set last, as they are the gates for the other vars.
Make them atomic for that reason.
376b3c2 Make the cs_sendProcessing a LOCK instead of a TRY_LOCK (Matt Corallo)
d7c58ad Split CNode::cs_vSend: message processing and message sending (Matt Corallo)
cs_vSend is used for two purposes - to lock the datastructures used
to queue messages to place on the wire and to only call
SendMessages once at a time per-node. I believe SendMessages used
to access some of the vSendMsg stuff, but it doesn't anymore, so
these locks do not need to be on the same mutex, and also make
deadlocking much more likely.
e60360e net: remove cs_vRecvMsg (Cory Fields)
991955e net: add a flag to indicate when a node's send buffer is full (Cory Fields)
c6e8a9b net: add a flag to indicate when a node's process queue is full (Cory Fields)
4d712e3 net: add a new message queue for the message processor (Cory Fields)
c5a8b1b net: rework the way that the messagehandler sleeps (Cory Fields)
c72cc88 net: remove useless comments (Cory Fields)
ef7b5ec net: Add a simple function for waking the message handler (Cory Fields)
f5c36d1 net: record bytes written before notifying the message processor (Cory Fields)
60befa3 net: handle message accounting in ReceiveMsgBytes (Cory Fields)
56212e2 net: set message deserialization version when it's actually time to deserialize (Cory Fields)
0e973d9 net: remove redundant max sendbuffer size check (Cory Fields)
6042587 net: wait until the node is destroyed to delete its recv buffer (Cory Fields)
f6315e0 net: only disconnect if fDisconnect has been set (Cory Fields)
5b4a8ac net: make GetReceiveFloodSize public (Cory Fields)
e5bcd9c net: make vRecvMsg a list so that we can use splice() (Cory Fields)
53ad9a1 net: fix typo causing the wrong receive buffer size (Cory Fields)
vRecvMsg is now only touched by the socket handler thread.
The accounting vars (nRecvBytes/nLastRecv/mapRecvBytesPerMsgCmd) are also
only used by the socket handler thread, with the exception of queries from
rpc/gui. These accesses are not threadsafe, but they never were. This needs to
be addressed separately.
Also, update comment describing data flow
Similar to the recv flag, but this one indicates whether or not the net's send
buffer is full.
The socket handler checks the send queue when a new message is added and pauses
if necessary, and possibly unpauses after each message is drained from its buffer.
Messages are dumped very quickly from the socket handler to the processor, so
it's the depth of the processing queue that's interesting.
The socket handler checks the process queue's size during the brief message
hand-off and pauses if necessary, and the processor possibly unpauses each time
a message is popped off of its queue.
In order to sleep accurately, the message handler needs to know if _any_ node
has more processing that it should do before the entire thread sleeps.
Rather than returning a value that represents whether ProcessMessages
encountered a message that should trigger a disconnnect, interpret the return
value as whether or not that node has more work to do.
Also, use a global fProcessWake value that can be set by other threads,
which takes precedence (for one cycle) over the messagehandler's decision.
Note that the previous behavior was to only process one message per loop
(except in the case of a bad checksum or invalid header). That was changed in
PR #3180.
The only change here in that regard is that the current node now falls to the
back of the processing queue for the bad checksum/invalid header cases.
Previously addnodes were in competition with outbound connections
for access to the eight outbound slots.
One result of this is that frequently a node with several addnode
configured peers would end up connected to none of them, because
while the addnode loop was in its two minute sleep the automatic
connection logic would fill any free slots with random peers.
This is particularly unwelcome to users trying to maintain links
to specific nodes for fast block relay or purposes.
Another result is that a group of nine or more nodes which are
have addnode configured towards each other can become partitioned
from the public network.
This commit introduces a new limit of eight connections just for
addnode peers which is not subject to any of the other connection
limitations (including maxconnections).
The choice of eight is sufficient so that under no condition would
a user find themselves connected to fewer addnoded peers than
previously. It is also low enough that users who are confused
about the significance of more connections and have gotten too
copy-and-paste happy will not consume more than twice the slot
usage of a typical user.
Any additional load on the network resulting from this will likely
be offset by a reduction in users applying even more wasteful
workaround for the prior behavior.
The retry delays are reduced to avoid nodes sitting around without
their added peers up, but are still sufficient to prevent overly
aggressive repeated connections. The reduced delays also make
the system much more responsive to the addnode RPC.
Ban-disconnects are also exempted for peers added via addnode since
the outbound addnode logic ignores bans. Previously it would ban
an addnode then immediately reconnect to it.
A minor change was also made to CSemaphoreGrant so that it is
possible to re-acquire via an object whos grant was moved.
This fixes one of the last major layer violations in the networking stack.
The network side is no longer in charge of message serialization, so it is now
decoupled from Bitcoin structures. Only the header is serialized and attached
to the payload.
CVectorWriter is useful for overwriting or appending an existing byte vector.
CNetMsgMaker is a shortcut for creating messages on-the-fly which are suitable
for pushing to CConnman.
d59a518 Use fixed preallocation instead of costly GetSerializeSize (Pieter Wuille)
25a211a Add optimized CSizeComputer serializers (Pieter Wuille)
a2929a2 Make CSerAction's ForRead() constexpr (Pieter Wuille)
a603925 Avoid -Wshadow errors (Pieter Wuille)
5284721 Get rid of nType and nVersion (Pieter Wuille)
657e05a Make GetSerializeSize a wrapper on top of CSizeComputer (Pieter Wuille)
fad9b66 Make nType and nVersion private and sometimes const (Pieter Wuille)
c2c5d42 Make streams' read and write return void (Pieter Wuille)
50e8a9c Remove unused ReadVersion and WriteVersion (Pieter Wuille)
Remove the nType and nVersion as parameters to all serialization methods
and functions. There is only one place where it's read and has an impact
(in CAddress), and even there it does not impact any of the recursively
invoked serializers.
Instead, the few places that need nType or nVersion are changed to read
it directly from the stream object, through GetType() and GetVersion()
methods which are added to all stream classes.
The changes here are dense and subtle, but hopefully all is more explicit
than before.
- CConnman is now in charge of sending data rather than the nodes themselves.
This is necessary because many decisions need to be made with all nodes in
mind, and a model that requires the nodes calling up to their manager quickly
turns to spaghetti.
- The per-node-serializer (ssSend) has been replaced with a (quasi-)const
send-version. Since the send version for serialization can only change once
per connection, we now explicitly tag messages with INIT_PROTO_VERSION if
they are sent before the handshake. With this done, there's no need to lock
for access to nSendVersion.
Also, a new stream is used for each message, so there's no need to lock
during the serialization process.
- This takes care of accounting for optimistic sends, so the
nOptimisticBytesWritten hack can be removed.
- -dropmessagestest and -fuzzmessagestest have not been preserved, as I suspect
they haven't been used in years.
