Eventually connect to NODE_NETWORK

jonasschnelli commented at 3:08 pm on May 11, 2017: contributor

Eventually connect to peers signalling NODE_NETWORK_LIMITED if we are out of IBD. Accept and relay NODE_NETWORK_LIMITED peers in addrman.

jonasschnelli added the label P2P on May 11, 2017

jonasschnelli force-pushed on May 12, 2017

jonasschnelli force-pushed on Aug 8, 2017

jonasschnelli renamed this:
~~[WIP] Define and signal NODE_NETWORK_LIMITED (pruned peers)~~
Implement BIP159, define and signal NODE_NETWORK_LIMITED (pruned peers)
on Aug 8, 2017

jonasschnelli added this to the milestone 0.16.0 on Aug 8, 2017

jonasschnelli commented at 8:53 pm on August 8, 2017: contributor

~~Rebased, overhauled and removed WIP tag. Conforms now to BIP159. Added 0.16 milestone tag.~~

jonasschnelli force-pushed on Aug 8, 2017

jonasschnelli force-pushed on Aug 11, 2017

jonasschnelli commented at 7:34 pm on August 11, 2017: contributor

~~Added a fingerprinting counter-measure which protects from leaking the prune depth by finding it through getdata requests (21c347c8c1a9ee4803b8f8f124185c218e01db76).~~

jonasschnelli force-pushed on Aug 12, 2017

jonasschnelli added this to the "Blockers" column in a project

jonasschnelli force-pushed on Sep 23, 2017

jonasschnelli added the label Needs release notes on Sep 23, 2017

in src/net_processing.cpp:1019 in 3fedeac1db outdated

1019@@ -1006,6 +1020,13 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
1020                     pfrom->fDisconnect = true;
1021                     send = false;
1022                 }
1023+                // Avoid leaking prune-height by never send blocks below the NODE_NETWORK_LIMITED threshold

fanquake commented at 2:46 am on September 26, 2017:

nit: sending

in src/validation.h:206 in 3fedeac1db outdated

202@@ -203,6 +203,8 @@ extern bool fPruneMode;
203 extern uint64_t nPruneTarget;
204 /** Block files containing a block-height within MIN_BLOCKS_TO_KEEP of chainActive.Tip() will not be pruned. */
205 static const unsigned int MIN_BLOCKS_TO_KEEP = 288;
206+/** Minimum blocks to must be available to signal NODE_NETWORK_LIMITED */

fanquake commented at 2:48 am on September 26, 2017:

s/to must be available/required

jonasschnelli force-pushed on Sep 28, 2017

jonasschnelli commented at 4:10 am on September 28, 2017: contributor

Fixed travis trailing whitespace in test issue.

in src/net_processing.h:74 in 837ef7497f outdated

69+ * @param[in]   pto             The node which we are sending messages to.
70+ * @param[in]   connman         The connection manager for that node.
71+ * @param[in]   interrupt       Interrupt condition for processing threads
72+ * @return                      True if there is more work to be done
73+ */
74+bool SendMessages(CNode* pto, CConnman& connman, const std::atomic<bool>& interrupt);

TheBlueMatt commented at 10:35 pm on September 28, 2017:

Looks like a rebase error - these functions were moved to the PeerLogicValidation.

in src/net_processing.cpp:3325 in 837ef7497f outdated

3320+
3321+        // make sure NODE_NETWORK is disabled
3322+        nLocalServices = ServiceFlags(nLocalServices & ~NODE_NETWORK);
3323+
3324+        // don't signal NODE_NETWORK_LIMITED during initial block download
3325+        if (IsInitialBlockDownload()) {

TheBlueMatt commented at 10:38 pm on September 28, 2017:

Hmm, this feels pretty strange to me. Service bits indicate a commitment to provide certain services, not your current state. More importantly, IBD is always neccessarily fuzzy - we can’t know how “in sync” we are.

in src/net_processing.cpp:3350 in 837ef7497f outdated

3345+    }
3346+    else {
3347+        // currently, the prune mode can't be changed during runtime
3348+        // implement the service bit switch for possible future flexibility
3349+        nLocalServices = ServiceFlags(nLocalServices | NODE_NETWORK);
3350+        nLocalServices = ServiceFlags(nLocalServices & ~NODE_NETWORK_LIMITED);

TheBlueMatt commented at 10:42 pm on September 28, 2017:

Why? The BIP seems somewhat unclear here, but I’d stronlgy prefer that NODE_NETWORK_LIMITED simply be always set to indicate that we can always provide the most recent 288 blocks, irrespective of if we’re pruned or not.

in src/net_processing.cpp:3330 in 837ef7497f outdated

3325+        if (IsInitialBlockDownload()) {
3326+            return;
3327+        }
3328+
3329+        // check how many blocks we have available (backwards from tip)
3330+        // don't go futher down then chainActive.Height()-NODE_NETWORK_LIMITED

TheBlueMatt commented at 10:45 pm on September 28, 2017:

This seems entirely redundant - even if by super rare chance we end up due to reorg short of 288 blocks, we won’t be very short of 288 blocks, and will be caught back up within the next block or two, might as well just always set NODE_NETWORK_LIMITED (you can static_assert MIN_BLOCKS_TO_KEEP is at least 288 if you really want to have a check).

in src/protocol.h:254 in e08e49f899 outdated

249@@ -250,8 +250,8 @@ enum ServiceFlags : uint64_t {
250     // Nothing
251     NODE_NONE = 0,
252     // NODE_NETWORK means that the node is capable of serving the block chain. It is currently
253-    // set by all Bitcoin Core nodes, and is unset by SPV clients or other peers that just want
254-    // network services but don't provide them.
255+    // set by all Bitcoin Core non pruned nodes, and is unset by SPV clients or other peers
256+    // that just want network services but don't provide them.

TheBlueMatt commented at 10:52 pm on September 28, 2017:

If you’re gonna update this maybe overhaul the text? The concept of “want network services but don’t provide them” doesn’t really make sense anymore (especially with NODE_NETWORK_LIMITED). Maybe just say “indicates peer is capable of serving the full historical block chain”.

in src/net.cpp:2675 in 62cdcc6ec2 outdated

2671@@ -2672,6 +2672,11 @@ ServiceFlags CConnman::GetLocalServices() const
2672     return nLocalServices;
2673 }
2674 
2675+void CConnman::SetLocalServices(ServiceFlags flagsIn)

TheBlueMatt commented at 10:55 pm on September 28, 2017:

I’m entirely unconvinced NODE_NETWORK_LIMITED needs this (and would prefer to avoid adding it until we need it, though its no big deal to add it).

in src/rpc/blockchain.cpp:904 in 6aae178554 outdated

896@@ -895,6 +897,13 @@ UniValue pruneblockchain(const JSONRPCRequest& request)
897     }
898 
899     PruneBlockFilesManual(height);
900+
901+    // check if we need to change the local service flags in order to signal NODE_NETWORK_LIMITED flags
902+    if(g_connman) {
903+        ServiceFlags nFlags = g_connman->GetLocalServices();
904+        CheckLocalServices(nFlags);

TheBlueMatt commented at 10:56 pm on September 28, 2017:

Why do we need to do this? You can’t prune deeper than 128 and NODE_NETWORK is already unset for manually-pruned nodes.

in src/net_processing.cpp:1021 in 39926136e6 outdated

1019@@ -1020,6 +1020,13 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
1020                     pfrom->fDisconnect = true;
1021                     send = false;
1022                 }
1023+                // Avoid leaking prune-height by never send blocks below the NODE_NETWORK_LIMITED threshold
1024+                if (send && !pfrom->fWhitelisted && (
1025+                        (((connman->GetLocalServices() & NODE_NETWORK_LIMITED) == NODE_NETWORK_LIMITED) && (chainActive.Tip()->nHeight - mi->second->nHeight > (int)NODE_NETWORK_LIMITED_MIN_BLOCKS) )

TheBlueMatt commented at 11:01 pm on September 28, 2017:

This should use pfrom->GetLocalServices() not connman->GetLocalServices() - our decisions should be based on what we told the peer we support, not what we would tell a new peer to support.

TheBlueMatt commented at 11:03 pm on September 28, 2017: member

I think this PR is significantly needlessly over-complicated (mostly we dont allow nodes to go below 288 blocks, no need to check for it in so many places and update our services accordingly). Excited to see this go in, though.

jonasschnelli commented at 2:56 am on September 29, 2017: contributor

~~Thanks for the review @TheBlueMatt. I agree with all you points and most things can now go away since the NODE_NETWORK_LIMITED_HIGH flag (1152 blocks minimum) is gone.~~

~~Will overhaul and simplify it now.~~

jonasschnelli force-pushed on Oct 1, 2017

jonasschnelli commented at 4:22 am on October 1, 2017: contributor

Simplified the PR and addressed @TheBlueMatt and @fanquake points.

in src/init.cpp:1594 in 78011f7e2a outdated

1589@@ -1590,6 +1590,8 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1590     if (fPruneMode) {
1591         LogPrintf("Unsetting NODE_NETWORK on prune mode\n");
1592         nLocalServices = ServiceFlags(nLocalServices & ~NODE_NETWORK);
1593+        // always set NODE_NETWORK_LIMITED (BIP159) in pruned mode
1594+        nLocalServices = ServiceFlags(nLocalServices | NODE_NETWORK_LIMITED);

TheBlueMatt commented at 2:02 pm on October 3, 2017:

Hmm, I thought we were just going to alays set this? Why bother only setting it when pruned - a node that is not pruned is also capable of serving the last 288 blocks, which is the way I read the definition of NODE_NETWORK_LIMITED.

theuni commented at 5:34 pm on October 3, 2017:

I agree. Came to complain about the same thing.

NODE_NETWORK implies NODE_NETWORK_LIMITED, so _LIMITED should always be set. Otherwise, we’re introducing negative service flags, which is a bad idea imo.

jonasschnelli commented at 3:46 am on October 4, 2017:

Okay. Agree… will change it.

in src/protocol.h:268 in 71d9aafb07 outdated

265@@ -267,6 +266,9 @@ enum ServiceFlags : uint64_t {
266     // NODE_XTHIN means the node supports Xtreme Thinblocks
267     // If this is turned off then the node will not service nor make xthin requests
268     NODE_XTHIN = (1 << 4),
269+    // NODE_NETWORK_LIMITED means the same as NODE_NETWORK with the limitation of only
270+    // serving the last 288 (2 day) blocks

jimpo commented at 5:47 pm on October 3, 2017:

Perhaps reference BIP 159 in the comment here for additional explanation?

in src/net_processing.cpp:824 in d1ce50c1bc outdated

817@@ -818,6 +818,16 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
818     }
819 
820     nTimeBestReceived = GetTime();
821+
822+    // update service flags
823+    {
824+        LOCK(cs_main);

jimpo commented at 5:51 pm on October 3, 2017:

We can avoid obtaining this lock if the alternative services is already NODE_NETWORK_LIMITED.

jonasschnelli commented at 4:00 am on October 4, 2017:

I think we should not connect to NODE_NETWORK_LIMITED peers if IBD is still ongoing. NODE_NETWORK_LIMITED implementations really should make sure to not serve deeper then >288 blocks (fingerprinted). Seems useless to me connect until clear IBD.

jimpo commented at 9:25 pm on October 4, 2017:

Yeah, I understand that, but unless I’m missing something, this code is run every time there is a new block after IBD is done instead of happening only once. It seems possible to change the logic to exit before obtaining the lock if AlternativeServices has already been switched from NODE_NETWORK to NODE_NETWORK_LIMITED.

in src/net_processing.cpp:1648 in d1ce50c1bc outdated

1311@@ -1302,7 +1312,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
1312             pfrom->cleanSubVer = cleanSubVer;
1313         }
1314         pfrom->nStartingHeight = nStartingHeight;
1315-        pfrom->fClient = !(nServices & NODE_NETWORK);
1316+        pfrom->fClient = (!(nServices & NODE_NETWORK) && !(nServices & NODE_NETWORK_LIMITED));

theuni commented at 5:56 pm on October 3, 2017:

I’d rather not use fClient as a proxy for NODE_NETWORK_LIMITED. It deserves its own classification.

theuni commented at 6:40 pm on October 3, 2017:

Yes, this needs its own bool so that fClient gets checked when testing for our initial headers-sync peer. Then, fPreferredDownload is only set to false if we’re still in IBD.

That way, we request from this peer as if they were a regular full node as long as we’re synced.

jonasschnelli commented at 5:11 am on October 4, 2017:

IMO we want pruned peers for header sync… but right, we need a protection to not request blocks from limited peers during IBD. What about adding an fLimitedNode bool and check it at the point we populate vGetData with block requests?

Seems also simpler and more constant (NODE_NETWORK_LIMITED is not really a “client”).

I changed this PR and applied my approach which I have written above.

in test/functional/test_framework/mininode.py:230 in f63c723953 outdated

226         self.pchReserved = b"\x00" * 10 + b"\xff" * 2
227         self.ip = "0.0.0.0"
228         self.port = 0
229 
230-    def deserialize(self, f):
231+    def deserialize(self, f, without_time=False):

theuni commented at 6:07 pm on October 3, 2017:

nit: a default of without_foo = False isn’t no bad thing for nobody.

with_time=True please :)

in src/net.cpp:1841 in abd60f63ac outdated

1837+            if ( ((addr.nServices & nAlternativeServices) == nAlternativeServices) && (addr.nServices & nRelevantServices) != nRelevantServices && (nTries < 20 || nOutbound >= (nMaxOutbound >> 2)))
1838+                continue;
1839+
1840             // only consider nodes missing relevant services after 40 failed attempts and only if less than half the outbound are up.
1841             ServiceFlags nRequiredServices = nRelevantServices;
1842             if (nTries >= 40 && nOutbound < (nMaxOutbound >> 1)) {

jimpo commented at 6:08 pm on October 3, 2017:

In this clause, should nRequiredServices get set to NODE_NETWORK_LIMITED if peer is in sync? Essentially, replace REQUIRED_SERVICES below with a value that goes from NODE_NETWORK to NODE_NETWORK_LIMITED when peer is out of IBD.

jimpo commented at 6:20 pm on October 3, 2017: contributor

This change prefers connecting to full nodes rather than pruned nodes even if fully synced. Why is that the desired approach?

I find that the required services/relevant services/alternative services stuff is getting confusing. If I’m reading it right, the order of preference is:

NODE_NETWORK & NODE_WITNESS
NODE_NETWORK_LIMITED & NODE_WITNESS (if !IsInitialDownload)
NODE_NETWORK

It seems like there should be a way to simplify this, where the ConnManager tracks the ranked preferences of peer services with the nOutbound quota and nTries threshold for each.

jonasschnelli commented at 3:53 am on October 4, 2017: contributor

This change prefers connecting to full nodes rather than pruned nodes even if fully synced. Why is that the desired approach? […] I find that the required services/relevant services/alternative services stuff is getting confusing. If I’m reading it right, the order of preference is… […]

We probably should avoid an abrupt change for pruned peers. This approach (alternative services) would result is a slow usage of the current deployed (and then upgraded) pruned peers. Otherwise, pruned peers may see a significant overall usage (bandwidth / CPU / disk-usage) bump.

Since there is no toggle to disable NODE_NETWORK_LIMITED signalling in pruned mode, I think this (slow) approach would be wise.

jonasschnelli force-pushed on Oct 4, 2017

jonasschnelli commented at 5:25 am on October 4, 2017: contributor

Addressed @theuni & @TheBlueMatt points. Now mentions BIP159 in protocol.h (found by @jimpo).

jonasschnelli force-pushed on Oct 4, 2017

theuni commented at 9:04 pm on October 4, 2017: member

“Set NODE_NETWORK_LIMITED bit in pruned mode” commit message is confusing now. Mind rewording it to reflect the new behavior?

jonasschnelli force-pushed on Oct 4, 2017

jonasschnelli commented at 9:17 pm on October 4, 2017: contributor

@theuni: overhauled that commit message.

in src/net.cpp:1836 in 4f5d61e86e outdated

1832             // only consider very recently tried nodes after 30 failed attempts
1833             if (nANow - addr.nLastTry < 600 && nTries < 30)
1834                 continue;
1835 
1836+            // only consider nodes matching alternative and missing relevant services after 10 failed attempts and only if less than a fourth of the outbound are up.
1837+            if ( ((addr.nServices & nAlternativeServices) == nAlternativeServices) && (addr.nServices & nRelevantServices) != nRelevantServices && (nTries < 20 || nOutbound >= (nMaxOutbound >> 2)))

jimpo commented at 9:39 pm on October 4, 2017:

Seems that we would never connect to limited nodes because of the code on lines 1840-1847. nRequiredServices is set to either nRelevantServices or REQUIRED_SERVICES, never nAlternativeServices.

TheBlueMatt commented at 2:55 pm on October 5, 2017: member

The nRelevantServices stuff is somewhat already a mess, and trying to work within the current code is just getting messier :/. I spent a bit of time rewriting it and (mostly) like the outcome, its at https://github.com/TheBlueMatt/bitcoin/commit/2017-09-service-flags-cleanups - and I think basing this PR on it would simplify this one a lot. It sucks to add some functions to protocol.h/cpp, especially if they have knowledge of IBD state, but that should be able to move soon once @theuni makes net.cpp not aware of service bits.

theuni commented at 2:59 pm on October 5, 2017: member

Agree with @TheBlueMatt on taking that as a starting point. We discussed it a little, and I think defining functions for the filtered use-cases makes life much easier. @jonasschnelli it’s kinda intrusive on the PR, but would it be possible to work that in?

As for making net not aware of service bits, I’m working on breaking addrman out right now. Should be pretty painless and a nice improvement.

in test/functional/test_framework/mininode.py:231 in 4f5d61e86e outdated

227         self.ip = "0.0.0.0"
228         self.port = 0
229 
230-    def deserialize(self, f):
231+    def deserialize(self, f, with_time=True):
232+        if with_time == True:

promag commented at 8:35 am on October 7, 2017:

Remove == True.

in test/functional/test_framework/mininode.py:240 in 4f5d61e86e outdated

237         self.port = struct.unpack(">H", f.read(2))[0]
238 
239-    def serialize(self):
240+    def serialize(self, with_time=True):
241         r = b""
242+        if with_time == True:

promag commented at 8:35 am on October 7, 2017:

Remove == True.

in src/net.cpp:186 in 4f5d61e86e outdated

182@@ -183,6 +183,10 @@ void AdvertiseLocal(CNode *pnode)
183     if (fListen && pnode->fSuccessfullyConnected)
184     {
185         CAddress addrLocal = GetLocalAddress(&pnode->addr, pnode->GetLocalServices());
186+        if (gArgs.GetBoolArg("-addrmantest", false) == true) {

promag commented at 8:35 am on October 7, 2017:

Remove == true.

in src/net.cpp:198 in 4f5d61e86e outdated

194@@ -191,7 +195,7 @@ void AdvertiseLocal(CNode *pnode)
195         {
196             addrLocal.SetIP(pnode->GetAddrLocal());
197         }
198-        if (addrLocal.IsRoutable())
199+        if (addrLocal.IsRoutable() || gArgs.GetBoolArg("-addrmantest", false) == true)

promag commented at 8:35 am on October 7, 2017:

Remove == true.

in test/functional/node_network_limited.py:48 in 4f5d61e86e outdated

43+        node.wait_for_verack()
44+        node.send_message(msg_verack())
45+        getdata_request = msg_getdata()
46+        getdata_request.inv.append(CInv(2, int(blockhash, 16)))
47+        node.send_message(getdata_request)
48+        #check if we the peer sends us the block

promag commented at 8:36 am on October 7, 2017:

Nit, space after # here and below.

in test/functional/node_network_limited.py:81 in 4f5d61e86e outdated

76+        self.tryGetBlockViaGetData(blocks[1], True)
77+        self.tryGetBlockViaGetData(blocks[0], False)
78+
79+        #NODE_NETWORK_LIMITED must still be signaled after restart
80+        self.stop_node(0)
81+        self.start_node(0, extra_args=self.extra_args[0])

promag commented at 8:37 am on October 7, 2017:

self.restart_node(0).

sipa referenced this in commit 326a5652e0 on Oct 13, 2017

jonasschnelli force-pushed on Oct 14, 2017

jonasschnelli commented at 5:05 am on October 14, 2017: contributor

Rebased (since #11456 is now merged). Tried to keep calls to InitialBlockDownload() to a minimum, therefore added initialBlockDownloadCompleted (bool) to protocol.h. Not sure if this is the best idea (ping @TheBlueMatt & @theuni).

jonasschnelli force-pushed on Oct 15, 2017

jonasschnelli force-pushed on Oct 29, 2017

fanquake commented at 9:25 am on November 4, 2017: member

This needs a rebase.

in src/protocol.h:308 in cef6bc63ec outdated

298@@ -297,6 +299,9 @@ enum ServiceFlags : uint64_t {
299  * Thus, generally, avoid calling with peerServices == NODE_NONE.
300  */
301 static ServiceFlags GetDesirableServiceFlags(ServiceFlags services) {
302+    if (initialBlockDownloadCompleted) {
303+        return ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS);

TheBlueMatt commented at 10:05 pm on November 6, 2017:

This would result in us only connecting to NODE_NETWORK_LIMITED after IBD - I think this should be:

0if (services & NODE_NETWORK_LIMITED && initialBlockDownloadCompleted) return ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS);
1return ServiceFlags(NODE_NETWORK | NODE_WITNESS);

in src/protocol.h:327 in 005fafdaa9 outdated

310@@ -311,10 +311,10 @@ static inline bool HasAllDesirableServiceFlags(ServiceFlags services) {
311 
312 /**
313  * Checks if a peer with the given service flags may be capable of having a
314- * robust address-storage DB. Currently an alias for checking NODE_NETWORK.
315+ * robust address-storage DB.
316  */
317 static inline bool MayHaveUsefulAddressDB(ServiceFlags services) {
318-    return services & NODE_NETWORK;
319+    return (services & NODE_NETWORK) || (services & NODE_NETWORK_LIMITED);

TheBlueMatt commented at 10:36 pm on November 6, 2017:

The commit message is weird here - this function is intended to be used to decide whether a node may be a useful feeler, and thus the commit message should be about relayed addresses from nodes with NODE_NETWORK_LIMITED set. But since we also use it in ADDR message processing, it kinda means both - I think we should change the ADDR message processing to MayHaveUsefulAddressDB || HasAllDesirableServiceFlags to clarify things (which will not actually be a chahnge, but thats fine).

jonasschnelli commented at 7:40 pm on December 12, 2017:

Fixed

TheBlueMatt commented at 10:47 pm on November 6, 2017: member

Looks pretty good. Didn’t review tests, needs rebase as noted.

gmaxwell commented at 2:33 am on November 10, 2017: contributor

Perhaps this should be split into “set the limited flag” PR and make use of it, the setting is ultra trivial and could even be considered for backport, esp since the anti-fingerprinting part is a bugfix.

The actual use of the flag in our connection I think needs work, we should be using our current time and our current headers to decide if LIMITED peers are likely useful to us. IsInitialBlockDownload is a very vague and general criteria and isn’t the one we want here.

I think what we want is a function that tells us if we expect limited peers to be useful. If the headers we have say that there are more than 280 blocks missing, limited is not useful for us. Or if our best header timestamp is more than (say) 1.75 days old, we should assume they’re not useful to us. When we connect, we’ll learn headers, so if for example we’re 300 blocks behind we’ll find out then, and then not consider limited peers anymore until we catch up.

For deciding on fetching, we should already have headers, and so the fetch should perhaps just only be checking if the peer block is more than 280 blocks back (again, giving 8 blocks slack to cover cases where the peer gets blocks where we haven’t heard of the headers yet).

in src/net_processing.cpp:1103 in 2d6893ad63 outdated

1058@@ -1059,6 +1059,13 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
1059                     pfrom->fDisconnect = true;
1060                     send = false;
1061                 }
1062+                // Avoid leaking prune-height by never sending blocks below the NODE_NETWORK_LIMITED threshold
1063+                if (send && !pfrom->fWhitelisted && (
1064+                        (((pfrom->GetLocalServices() & NODE_NETWORK_LIMITED) == NODE_NETWORK_LIMITED) && ((pfrom->GetLocalServices() & NODE_NETWORK) != NODE_NETWORK) && (chainActive.Tip()->nHeight - mi->second->nHeight > (int)NODE_NETWORK_LIMITED_MIN_BLOCKS) )
1065+                   )) {
1066+                    LogPrint(BCLog::NET, "Ignore block request below NODE_NETWORK_LIMITED threshold from peer=%d\n", pfrom->GetId());
1067+                    send = false;

gmaxwell commented at 2:34 am on November 10, 2017:

This needs to disconnect, I think. Otherwise we DOS attack our peer, causing them to take a 20 minute timeout on their attempted request to fetch a block they think we’re doing to provide.

jonasschnelli force-pushed on Nov 15, 2017

jonasschnelli commented at 8:45 pm on November 15, 2017: contributor

Rebased and worked in @TheBlueMatt and @gmaxwell point. It’s still based on IsInitialBlockDownload() but I have plans to implement @gmaxwell idea (which makes sense to me) to rely on out time and headers.

jonasschnelli force-pushed on Nov 16, 2017

Sjors commented at 5:24 pm on November 20, 2017: member

This 288 blocks limit seems a bit overzealous, but I suppose it can be expanded later if nodes just assume 288 for un-upgraded peers.

I couldn’t find the discussion that led to that change in BIP159, so maybe this was already brought up.

The node on my laptop is pruned to 10 GB, Bitseed and Raspberry Pie nodes might be pruned to 100-200 GB. They can be of use to nodes even during IBD. To prevent fingerprinting and still allow more flexibility in sharing data, perhaps NODE_NETWORK_LIMITED_MIN_BLOCKS can be made configurable and somehow communicated? The default could still be 288, with a number of specific suggestions in bitcoin.conf.

The setting could be sanity checked against prune= and - better, but not sure if possible - automatically chosen based on the worst case for prune= (again, limited options, e.g. …, 1 GB, 10 GB, 100 GB, …).

Also, what happens if a node has a prune= setting that’s above the current blockchain size? Does it advertise as NODE_NETWORK_LIMITED? Ideally it wouldn’t. Perhaps this can be added to the BIP?

Sjors commented at 5:38 pm on November 20, 2017: member

Found this (Google is more helpful when you search by name than by BIP number): https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-May/014314.html (should be added to the References section in the BIP?)

From that discussion:

The peak at syncing is at ~24 hours

This explains why even 288 is very useful (worth mentioning in the BIP).

There was some discussion around which sizes to allow, but it was mostly focussed on not using more service bits than needed (or states). I wouldn’t want to waste those of course. Can a node simply disclose the pruning depth when asked (within a limited set of choices to prevent fingerprinting), or would that open a can of worms?

jonasschnelli commented at 8:05 pm on November 20, 2017: contributor

I think we should only discuss the implementation here. The BIP discussion belongs to the mailing list (https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-May/thread.html#14315).

Revealing the prune depth is considered a fingerprinting weakness.

The current BIP and implementation is extendable. I’d like to start with a sane buffer 24h of blocks (144) + 144 block reorganisation buffer. It is possible to extend this later with more differentiated depths.

It would also be possible to bypass the anti-fingerprinting hard 288blocks limit when whitelisting the peer (or once we have BIP150) (not in scope for this PR).

in doc/bips.md:36 in e89dbb67bf outdated

32@@ -33,3 +33,4 @@ BIPs that are implemented by Bitcoin Core (up-to-date up to **v0.13.0**):
33 * [`BIP 145`](https://github.com/bitcoin/bips/blob/master/bip-0145.mediawiki): getblocktemplate updates for Segregated Witness as of **v0.13.0** ([PR 8149](https://github.com/bitcoin/bitcoin/pull/8149)).
34 * [`BIP 147`](https://github.com/bitcoin/bips/blob/master/bip-0147.mediawiki): NULLDUMMY softfork as of **v0.13.1** ([PR 8636](https://github.com/bitcoin/bitcoin/pull/8636) and [PR 8937](https://github.com/bitcoin/bitcoin/pull/8937)).
35 * [`BIP 152`](https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki): Compact block transfer and related optimizations are used as of **v0.13.0** ([PR 8068](https://github.com/bitcoin/bitcoin/pull/8068)).
36+* [`BIP 159`](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki): NODE_NETWORK_LIMITED service bits are supported as of **v0.16.0** ([PR 10387](https://github.com/bitcoin/bitcoin/pull/10387)).

promag commented at 1:21 am on November 29, 2017:

service bit (singular)?

in test/functional/node_network_limited.py:67 in e89dbb67bf outdated

51+        self.nodes[0].disconnect_p2ps()
52+        node.wait_for_disconnect()
53+
54+    def run_test(self):
55+        #NODE_BLOOM & NODE_WITNESS & NODE_NETWORK_LIMITED must now be signaled
56+        assert_equal(self.getSignaledServiceFlags(), 1036) #1036 == 0x40C == 0100 0000 1100

promag commented at 1:25 am on November 29, 2017:

Add space after # so that the arrows below point to the correct bits.

promag commented at 1:26 am on November 29, 2017: member

Needs rebase.

laanwj referenced this in commit 59d3dc85b6 on Dec 9, 2017

jonasschnelli force-pushed on Dec 11, 2017

jonasschnelli renamed this:
~~Implement BIP159, define and signal NODE_NETWORK_LIMITED (pruned peers)~~
Eventually connect to NODE_NETWORK_LIMITED peers
on Dec 11, 2017

in src/net_processing.cpp:942 in 8a3ecc9018 outdated

942-             !IsInitialBlockDownload() &&
943-             mapBlocksInFlight.count(hash) == mapBlocksInFlight.size()) {
944-        if (it != mapBlockSource.end()) {
945-            MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first, connman);
946+    else if (state.IsValid()) {
947+        initialBlockDownloadCompleted = !IsInitialBlockDownload();

TheBlueMatt commented at 7:16 pm on December 12, 2017:

Can you move this update to UpdatedBlockTip? (it already has an fInitialDownload instead of having to call IsInitialDownload, even).

jonasschnelli commented at 7:40 pm on December 12, 2017:

This code part doesn’t add a new call, it just preserves the state of the result of IsInitialBlockDownload() in the new IsInitialBlockDownload variable.

TheBlueMatt commented at 4:50 pm on December 13, 2017:

Yes, I’m suggesting you move the update of initialBlockDownloadCompleted to UpdatedBlockTip instead of BlockChecked.

jonasschnelli commented at 7:33 pm on December 13, 2017:

I see. Fixed.

TheBlueMatt commented at 7:23 pm on December 12, 2017: member

I think IsInitialBlockDownload is fine for now. It requires that the block time on our block tip be at least within the last day, which is a reasonable metric for if LIMITED peers are useful. Sadly we don’t have a concept of our best header against which to test, and I’m not sure we want to decide LIMITED peers are not useful if we find a chain (eg fork that pays out 10x reward to encourage miners temporarily) that is invalid.

jonasschnelli commented at 7:42 pm on December 12, 2017: contributor

Rebased and removed the now merged signalling part. This PR adds address relay and eventually connecting to NODE_NETWORK_LIMITED if we are out of IBD. Not sure, but maybe @gmaxwell idea should be implemented later?

in src/protocol.h:299 in 8a3ecc9018 outdated

295@@ -294,6 +296,7 @@ enum ServiceFlags : uint64_t {
296  * Thus, generally, avoid calling with peerServices == NODE_NONE.
297  */
298 static ServiceFlags GetDesirableServiceFlags(ServiceFlags services) {
299+    if (services & NODE_NETWORK_LIMITED && initialBlockDownloadCompleted) return ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS);

NicolasDorier commented at 2:44 am on December 13, 2017:

On https://github.com/bitcoin/bitcoin/pull/10387/files#diff-eff7adeaec73a769788bb78858815c91R1625 you are defining a limitedPeer as one which does not have NODE_NETWORK and which have NODE_NETWORK_LIMITED. Which is different from this logic which check only if NODE_NETWORK_LIMITED is available. I think the logic should be consistent.

jonasschnelli commented at 6:12 am on December 13, 2017:

I can’t follow you comment. pfrom->fLimitedNode is set to true if the peer does not signal NODE_NETWORK but signals NODE_NETWORK_LIMITED. At this point GetDesirableServiceFlags(), we should return NODE_NETWORK_LIMITED (not NODE_NETWORK) if we are out of IBD. I think this is correct,…

NicolasDorier commented at 6:32 am on December 13, 2017:

I am not talking about the return value, in other words, this is what I would have done:

0var limited =  (!(services & NODE_NETWORK) && (services & NODE_NETWORK_LIMITED));
1if (limited && initialBlockDownloadCompleted) return ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS);
2return ServiceFlags(NODE_NETWORK | NODE_WITNESS);

The reason is that this is how you define if a peer is limited from his services flags before, I don’t understand why you are using different logic here.

jonasschnelli commented at 6:52 am on December 13, 2017:

I don’t think we should check for !(services & NODE_NETWORK) because we also want to connect to NODE_NETWORK peers… but we are also happy by connecting to a peer that has NODE_NETWORK_LIMITED but not NODE_NETWORK.

NicolasDorier commented at 7:38 am on December 13, 2017:

Imagine the following scenario:

You are not in IBD, connect to peers only with NODE_NETWORK|NODE_NETWORK_LIMITED
For some reason, you start being in IBD once again.

Now your node is stuck because of this.

An alternative fix would be to only check for NODE_NETWORK_LIMITED here.

In general I think it is weird that we treat differently NODE_NETWORK|NODE_NETWORK_LIMITED peer from NODE_NETWORK peers.

jonasschnelli commented at 7:32 pm on December 13, 2017:

In general I think it is weird that we treat differently NODE_NETWORK|NODE_NETWORK_LIMITED peer from NODE_NETWORK peers.

I think we have to. Otherwise we would never connect to pruned peers… what we want is also considering connecting to pruned if we are not deeper then 144 blocks from the expected tip.

For some reason, you start being in IBD once again.

I think there you are right. When switching back to IBD below say 144*1.5 blocks from the tip, we eventually hit a peer stalling us. Though it should be detected by the stalling logic and should get disconnected. The free slow should be then be re-used with a peer that must have NODE_NETWORK (because GetDesirableServiceFlags() will required NODE_NETWORK again at this point).

But I agree that disconnecting pruned (NODE_NETWORK_LIMITED only) peers once we fallback to IBD should be something we implement.

NicolasDorier commented at 3:54 am on December 14, 2017:

NODE_NETWORK|NODE_NETWORK_LIMITED makes no sense to me. I was under the impression that if a peer was serving only limited block it would be NODE_NETWORK_LIMITED. If the peer can serve more, it would be NODE_NETWORK.

So what is the meaning of a peer NODE_NETWORK|NODE_NETWORK_LIMITED ? I would argue that this mean they can serve historical blocks, so they should be considered as if they were NODE_NETWORK only. Is there another meaning to it?

jonasschnelli commented at 6:06 am on December 14, 2017:

As of BIP159, NODE_NETWORK_LIMITED signals that a peer has the last 288 blocks. This is “always” true for a NODE_NETWORK peer. This was chosen because services are a bit-set and bits should not depend on each other.

Conclusion:

NODE_NETWORK (unpruned peer, can serve the complete history)
NODE_NETWORK_LIMITED ^NODE_NETWORK (no NODE_NETWORK) (can serve the last 288 blocks but can’t serve the complete history)
NODE_NETWORK | NODE_NETWORK_LIMITED (can serve the complete history and the last 288 blocks [which is obvious])

NicolasDorier commented at 7:01 am on December 14, 2017:

OK I got it now. Thanks for explanation, this is fine to me.

jimpo commented at 8:32 am on January 4, 2018:

nit: Please put parentheses around services & NODE_NETWORK_LIMITED so it’s easier to visually parse. And put the return on the next line as this one is rather long.

jonasschnelli force-pushed on Dec 13, 2017

NicolasDorier commented at 7:03 am on December 14, 2017: contributor

Code review ACK. Might need in a follow up PR to kick limited peers when we go back in IBD though.

in src/protocol.h:281 in ee86431134 outdated

277@@ -277,6 +278,7 @@ enum ServiceFlags : uint64_t {
278     // BIP process.
279 };
280 
281+extern std::atomic<bool> initialBlockDownloadCompleted;

jimpo commented at 8:22 am on January 4, 2018:

nit: I believe it should be g_initial_block_download_completed per style guidelines.

in src/net.h:646 in ee86431134 outdated

642@@ -643,6 +643,7 @@ class CNode
643     bool fOneShot;
644     bool m_manual_connection;
645     bool fClient;
646+    bool fLimitedNode; //after BIP159

jimpo commented at 8:23 am on January 4, 2018:

nit: m_limited_node?

in test/functional/test_framework/messages.py:861 in ddba9453c2 outdated

853@@ -849,11 +854,11 @@ def deserialize(self, f):
854         self.nServices = struct.unpack("<Q", f.read(8))[0]
855         self.nTime = struct.unpack("<q", f.read(8))[0]
856         self.addrTo = CAddress()
857-        self.addrTo.deserialize(f)
858+        self.addrTo.deserialize(f, False)

jimpo commented at 7:45 pm on January 4, 2018:

I think it’s more clear to call with keyword arguments for boolean options: deserialize(f, with_time=False). Same for invocations below.

laanwj commented at 4:13 pm on February 14, 2018:

Agree; using a named argument would make this code more readable.

jimpo commented at 7:57 pm on January 4, 2018: contributor

A few stylistic comments.

jonasschnelli force-pushed on Jan 9, 2018

jonasschnelli commented at 7:01 am on January 9, 2018: contributor

Rebased (needed due to the the node network test overhaul). Fixed @jimpo’s points.

jonasschnelli force-pushed on Jan 9, 2018

laanwj removed this from the milestone 0.16.0 on Jan 11, 2018

laanwj added this to the milestone 0.17.0 on Jan 11, 2018

Accept addresses with NODE_NETWORK_LIMITED flag 31c45a927e

Connect to peers signaling NODE_NETWORK_LIMITED when out-of-IBD 6fe57bdaac

[QA] Allow addrman loopback tests (add debug option -addrmantest) fa999affad

[QA] fix mininode CAddress ser/deser 158e1a6f0f

[QA] add NODE_NETWORK_LIMITED address relay and sync test 3f56df5b75

jonasschnelli commented at 9:02 am on February 9, 2018: contributor

Rebased.

jonasschnelli force-pushed on Feb 9, 2018

laanwj commented at 4:15 pm on February 14, 2018: member

utACK 3f56df5

Add setter for g_initial_block_download_completed eb9183535d

in src/protocol.h:281 in 3f56df5b75 outdated

277@@ -277,6 +278,7 @@ enum ServiceFlags : uint64_t {
278     // BIP process.
279 };
280 
281+extern std::atomic<bool> g_initial_block_download_completed;

laanwj commented at 4:19 pm on February 14, 2018:

I don’t like that we need to introduce a global here, especially one that has to be accessible externally (in the header file). Any drawback to moving GetDesirableServiceFlags to the .cpp instead?

jonasschnelli commented at 10:30 am on February 17, 2018: contributor

Added a commit that hides the new protocol.h/.cpp global (g_initial_block_download_completed) behinds a setter (@laanwj / #10387 (review)).

laanwj merged this on Mar 1, 2018

laanwj closed this on Mar 1, 2018

laanwj referenced this in commit 5c2aff8d95 on Mar 1, 2018

laanwj removed this from the "Blockers" column in a project

fanquake removed the label Needs release note on Mar 20, 2019

codablock referenced this in commit 1cf3a1e3ab on Sep 26, 2019

codablock referenced this in commit f8c310a974 on Sep 29, 2019

barrystyle referenced this in commit a9de008049 on Jan 22, 2020

PastaPastaPasta referenced this in commit 399155e6e1 on Jan 26, 2020

PastaPastaPasta referenced this in commit a0709c490b on Feb 13, 2020

PastaPastaPasta referenced this in commit 5d7dc51092 on Feb 27, 2020

PastaPastaPasta referenced this in commit f175a2e351 on Feb 27, 2020

PastaPastaPasta referenced this in commit 7b71e8b8b5 on Apr 16, 2020

PastaPastaPasta referenced this in commit d99fd0dfd0 on Apr 18, 2020

PastaPastaPasta referenced this in commit cccbc207db on Apr 18, 2020

UdjinM6 referenced this in commit 087d98477b on Apr 19, 2020

ckti referenced this in commit 2bcba6fb76 on Mar 28, 2021

ckti referenced this in commit 6b980f6243 on Mar 28, 2021

gades referenced this in commit f29be0db44 on Jun 30, 2021

gades referenced this in commit cb66db5d3a on Jun 30, 2021

gades referenced this in commit 0ef2ea3ee1 on Feb 10, 2022

DrahtBot locked this on Feb 15, 2022

Eventually connect to NODE_NETWORK_LIMITED peers #10387