This is an alternative approach to #11534. Rather than disconnect an outbound peer when our tip looks stale, instead try to connect to an additional outbound peer.
Periodically, check to see if we have more outbound peers than we target (ie if any extra peers are in use), and if so, disconnect the one that least recently announced a new block (breaking ties by choosing the newest peer that we connected to).
2953+ // If we have more outbound peers than we target, disconnect one.
2954+ // Pick the outbound peer that least recently announced
2955+ // us a new block, with ties broken by choosing the more recent
2956+ // connection (higher node id)
2957+ NodeId worst_peer = -1;
2958+ int64_t oldest_block_announcement = time_in_seconds;
Regarding the scheduler, I mentioned this on IRC to @gmaxwell:
gmaxwell: my understanding of the scheduler is that it’ll start scheduling callbacks at some point after startup, so i wouldn’t expect the network to be completely synced gmaxwell: also there’s random drift, since the scheduler schedules the next callback N milliseconds after the prior one finishes but worth discussing whether the interval is short enough that there still might be too much concentration of everyone trying to find a new peer
I updated this PR with a unit test.
258+ void SetTryNewOutboundPeer(bool flag);
259+ bool GetTryNewOutboundPeer();
260+
261+ // Return the number of outbound peers we have in excess of our target (eg,
262+ // if we previously called SetTryNewOutboundPeer(true), and have since set
263+ // to false, we may have extra peers that we wish to disconnect).
1067@@ -1068,7 +1068,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
1068 SOCKET hSocket = accept(hListenSocket.socket, (struct sockaddr*)&sockaddr, &len);
1069 CAddress addr;
1070 int nInbound = 0;
1071- int nMaxInbound = nMaxConnections - (nMaxOutbound + nMaxFeeler);
1072+ int nMaxInbound = nMaxConnections - (nMaxOutbound + nMaxFeeler + nMaxExtraOutbound);
it looks like we wont ever have both a feeler and an extra outbound at one time
We will in general have both a feeler and an extra outbound at the same time. There was an incorrect comment (from a first iteration of this patch) describing the extra outbound peer as “stealing” the feeler connection, but that is not actually true. I’ll delete that comment.
1821+
1822+ bool extra_outbound = GetTryNewOutboundPeer();
1823+
1824+ int desired_outbound = nMaxOutbound + (extra_outbound ? nMaxExtraOutbound : 0);
1825+
1826+ if (nOutbound >= desired_outbound) {
425@@ -413,6 +426,10 @@ class CConnman
426 std::thread threadOpenAddedConnections;
427 std::thread threadOpenConnections;
428 std::thread threadMessageHandler;
429+
430+ /** flag for stealing a feeler connection to find a new outbound peer */
2398@@ -2382,6 +2399,10 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
2399 // because it is set in UpdateBlockAvailability. Some nullptr checks
2400 // are still present, however, as belt-and-suspenders.
2401
2402+ if (received_new_header && pindexLast->nChainWork > chainActive.Tip()->nChainWork) {
2403+ nodestate->m_last_block_announcement = GetTime();
442+ g_last_tip_update = GetTime();
443+ }
444+ return g_last_tip_update < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty();
445+}
446+
447+// Requires cs_main
2981+ if (worst_peer != -1) {
2982+ connman->ForNode(worst_peer, [&](CNode *pnode) {
2983+ // Only disconnect a peer that has been connected to us for
2984+ // some reasonable fraction of our check-frequency, to give
2985+ // it time for new information to have arrived.
2986+ if (time_in_seconds - pnode->nTimeConnected > STALE_CHECK_INTERVAL/2) {
Maybe almost always, but definitely not always – our outbound peers can initiate disconnects, too, and it might take us a while to find one.
I’ve pushed up changes to split the peer eviction onto a shorter timer (2.5 minutes), where we only require 30 seconds of connectivity to disconnect.
@TheBlueMatt raised concerns about holding the 9th connection for too long. One concern would be if we are reducing the number of available inbound slots on the network by too much. Another potentially more serious concern is that if we’re holding the 9th connection long enough for a new block to be found and relayed, then over time we’ll be selecting for peers with the fastest connectivity.
In the extreme, you could imagine that this algorithm would reduce to nodes grinding away, trying to connect to the peer that was closest to the miners (or the FIBRE network or similar), which is probably not what we want.
In this case, we only intend for the 9th connection to tell us whether we’re really behind (eg because our existing peers are all broken) or if the network is just slow. We can determine that very quickly after connection, so I think I’m going to change this so that we disconnect extra peers more quickly (ie on a faster timer than every 10 minutes, and requiring a shorter connection time), and reset the TryExtraOutboundPeer flag in CConnman after disconnecting an extra peer, essentially waiting until we decide at the next stale-tip-check that we’re still stale, before trying another 9th peer.
1709+// Exclude peers that are marked for disconnect, or are going to be
1710+// disconnected soon (eg one-shots and feelers)
1711+// Also exclude peers that haven't finished initial connection handshake yet
1712+// (so that we don't decide we're over our desired connection limit, and then
1713+// evict some peer that has finished the handshake)
1714+int CConnman::GetExtraOutboundCount()
Any thoughts on @TheBlueMatt’s suggestion (mentioned on IRC yesterday) to just use the feeler connection for the extra peer, rather than initiate a potential 10th connection?
https://botbot.me/freenode/bitcoin-core-dev/2017-10-27/?msg=92828477&page=2
I’m leaning towards doing that, so nMaxExtraOutbound would go away. I guess this function wouldn’t change though.
Without seeing the code, I think that would probably be simpler and more obvious.
I wonder if we could actually combine the behavior with feelers. Launch a feeler every 20 min or so as usual, and rather than disconnecting immediately, replace a worse connection if necessary, and upgrade from feeler to full outbound.
On Oct 28, 2017 11:11 AM, “Suhas Daftuar” notifications@github.com wrote:
@sdaftuar commented on this pull request.
In src/net.cpp https://github.com/bitcoin/bitcoin/pull/11560#discussion_r147556553:
- return m_try_another_outbound_peer; +}
+void CConnman::SetTryNewOutboundPeer(bool flag) +{
- LOCK(m_cs_outbound_peer);
- m_try_another_outbound_peer = flag; +}
+// Return the number of peers we have over our outbound connection limit +// Exclude peers that are marked for disconnect, or are going to be +// disconnected soon (eg one-shots and feelers) +// Also exclude peers that haven’t finished initial connection handshake yet +// (so that we don’t decide we’re over our desired connection limit, and then +// evict some peer that has finished the handshake) +int CConnman::GetExtraOutboundCount()
Any thoughts on @TheBlueMatt https://github.com/thebluematt’s suggestion (mentioned on IRC yesterday) to just use the feeler connection for the extra peer, rather than initiate a potential 10th connection?
https://botbot.me/freenode/bitcoin-core-dev/2017-10-27/? msg=92828477&page=2
I’m leaning towards doing that, so nMaxExtraOutbound would go away. I guess this function wouldn’t change though.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/11560#discussion_r147556553, or mute the thread https://github.com/notifications/unsubscribe-auth/AAZdEzhjrlXqpR_iPo_-CIcNtvbYKR8cks5sw0QdgaJpZM4QGjZX .
3090+ LogPrintf("Potential stale tip detected, will try using extra outbound peer (last tip update: %d seconds ago)\n", time_in_seconds - g_last_tip_update);
3091+ connman->SetTryNewOutboundPeer(true);
3092+ } else {
3093+ connman->SetTryNewOutboundPeer(false);
3094+ }
3095+ m_stale_tip_check_time += STALE_CHECK_INTERVAL;
1812@@ -1781,7 +1813,8 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
1813 // * Only make a feeler connection once every few minutes.
1814 //
1815 bool fFeeler = false;
1816- if (nOutbound >= nMaxOutbound) {
1817+
1818+ if (nOutbound >= nMaxOutbound && !GetTryNewOutboundPeer()) {
GetTryNewOutboundPeer is set, then we keep trying outbound peers up to the semaphore count, at which point I believe we block. Is that problematic for some reason? I’m not super familiar with the design here but my understanding from reading the code was that this was how this logic was originally written (ie before feelers were introduced, with the continue in the if block to avoid consuming the last semaphore value via a non-feeler).
3058+ // Only disconnect a peer that has been connected to us for
3059+ // some reasonable fraction of our check-frequency, to give
3060+ // it time for new information to have arrived.
3061+ // Also don't disconnect any peer we're trying to download a
3062+ // block from.
3063+ constexpr int64_t min_connect_time = EXTRA_PEER_CHECK_INTERVAL / 5;
59@@ -55,6 +60,10 @@ class PeerLogicValidation : public CValidationInterface, public NetEventsInterfa
60 bool SendMessages(CNode* pto, std::atomic<bool>& interrupt) override;
61
62 void ConsiderEviction(CNode *pto, int64_t time_in_seconds);
63+ void CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams);
64+ void EvictExtraOutboundPeers(int64_t time_in_seconds);
65+
66+ int64_t m_stale_tip_check_time;
Updated to address a nit from @jimpo, and reduce the check-for-eviction interval down to 45 seconds (as @TheBlueMatt suggested).
Squashed https://github.com/sdaftuar/bitcoin/commits/11560.0 -> 8a27fdf9ae691eb32b8b5e9f531e16266f8227a8
439+{
440+ AssertLockHeld(cs_main);
441+ if (g_last_tip_update == 0) {
442+ g_last_tip_update = GetTime();
443+ }
444+ return g_last_tip_update < GetTime() - consensusParams.nPowTargetSpacing * 3 && mapBlocksInFlight.empty();
I think the scheduler behavior will mean that nodes are going to spread out their extra connections over a 10 minute window. Since we already do feeler connections on a (poisson-random) 2 minute interval, and this is taking the place of a feeler, I don’t think this will introduce too much network load.
If there’s a fingerprinting concern, I can try to come up with some fuzzing on when the connection is made (maybe another poisson-random fuzz?), but I’m not sure I understand the specific fingerprinting risk with this logic?
but I’m not sure I understand the specific fingerprinting risk with this logic?
I don’t think there is any. I had a fingerprinting concern but then realized that that concern was stupid and stopped being concerned about it.
3033+ NodeId worst_peer = -1;
3034+ int64_t oldest_block_announcement = std::numeric_limits<int64_t>::max();
3035+
3036+ LOCK(cs_main);
3037+
3038+ connman->ForEachNode([&](CNode* pnode) {
Can we loop through mapNodeState here instead? I’d really like to avoid gluing these layers back together.
Also, I believe that would mean that the nBlocksInFlight != 0 check could be applied here.
I just wrote this in my other response, but I don’t think the blocks in flight check is a good thing to sort on – I think there may be edge cases where somehow your slowest peer gets protected, and you would evict someone else.
I’ll see what it looks like when I try to loop mapNodeState instead. I think I’ll still have to call ForNode inside that loop in order to get to the pnode, so I don’t really see the difference in design…? Can you explain a little more what the design goal is for stuff like this?
3050+ // some reasonable fraction of our check-frequency, to give
3051+ // it time for new information to have arrived.
3052+ // Also don't disconnect any peer we're trying to download a
3053+ // block from.
3054+ CNodeState &state = *State(pnode->GetId());
3055+ if (time_in_seconds - pnode->nTimeConnected > MINIMUM_CONNECT_TIME && state.nBlocksInFlight == 0) {
Doesn’t this mean that if the worst peer is only slightly worse than the second-worst, and the worst has a block in flight, both would stay connected? In that case, I would expect the second-worst to be evicted.
Adding to the comment above, I think that checking for nBlocksInFlight in the worst_peer calculation above avoids this?
I don’t think it’s necessary to evict in that situation, since we’ll just try to evict again in 45 seconds, and I thought it was more conservative to not evict. For instance, the block in flight might be at equal work to our tip, rather than more work, and we wouldn’t want to disconnect the older peer in that situation where we’re just downloading an equal-work-chain-tip.
Also if you evict the second-worst peer while a block download is in flight, then it’s possible that the new peer will stall your block download, and you’ll have lost one of your earlier peers unnecessarily.
Anyway I mostly just want to avoid anything that can be gamed, and postponing eviction seems like a safer option than choosing a different peer to evict, I think?
3077+{
3078+ if (connman == nullptr) return;
3079+
3080+ int64_t time_in_seconds = GetTime();
3081+
3082+ EvictExtraOutboundPeers(time_in_seconds);
27@@ -27,13 +28,19 @@ static constexpr int64_t HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER = 1000; // 1ms/head
28 static constexpr int32_t MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT = 4;
29 /** Timeout for (unprotected) outbound peers to sync to our chainwork, in seconds */
30 static constexpr int64_t CHAIN_SYNC_TIMEOUT = 20 * 60; // 20 minutes
31+/** How frequently to check for stale tips, in seconds */
32+static constexpr int64_t STALE_CHECK_INTERVAL = 10 * 60; // 10 minutes
33+/** How frequently to check for extra outbound peers and disconnect, in seconds */
34+static constexpr int64_t EXTRA_PEER_CHECK_INTERVAL = 45;
35+/** Minimum time an outbound-peer-eviction candidate must be connected for, in order to evict, in seconds */
36+static constexpr int64_t MINIMUM_CONNECT_TIME = 30;
I think we should probably bump this to 60sec, to match the minimum time allowed for the handshake. Or was 30sec a deliberate choice?
That would also give us the handy property that all fSuccessfullyConnected nodes can be disconnected without checking the connection duration.
disconnected without checking the connection duration.
Huh.. then you’d disconnect someone who completed the handshake fast, without giving them more than a few seconds to transfer blocks.
ACK, I’ve had various versions of this running on a mainnet node for about 5 days now, but with the stall timeout cut down to a fraction of the default value so I could see it in action. Latest versions appear to work as expected. Including managing to keep the latest outbound when a block came over it, dropping instead a different outbound that never got a block.
I’m not too worried about matt’s unbalancing point: this only runs fairly rarely, it runs fairly slowly, and there are 4 protected peers that it won’t unbalance even in the worst case.
One gotcha is that the Turning off message will pretty much never print and is kind of pointless and confusing. Perhaps it should be moved to the inside of settrynewoutbound.
I’m not too worried about matt’s unbalancing point: this only runs fairly rarely, it runs fairly slowly, and there are 4 protected peers that it won’t unbalance even in the worst case.
Actually, the protected-peers logic from the chain sync eviction algorithm wasn’t being used here. After further thought I don’t actually have any good reason not to protect those peers from eviction, and I think the same reasons to protect peers apply here, so I’ve updated this PR accordingly.
I also pushed up a commit that improves the logging.
425@@ -413,6 +426,12 @@ class CConnman
426 std::thread threadOpenAddedConnections;
427 std::thread threadOpenConnections;
428 std::thread threadMessageHandler;
429+
430+ /** flag for deciding to connect to an extra outbound peer,
431+ * in excess of nMaxOutbound
432+ * This takes the place of a feeler connection */
433+ CCriticalSection m_cs_outbound_peer;
std::atomic<bool>.
@TheBlueMatt just pointed out to me that ForNode has to walk the vector of CNode’s to find the one we’re looking for, so by rewriting the ForEachNode to a loop on mapNodeState (as suggested by @theuni up here: #11560 (review)), we’re now doing n^2 work instead of n log n work… That seems unfortunate to me – default configurations for listening nodes have > 100 connections right?
I’d like to revert that change.
3077+ }
3078+}
3079+
3080+void PeerLogicValidation::CheckForStaleTipAndEvictPeers(const Consensus::Params &consensusParams)
3081+{
3082+ if (connman == nullptr) return;
CConnman* const connman; (which compiles without other changes)
3038+
3039+ connman->ForEachNode([&](CNode* pnode) {
3040+ // Ignore non-outbound peers, or nodes marked for disconnect already
3041+ if (!IsOutboundDisconnectionCandidate(pnode) || pnode->fDisconnect) return;
3042+ CNodeState *state = State(pnode->GetId());
3043+ if (state == nullptr) return; // shouldn't be possible, but just in case
I thought our preference was to not assert except in situations where node shutdown is necessary to avoid eg consensus failure or lost funds or something similarly bad? I’ll add the assert if that’s your preference, but I was just nervous about introducing a crash if I got some race condition wrong.
I could add a LogPrinf() here instead perhaps?
@TheBlueMatt Perhaps asserting is too harsh, but silently ignoring the fact that an underlying assumption is broken seems bad too.
Also, it seems very reasonable to assume that net does not invoke any net_processing operations before initializing a node, or after finalizing it? That seems easy to guarantee, and very hard for handlers to deal with if untrue.
utACK 7d9415396b049dd1477febb71930a70870c5a541
I did not review the test changes.
If our tip hasn't updated in a while, that may be because our peers are
not relaying blocks to us that we would consider valid. Allow connection
to an additional outbound peer in that circumstance.
Also, periodically check to see if we are exceeding our target number of
outbound peers, and disconnect the one which has least recently
announced a new block to us (choosing the newest such peer in the case
of tie).
Squashed https://github.com/sdaftuar/bitcoin/commits/11560.4 -> 626291508c433488439b662f2e88882048fb59fb
0$ git diff 626291508c433488439b662f2e88882048fb59fb 7d9415396b049dd1477febb71930a70870c5a541
1diff --git a/src/net_processing.h b/src/net_processing.h
2index 0a49972eed..c1f1274f80 100644
3--- a/src/net_processing.h
4+++ b/src/net_processing.h
5@@ -37,7 +37,7 @@ static constexpr int64_t MINIMUM_CONNECT_TIME = 30;
6
7 class PeerLogicValidation : public CValidationInterface, public NetEventsInterface {
8 private:
9- CConnman* const connman;
10+ CConnman* connman;
11
12 public:
13 explicit PeerLogicValidation(CConnman* connman, CScheduler &scheduler);
I had a slow sync issue which may have been related to and/or fixed by this… See #12514
I eventually did get the recompile of the GitHub code but the version # did not change.
I hope my issue/resolution helps in fixing this issue or confirming it is fixed :)