Implement BIP159 NODE_NETWORK_LIMITED (pruned peers) signaling only #11740

jonasschnelli · 2017-11-20T21:02:12Z

Extracted from #10387.
Does implement BIP159, but only the signalling part. No connections are made to NODE_NETWORK_LIMITED in this PR.

The address relay and connection work (the more complicated part) can then be separated (probably in #10387).

fanquake · 2017-11-20T23:26:19Z

e874478a42eded1c5952fc0407f04279dd4837c1 forgot to add node_network_limited.py to test_runner.py.

jonasschnelli · 2017-11-20T23:28:40Z

@fanquake! Thanks. Done.

gmaxwell

utACK

laanwj · 2017-11-29T11:10:20Z

utACK af05b92

theuni · 2017-11-30T18:50:50Z

utACK af05b92591b72c34f8fa04a0917b3c1225e2417c

TheBlueMatt · 2017-12-04T15:49:56Z

src/net_processing.cpp

@@ -1091,6 +1091,16 @@ void static ProcessGetData(CNode* pfrom, const Consensus::Params& consensusParam
                    pfrom->fDisconnect = true;
                    send = false;
                }
+                // Avoid leaking prune-height by never sending blocks below the NODE_NETWORK_LIMITED threshold
+                if (send && !pfrom->fWhitelisted && (
+                        (((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) )


This needs a buffer beyond NODE_NETWORK_LIMITED_MIN_BLOCK. Because it gets a disconnect, one or two blocks should suffice.

TheBlueMatt · 2017-12-04T15:50:31Z

src/net_processing.cpp

+                   )) {
+                    LogPrint(BCLog::NET, "Ignore block request below NODE_NETWORK_LIMITED threshold from peer=%d\n", pfrom->GetId());
+
+                    //disconnect node


Can you fill out this comment a bit more? Something about how we could otherwise stall a node if we were just finishing IBD and announced blocks to our peers which they wanted?

TheBlueMatt · 2017-12-04T15:53:30Z

test/functional/node_network_limited.py

+        except:
+            pass
+        assert_equal(res, result)
+        self.nodes[0].disconnect_p2ps()


Can you move this into the try block? This checks that the node disconnected us, we didn't just timeout in the fail case.

The current pruning implementation does ensure to always conform to BIP159

…rmeasure)

jonasschnelli · 2017-12-05T21:21:32Z

Fixed points reported by @TheBlueMatt.
Had to rebase and therefor amend changed the fixes.
The only code change is the two blocks buffer extension for possible race conditions.

TheBlueMatt · 2017-12-06T21:51:58Z

test/functional/node_network_limited.py

+        try:
+            node.wait_for_block(int(blockhash, 16), 3)
+            res = True
+            assert_equal(res, result)


It seems we never check anything if we got disconnected, I guess this check needs to move to after the except?

jonasschnelli · 2017-12-07T07:14:23Z

Improved the disconnect test (amend commit).

TheBlueMatt · 2017-12-08T15:54:27Z

utACK de74c62

jonasschnelli · 2017-12-08T21:10:05Z

@laanwj, @gmaxwell: mind to re-ack?

laanwj · 2017-12-09T07:38:22Z

utACK de74c62

…ignaling only* de74c62 [Doc] Update bip.md, add support for BIP 159 (Jonas Schnelli) e054d0e [QA] Add node_network_limited test (Jonas Schnelli) bd09416 Avoid leaking the prune height through getdata (fingerprinting countermeasure) (Jonas Schnelli) 27df193 Always set NODE_NETWORK_LIMITED bit (Jonas Schnelli) 7caba38 Add NODE_NETWORK_LIMITED flags and min block amount constants (Jonas Schnelli) Pull request description: Extracted from #10387. Does implement BIP159, but only the signalling part. No connections are made to NODE_NETWORK_LIMITED in this PR. The address relay and connection work (the more complicated part) can then be separated (probably in #10387). Tree-SHA512: e3218eb4789a9320b0f42dc10f62d30c13c49bdef00443fbe653bee22933477adcfc1cf8f6a95269324560b5721203ed41f3c5e2dd8a98ec2791f6a9d8346b1a

jnewbery · 2017-12-11T18:42:23Z

test is racy:

node_network_limited.py failed, Duration: 61 s

stdout:
2017-12-11 18:39:35.906000 TestFramework (INFO): Initializing test directory /tmp/user/1000/bitcoin_test_runner_20171211_183839/node_network_limited_227
2017-12-11 18:39:36.171000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:12816
2017-12-11 18:39:36.498000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:12816
2017-12-11 18:39:36.604000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:12816
2017-12-11 18:39:36.757000 TestFramework.mininode (INFO): Connecting to Bitcoin Node: 127.0.0.1:12816
2017-12-11 18:40:36.768000 TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/home/ubuntu/bitcoin/test/functional/test_framework/test_framework.py", line 118, in main
    self.run_test()
  File "/home/ubuntu/bitcoin/test/functional/node_network_limited.py", line 65, in run_test
    self.tryGetBlockViaGetData(blocks[0], True) #first block outside of the 288+2 limit
  File "/home/ubuntu/bitcoin/test/functional/node_network_limited.py", line 33, in tryGetBlockViaGetData
    node.wait_for_verack()
  File "/home/ubuntu/bitcoin/test/functional/test_framework/mininode.py", line 373, in wait_for_verack
    wait_until(test_function, timeout=timeout, lock=mininode_lock)
  File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 222, in wait_until
    assert_greater_than(timeout, time.time())
  File "/home/ubuntu/bitcoin/test/functional/test_framework/util.py", line 42, in assert_greater_than
    raise AssertionError("%s <= %s" % (str(thing1), str(thing2)))
AssertionError: 1513017636.75866 <= 1513017636.7688699
2017-12-11 18:40:36.769000 TestFramework (INFO): Stopping nodes
2017-12-11 18:40:36.931000 TestFramework (WARNING): Not cleaning up dir /tmp/user/1000/bitcoin_test_runner_20171211_183839/node_network_limited_227
2017-12-11 18:40:36.931000 TestFramework (ERROR): Test failed. Test logging available at /tmp/user/1000/bitcoin_test_runner_20171211_183839/node_network_limited_227/test_framework.log
2017-12-11 18:40:36.931000 TestFramework (ERROR): Hint: Call /home/ubuntu/bitcoin/test/functional/combine_logs.py '/tmp/user/1000/bitcoin_test_runner_20171211_183839/node_network_limited_227' to consolidate all logs

fix is here: https://github.com/jnewbery/bitcoin/tree/improve_node_network_test . I'll open a PR.

jonasschnelli · 2017-12-11T18:43:37Z

Thanks @jnewbery!

ee5efad [tests] refactor node_network_limited (John Newbery) b425131 [tests] remove redundant duplicate tests from node_network_limited (John Newbery) 2e02984 [tests] node_network_limited - remove race condition (John Newbery) dbfe294 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery) 1285312 [tests] fix flake8 warnings in node_network_limited.py (John Newbery) Pull request description: Fixes race condition in the node_network_limited test case introduced in #11740. Also tidies up the test and removes redundant duplicate tests. Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011

…ers) *signaling only* de74c62 [Doc] Update bip.md, add support for BIP 159 (Jonas Schnelli) e054d0e [QA] Add node_network_limited test (Jonas Schnelli) bd09416 Avoid leaking the prune height through getdata (fingerprinting countermeasure) (Jonas Schnelli) 27df193 Always set NODE_NETWORK_LIMITED bit (Jonas Schnelli) 7caba38 Add NODE_NETWORK_LIMITED flags and min block amount constants (Jonas Schnelli) Pull request description: Extracted from bitcoin#10387. Does implement BIP159, but only the signalling part. No connections are made to NODE_NETWORK_LIMITED in this PR. The address relay and connection work (the more complicated part) can then be separated (probably in bitcoin#10387). Tree-SHA512: e3218eb4789a9320b0f42dc10f62d30c13c49bdef00443fbe653bee22933477adcfc1cf8f6a95269324560b5721203ed41f3c5e2dd8a98ec2791f6a9d8346b1a

ee5efad [tests] refactor node_network_limited (John Newbery) b425131 [tests] remove redundant duplicate tests from node_network_limited (John Newbery) 2e02984 [tests] node_network_limited - remove race condition (John Newbery) dbfe294 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery) 1285312 [tests] fix flake8 warnings in node_network_limited.py (John Newbery) Pull request description: Fixes race condition in the node_network_limited test case introduced in bitcoin#11740. Also tidies up the test and removes redundant duplicate tests. Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011

…ers) *signaling only* de74c62 [Doc] Update bip.md, add support for BIP 159 (Jonas Schnelli) e054d0e [QA] Add node_network_limited test (Jonas Schnelli) bd09416 Avoid leaking the prune height through getdata (fingerprinting countermeasure) (Jonas Schnelli) 27df193 Always set NODE_NETWORK_LIMITED bit (Jonas Schnelli) 7caba38 Add NODE_NETWORK_LIMITED flags and min block amount constants (Jonas Schnelli) Pull request description: Extracted from bitcoin#10387. Does implement BIP159, but only the signalling part. No connections are made to NODE_NETWORK_LIMITED in this PR. The address relay and connection work (the more complicated part) can then be separated (probably in bitcoin#10387). Tree-SHA512: e3218eb4789a9320b0f42dc10f62d30c13c49bdef00443fbe653bee22933477adcfc1cf8f6a95269324560b5721203ed41f3c5e2dd8a98ec2791f6a9d8346b1a

ee5efad [tests] refactor node_network_limited (John Newbery) b425131 [tests] remove redundant duplicate tests from node_network_limited (John Newbery) 2e02984 [tests] node_network_limited - remove race condition (John Newbery) dbfe294 [tests] define NODE_NETWORK_LIMITED in test framework (John Newbery) 1285312 [tests] fix flake8 warnings in node_network_limited.py (John Newbery) Pull request description: Fixes race condition in the node_network_limited test case introduced in bitcoin#11740. Also tidies up the test and removes redundant duplicate tests. Tree-SHA512: a5240fe35509d81a47c3d3b141a956378675926093e658d24be43027b20d3b5f0ba7c6017c8208487a1849d4fdfb911a361911d571423db7c50711250aba3011

jonasschnelli added the P2P label Nov 20, 2017

jonasschnelli force-pushed the 2017/11/NNL_signaling branch from a26d177 to af05b92 Compare November 20, 2017 23:28

gmaxwell approved these changes Nov 23, 2017

View reviewed changes

fanquake changed the title Implement BIP159 NODE_NETWORK_LIMITED (pruned peers) *signalling only* Implement BIP159 NODE_NETWORK_LIMITED (pruned peers) *signaling only* Nov 29, 2017

TheBlueMatt reviewed Dec 4, 2017

View reviewed changes

jonasschnelli added 3 commits December 5, 2017 11:03

Add NODE_NETWORK_LIMITED flags and min block amount constants

7caba38

Always set NODE_NETWORK_LIMITED bit

27df193

The current pruning implementation does ensure to always conform to BIP159

Avoid leaking the prune height through getdata (fingerprinting counte…

bd09416

…rmeasure)

jonasschnelli force-pushed the 2017/11/NNL_signaling branch from f06c84e to d57a4e5 Compare December 5, 2017 21:19

TheBlueMatt reviewed Dec 6, 2017

View reviewed changes

jonasschnelli added 2 commits December 6, 2017 21:13

[QA] Add node_network_limited test

e054d0e

[Doc] Update bip.md, add support for BIP 159

de74c62

jonasschnelli force-pushed the 2017/11/NNL_signaling branch from d57a4e5 to de74c62 Compare December 7, 2017 07:14

laanwj merged commit de74c62 into bitcoin:master Dec 9, 2017

jnewbery mentioned this pull request Dec 11, 2017

Improve node network test #11867

Merged

meshcollider mentioned this pull request Jan 17, 2018

TODO for release notes 0.16.0 #11054

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Implement BIP159 NODE_NETWORK_LIMITED (pruned peers) *signaling only* #11740

Implement BIP159 NODE_NETWORK_LIMITED (pruned peers) *signaling only* #11740

Uh oh!

Conversation

jonasschnelli commented Nov 20, 2017

Uh oh!

fanquake commented Nov 20, 2017

Uh oh!

jonasschnelli commented Nov 20, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 29, 2017

Uh oh!

theuni commented Nov 30, 2017

Uh oh!

TheBlueMatt Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Dec 4, 2017

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Dec 5, 2017

Uh oh!

TheBlueMatt Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Dec 7, 2017

Uh oh!

TheBlueMatt commented Dec 8, 2017

Uh oh!

jonasschnelli commented Dec 8, 2017

Uh oh!

laanwj commented Dec 9, 2017

Uh oh!

jnewbery commented Dec 11, 2017

Uh oh!

jonasschnelli commented Dec 11, 2017

Uh oh!

Uh oh!

Implement BIP159 NODE_NETWORK_LIMITED (pruned peers) signaling only #11740

Implement BIP159 NODE_NETWORK_LIMITED (pruned peers) signaling only #11740