[tests] Allow outbound & block-relay-only connections in functional tests. #19315
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK. Being able to explicitly test different connection types including outbounds will be really useful. |
|
Concept ACK, that's a substantial move to increase coverage of p2p! @t-bast, see https://github.com/ariard/bitcoin/commits/2020-07-tda-mitigation-block-relay, based on this PR, it should allow you to open manual block-relay-only connection to a side-node without leaking its presence due to addr-relay. That's one of the mitigation we talked offline against time-dilation and it should prevent you against unknown in-protocol eclipses. This is experimental software ofc, beware of not killing any kittens :) |
|
@t-bast I'd strongly encourage you to not use this branch or anything based on it for anything other than testing:
|
|
Thanks for the concerns @jnewbery, no worries I'm not planning on using anything else than official |
|
@jnewbery thanks to press on the warning. The provided commit is for experimentation-only and MUST NOT be used in production for reasons aforementioned. For anyone reading this and providing more context, Lightning nodes interested to prevent against time-dilation attacks (a no-hashrate eclipse variant against offchain protocols) should have multiple access to the chain view. As of today, the classic infra setup is to run the LN stack on top of one full-node, if this one gets sybilled and partitioned from the rest of the network, the channel funds are at risk. Running a self-hosted or controlled secondary node connected to your main node would mitigate at least eclipses due to weaknesses in addr management or peer selection. As of today, we do have automatic block-relay-only peers, for which in theory their topology should stay masked to an external observer. But a bug in peer selection may be leveraged to lure block-only-relay connections to attacker controlled-peers. Adding redundant manual block-relay-only connections will prevent this. Manual block-only secondary nodes won't fit there as they do leak addr-relay, which can be used to learn about their presence. Provided commit (ariard@ea99432) let you experiment such primary/secondary block-relay-only topology setup. There is no guarantee that anything will stay stable. If by playing with it, you do see value in this configuration, please concept ack and help to review downstream branches. If you don't understand what you're doing don't use it or ask questions. |
01e2830 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar) bc5d65b [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar) 2f2e13b [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar) 7f7b83d [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar) 35839e9 [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar) 4972c21 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar) 60156f5 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar) 7b322df [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar) 1492342 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar) 49efac5 [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar) d3698b5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar) 46578c0 [doc] Describe different connection types (Amiti Uttarwar) 442abae [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar) af59feb [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar) e1bc298 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar) 0e52a65 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar) 1521c47 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar) 26304b4 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar) 3f1b714 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar) Pull request description: **This is part 1 of #19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality. **This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions. This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types. (some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.) Overview of this PR: * rename `oneshot` to `addrfetch` * introduce `ConnectionType` enum * one by one, add different connection types to the enum * expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts) * fix the bug in counting different type of connections * some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive. ACKs for top commit: jnewbery: utACK 01e2830 laanwj: Code review ACK 01e2830, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall vasild: ACK 01e2830 sdaftuar: ACK 01e2830. fanquake: ACK 01e2830 - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now. jb55: wow this code was messy before... ACK 01e2830 Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
01e2830 [net] Remove unnecessary default args on CNode constructor (Amiti Uttarwar) bc5d65b [refactor] Remove IsOutboundDisconnectionCandidate (Amiti Uttarwar) 2f2e13b [net/refactor] Simplify multiple-connection checks (Amiti Uttarwar) 7f7b83d [net/refactor] Rework ThreadOpenConnections logic (Amiti Uttarwar) 35839e9 [net] Fix bug where AddrFetch connections would be counted as outbound full relay (Amiti Uttarwar) 4972c21 [net/refactor] Clarify logic for selecting connections in ThreadOpenConnections (Amiti Uttarwar) 60156f5 [net/refactor] Remove fInbound flag from CNode (Amiti Uttarwar) 7b322df [net/refactor] Remove m_addr_fetch member var from CNode (Amiti Uttarwar) 1492342 [net/refactor] Remove fFeeler flag from CNode (Amiti Uttarwar) 49efac5 [net/refactor] Remove m_manual_connection flag from CNode (Amiti Uttarwar) d3698b5 [net/refactor] Add connection type as a member var to CNode (Amiti Uttarwar) 46578c0 [doc] Describe different connection types (Amiti Uttarwar) 442abae [net/refactor] Add AddrFetch connections to ConnectionType enum (Amiti Uttarwar) af59feb [net/refactor] Extract m_addr_known logic from initializer list (Amiti Uttarwar) e1bc298 [net/refactor] Add block relay only connections to ConnectionType enum (Amiti Uttarwar) 0e52a65 [net/refactor] Add feeler connections to ConnectionType enum (Amiti Uttarwar) 1521c47 [net/refactor] Add manual connections to ConnectionType enum (Amiti Uttarwar) 26304b4 [net/refactor] Introduce an enum to distinguish type of connection (Amiti Uttarwar) 3f1b714 scripted-diff: Rename OneShot to AddrFetch (Amiti Uttarwar) Pull request description: **This is part 1 of bitcoin#19315, which enables the ability to test `outbound` and `block-relay-only` connections from the functional tests.** Please see that PR for more information of overall functionality. **This PR simplifies how we manage different connection types.** It introduces an enum with the various types of connections so we can explicitly define the connection type. The existing system relies on a series of independent flags, then has asserts scattered around to ensure that conflicting flags are not enabled at the same time. I find this approach to be both brittle and confusing. While making these changes, I found a small bug due to the silent assumptions. This PR also proposes a rename from `OneShot` to `AddrFetch`. I find the name `OneShot` to be very confusing, especially when we also have `onetry` manual connections. Everyone I've talked to offline has agreed that the name is confusing, so I propose a potential alternative. I think this is a good opportunity for a rename since I'm creating an enum to explicitly define the connection types. (some context for the unfamiliar: `oneshot` or `addrfetch` connections are short-lived connections created on startup. They connect to the seed peers, send a `getaddr` to solicit addresses, then close the connection.) Overview of this PR: * rename `oneshot` to `addrfetch` * introduce `ConnectionType` enum * one by one, add different connection types to the enum * expose the `conn_type` on CNode, and use this to reduce reliance on flags (& asserts) * fix the bug in counting different type of connections * some additional cleanup to simplify logic and make expectations explicit/inclusive rather than implicit/exclusive. ACKs for top commit: jnewbery: utACK 01e2830 laanwj: Code review ACK 01e2830, the commits are pretty straightforward to follow, and I think this is a move in the right direction overall vasild: ACK 01e2830 sdaftuar: ACK 01e2830. fanquake: ACK 01e2830 - I don't have as much experience with the networking code but these changes look fairly straight forward, the new code seems more robust/understandable and the additional documentation is great. I'm glad that a followup branch is already underway. There might be some more review comments here later today, so keep an eye on the discussion, however I'm going to merge this now. jb55: wow this code was messy before... ACK 01e2830 Tree-SHA512: 7bb644a6ed5849913d777ebc2ff89133ca0fbef680355a9a344e07496a979e6f9ff21a958e8eea93dcd7d5c343682b0c7174b1a3de380a4247eaae73da436e15
b43b7db to
e008486
Compare
|
Yay rebase! You can remove the "This PR builds on #19316. Please review that first." from the PR description. Is this PR now ready for review? |
6cc09a0 to
092da16
Compare
|
rebased |
|
reACK 092da16 With some help from The changes to the python asyncio pieces (renames, refactors, comments) are great! |
Add a new RPC endpoint to enable opening outbound connections from the tests. The functional test framework currently uses the addnode RPC, which has different behavior than general outbound peers. These changes enable creating both full-relay and block-relay-only connections. The new RPC endpoint calls through to a newly introduced AddConnection method on CConnman that ensures we stay within the allocated max.
In the interest of increasing our P2P test coverage, add support to create full-relay or block-relay-only connections. To support this, a P2P connection spins up a listening thread & uses a callback to trigger the node initiating the connection. Co-authored-by: Anthony Towns <aj@erisian.com.au>
…to helper. This is in preparation for use in the next commit.
Ensure we will disconnect if the peer sends us a transaction & we don't announce transactions to the peer.
Open max number of full-relay and block-relay-only connections from a functional test with different sorts of behaviors to ensure it behaves as expected.
092da16 to
b4dd2ef
Compare
|
rebased |
|
thanks for the re-review @troygiorshev ! should be trivial to re-ack, the rebase was just because of adjacent lines in |
| conn_gen_unsafe = loop.create_connection(lambda: self, host=self.dstaddr, port=self.dstport) | ||
| conn_gen = lambda: loop.call_soon_threadsafe(loop.create_task, conn_gen_unsafe) | ||
| return conn_gen | ||
| logger.debug('Connecting to Bitcoin Node: %s:%d' % (self.dstaddr, self.dstport)) |
There was a problem hiding this comment.
(Only if you have to retouch this branch for other reasons): use new style string formatting ("{}".format(var)) here, or even better, f-strings (f"{var}").
| # `proto` functions | ||
|
|
||
| listener = await cls.network_event_loop.create_server(peer_protocol, addr, port) | ||
| logger.debug("Listening server on %s:%d should be started" % (addr, port)) |
There was a problem hiding this comment.
I'm not sure what "should be" means here. I think the log could just say "Listening server on x started". Also, could use new style string formatting or f-strings.
This isn't a big deal. Only change it if you need to retouch the branch.
| conn.sync_with_ping() | ||
| assert(int(txid, 16) not in conn.get_invs()) | ||
|
|
||
| def check_p2p_tx_violation(self, index=1): |
There was a problem hiding this comment.
(re: #19315 (comment))
I don't think you need to pass the height at all. Just get the block at the tip:
diff --git a/test/functional/p2p_blocksonly.py b/test/functional/p2p_blocksonly.py
index c592ab52b1..e6c3f75955 100755
--- a/test/functional/p2p_blocksonly.py
+++ b/test/functional/p2p_blocksonly.py
@@ -80,7 +80,7 @@ class P2PBlocksOnly(BitcoinTestFramework):
# Ensure we disconnect if a block-relay-only connection sends us a transaction
self.nodes[0].add_outbound_p2p_connection(P2PInterface(), p2p_idx=0, connection_type="block-relay-only")
assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], False)
- _, txid, tx_hex = self.check_p2p_tx_violation(index=2)
+ _, txid, tx_hex = self.check_p2p_tx_violation()
self.log.info("Check that txs from RPC are not sent to blockrelay connection")
conn = self.nodes[0].add_outbound_p2p_connection(P2PTxInvStore(), p2p_idx=1, connection_type="block-relay-only")
@@ -97,9 +97,10 @@ class P2PBlocksOnly(BitcoinTestFramework):
conn.sync_with_ping()
assert(int(txid, 16) not in conn.get_invs())
- def check_p2p_tx_violation(self, index=1):
+ def check_p2p_tx_violation(self):
self.log.info('Check that txs from P2P are rejected and result in disconnect')
- input_txid = self.nodes[0].getblock(self.nodes[0].getblockhash(index), 2)['tx'][0]['txid']
+
+ input_txid = self.nodes[0].getblock(self.nodes[0].getbestblockhash(), 2)['tx'][0]['txid']
tx = create_transaction(self.nodes[0], input_txid, self.nodes[0].getnewaddress(), amount=(50 - 0.001))
txid = tx.rehash()
tx_hex = tx.serialize().hex()|
I think this is very close to being ready for merge. Previous ACKs: @troygiorshev (#19315 (comment)) |
|
reACK b4dd2ef Trivial from my last ACK :) For anyone interested resources on the python asyncio parts, the python asyncio docs, especially the transports and protocols page and the examples on the bottom of that page are useful! |
There was a problem hiding this comment.
Approach ACK b4dd2ef 🍢
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Approach ACK b4dd2ef8009703b81235e2d9a2a736a3a5e8152f 🍢
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi8SQwAtkpDpuzJotuEB7qpK2DCtuR6u/mOzV6KXL8G6W4NrPkWffPEc2DJ0+4b
Z2APDT0XKLjHSluduIQYkkJYewQNSn+kDbXqLG8fnGfHs0VPauSPr1YEWJ6ckM0C
G9YKu02lX0P7iIao+PlLCUc/NRrG1eo3FmS8RCQBzn1CfnBXviwJAK3Btau6r2vW
CC/4tyBANDD9cDqpS81bpWdnA/qggLjP/0p/cP3FIJ5kUVTdsfFhvnXW8rvk48bB
hREsZxEP/HjF6/Tw91C4FR+AtOL3uwll94kgbf99U/ld++98TeEEZmOHu9+vVYF7
p1mfaVevvWAcY8r+88nSy99KKecEt5DxCW56MFnB+Xlhn2DCLZWNNjiWQDB9sxDo
jEQlDiRbypIqGlVwL0qCoJB4bCOaz+qhkErqJgEIHXL3wV7HVk1vkim08r5sIUPm
rz7BY2lVy0lQpJAmQ2NUeZ3Ioms/fnvmUZJSFYFP5zXkX6ZXKdtdGTtCP2VC9iTT
/68NLVfe
=gZ6a
-----END PGP SIGNATURE-----
Timestamp of file with hash 3232b0dd96dd7ba9660c084f1b93bc4661873271630eb1ba4b50fb2f2cb5803b -
| * - Max total outbound connection capacity filled | ||
| * - Max connection capacity for type is filled | ||
| */ | ||
| bool AddConnection(const std::string& address, ConnectionType conn_type); |
There was a problem hiding this comment.
If this is only used in test, why not call it AddTestConnection? Otherwise someone might use it in "production".
| RPCResult::Type::OBJ, "", "", | ||
| { | ||
| { RPCResult::Type::STR, "address", "Address of newly added connection." }, | ||
| { RPCResult::Type::STR, "connection_type", "Type of connection opened." }, |
There was a problem hiding this comment.
I still think this should be removed. If this can't be parsed, a parse-failure will already be thrown.
Though, can be done in a follow-up to not invalidate reviews.
Unrelated, but posels' law is probably not applicable to security sensitive software, see http://www.langsec.org/papers/postel-patch.pdf
|
|
||
| RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VSTR}); | ||
| const std::string address = request.params[0].get_str(); | ||
| const std::string conn_type_in{TrimString(request.params[1].get_str())}; |
There was a problem hiding this comment.
Same commit: Any reason to trim the string? We don't do this anywhere else.
There was a problem hiding this comment.
This was my fault--I suggested it earlier on.
| @@ -71,6 +71,7 @@ def __init__(self, i, datadir, *, chain, rpchost, timewait, timeout_factor, bitc | |||
| """ | |||
|
|
|||
| self.index = i | |||
| self.p2p_conn_index = 1 | |||
| self.log.debug("Connecting to %s:%d %s" % (address, port, connection_type)) | ||
| self.addconnection('%s:%d' % (address, port), connection_type) | ||
|
|
||
| p2p_conn.peer_accept_connection(connect_cb=addconnection_callback, connect_id=p2p_idx + 1, net=self.chain, timeout_factor=self.timeout_factor, **kwargs)() |
There was a problem hiding this comment.
3997ab9: The +1 seems like a layer violation that should be done in the p2p module instead.
| coroutine = loop.create_connection(lambda: self, host=self.dstaddr, port=self.dstport) | ||
| return lambda: loop.call_soon_threadsafe(loop.create_task, coroutine) | ||
|
|
||
| def peer_accept_connection(self, connect_id, connect_cb=lambda: None, *, net, timeout_factor): |
There was a problem hiding this comment.
Any reason to add default arguments for an arg that is set in all call sites? I don't think it makes sense to skip the connect_cb here?
Also could enforce named args with *
| @@ -414,6 +435,10 @@ def test_function(): | |||
|
|
|||
| wait_until_helper(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor) | |||
|
|
|||
| def wait_for_connect(self, timeout=60): | |||
| test_function = lambda: self.is_connected | |||
| wait_until_helper(test_function, timeout=timeout, lock=p2p_lock) | |||
There was a problem hiding this comment.
Would be nice to not use the helper unless necessary. It doesn't matter here, but it won't propagate the scaling factor
| @@ -542,6 +569,48 @@ def close(self, timeout=10): | |||
| # Safe to remove event loop. | |||
| NetworkThread.network_event_loop = None | |||
|
|
|||
| @classmethod | |||
| def listen(cls, p2p, callback, port=None, addr=None, idx=1): | |||
There was a problem hiding this comment.
same commit:
What is the point of allowing those args when they are never set? I'd say if there is ever a test that needs them set, that pull can make the needed changes here. Because other changes are required for this anyway to work.
| @@ -51,13 +40,13 @@ def run_test(self): | |||
|
|
|||
| self.log.info('Check that txs from rpc are not rejected and relayed to other peers') | |||
| assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], True) | |||
| txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid'] | |||
|
|
|||
| assert_equal(self.nodes[0].testmempoolaccept([tx_hex])[0]['allowed'], True) | |||
There was a problem hiding this comment.
99791e7: If you touch this, I'd prefer to remove it. This doesn't check anything
| @@ -67,8 +56,7 @@ def run_test(self): | |||
| assert_equal(peer_1_info['permissions'], ['relay']) | |||
| peer_2_info = self.nodes[0].getpeerinfo()[1] | |||
| assert_equal(peer_2_info['permissions'], ['relay']) | |||
| assert_equal(self.nodes[0].testmempoolaccept([sigtx])[0]['allowed'], True) | |||
| txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid'] | |||
| assert_equal(self.nodes[0].testmempoolaccept([tx_hex])[0]['allowed'], True) | |||
Add a new RPC endpoint to enable opening outbound connections from the tests. The functional test framework currently uses the addnode RPC, which has different behavior than general outbound peers. These changes enable creating both full-relay and block-relay-only connections. The new RPC endpoint calls through to a newly introduced AddConnection method on CConnman that ensures we stay within the allocated max. Github-Pull: bitcoin#19315 Rebased-From: 5bc04e8
In the interest of increasing our P2P test coverage, add support to create full-relay or block-relay-only connections. To support this, a P2P connection spins up a listening thread & uses a callback to trigger the node initiating the connection. Co-authored-by: Anthony Towns <aj@erisian.com.au> Github-Pull: bitcoin#19315 Rebased-From: 3997ab9
…ests.
Summary:
```
The existing functional test framework uses the addnode RPC to spin up manual connections between bitcoind nodes. This limits our ability to add integration tests for our networking code, which often executes different code paths for different connection types.
This PR enables creating outbound & block-relay-only P2P connections in the functional tests. This allows us to increase our p2p test coverage, since we can now verify expectations around these connection types.
This builds out the prototype proposed by ajtowns in #14210. 🙌🏽
An overview of this branch:
introduces a new test-only RPC function addconnection which initiates opening an outbound or block-relay-only connection. (conceptually similar to addnode but for different connection types & restricted to regtest)
adds test_framework support so a mininode can open an outbound/block-relay-only connection to a P2PInterface/P2PConnection.
updates p2p_blocksonly tests to create a block-relay-only connection & verify expectations around transaction relay.
introduces p2p_add_connections test that checks the behaviors of the newly introduced add_outbound_p2p_connection test framework function.
With these changes, there are many more behaviors that we can add integration tests for. The blocksonly updates is just one example.
```
Backport of [[bitcoin/bitcoin#19315 | core#19315]].
Test Plan:
ninja all check-all
Reviewers: #bitcoin_abc, PiRK
Reviewed By: #bitcoin_abc, PiRK
Differential Revision: https://reviews.bitcoinabc.org/D10836
The existing functional test framework uses the
addnodeRPC to spin up manual connections between bitcoind nodes. This limits our ability to add integration tests for our networking code, which often executes different code paths for different connection types.This PR enables creating
outbound&block-relay-onlyP2P connections in the functional tests. This allows us to increase our p2p test coverage, since we can now verify expectations around these connection types.This builds out the prototype proposed by ajtowns in #14210. 🙌🏽
An overview of this branch:
addconnectionwhich initiates opening anoutboundorblock-relay-onlyconnection. (conceptually similar toaddnodebut for different connection types & restricted to regtest)test_frameworksupport so a mininode can open anoutbound/block-relay-onlyconnection to aP2PInterface/P2PConnection.p2p_blocksonlytests to create ablock-relay-onlyconnection & verify expectations around transaction relay.p2p_add_connectionstest that checks the behaviors of the newly introducedadd_outbound_p2p_connectiontest framework function.With these changes, there are many more behaviors that we can add integration tests for. The blocksonly updates is just one example.
Huge props to ajtowns for conceiving the approach & providing me feedback as I've built out this branch. Also thank you to jnewbery for lots of thoughtful input along the way.