This PR extends our functional test coverage of net_processing.cpp, following the overhaul of transaction request.
It extends p2p_ibd_txrelay.py to verify that we don’t request transaction during IBD, even lawfully ones from tx-relay peers. It also move network constants/methods in p2p.py to serve generically across the test framework.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
3024+ // transaction or this transaction is already double-spend but not yet seen block.
3025+ // Further, we can't assert accuracy of its feerate.
3026+ //
3027+ // Orphan transaction is discarded. Note, we don't both to add id to recentRejects
3028+ // as it's expected to be quickly clean up during IBD.
3029+ if (ChainstateActive().IsInitialBlockDownload()) return;
if (::ChainstateActive().IsInitialBlockDownload()) return;
d65dca43 partial WIP suggestions, though I think it could still be more clearly written
0- // To save bandwidth during IBD, stop orphan processing early. With the state of
1- // our UTXO set far from tip, we might not have yet the unspent output spend by this
2- // transaction or this transaction is already double-spend but not yet seen block.
3- // Further, we can't assert accuracy of its feerate.
4+ // If we are in IBD, exit from orphan processing early to save bandwidth. With the
5+ // state of our UTXO set far from the tip, we might not yet have the unspent output
6+ // of this transaction, or the transaction is already double-spent but not yet
7+ // included in a block. We also cannot assert the accuracy of its feerate.
8 //
9- // Orphan transaction is discarded. Note, we don't both to add id to recentRejects
10- // as it's expected to be quickly clean up during IBD.
11- if (ChainstateActive().IsInitialBlockDownload()) return;
12+ // Orphan transaction is discarded. We don't bother to add it to recentRejects, as
13+ // it's expected to be quickly cleaned up during IBD.
14+ if (::ChainstateActive().IsInitialBlockDownload()) return;
“it’s expected” is somewhat ambiguous, could you use an explicit subject?
115@@ -116,6 +116,9 @@
116 "signet": b"\x0a\x03\xcf\x40", # signet
117 }
118
119+# Constants from net_processing
120+INBOUND_PEER_TX_DELAY = 2 # seconds
121+TXID_RELAY_DELAY = 2 # seconds
78b1044 Could you update the name of INBOUND_PEER_TX_RELAY for clarity and searchability of its equivalent in the net_processing code?
0TXID_RELAY_DELAY = 2 # seconds
405@@ -401,6 +406,11 @@ def on_version(self, message):
406 self.send_message(msg_sendaddrv2())
407 self.nServices = message.nServices
408
409+ def on_getdata(self, message):
410+ for i in message.inv:
411+ if i.type & MSG_TYPE_MASK == MSG_TX or i.type & MSG_TYPE_MASK == MSG_WTX:
78b1044
0 if (i.type & MSG_TYPE_MASK) in {MSG_TX, MSG_WTX}:
85+
86+ peer = self.nodes[0].add_p2p_connection(P2PInterface())
87+ peer.send_message(msg_tx(tx_orphan))
88+ # As tx-announcing connection is inbound, wait for the inbound delay applied by requester.
89+ # Also apply the txid delay as orphan parent are requested in the legacy way.
90+ sleep(INBOUND_PEER_TX_DELAY+TXID_RELAY_DELAY)
8afa4fa s/parent are/parents are/
0 sleep(INBOUND_PEER_TX_DELAY + TXID_RELAY_DELAY)
34+ OP_TRUE,
35+ OP_0,
36+)
37+from test_framework.util import (
38+ assert_equal,
39+)
feefilter message. For a node with incoming connections, minor traffic waste doesn’t seem to be a concern.
Right, I should have explicit the assumption on sender, that they are non-Bitcoin Core clients. Thus we can’t be certain they enforce the regular tx-relay flow inv,getdata,tx but might send directly txn, don’t bothering with any feefilter at all nor sending an inv first.
I agree that the waste is likely to be minor in any-case, but note that this PR is aiming to align behavior with current inv-reception code (L2690) as described in PR description. This point has the added benefit to make easier to reason on holistic tx-relay behavior when we review more important p2p stuff, IMHO.
EDIT: this was for a different version of this PR.
This seems like an extremely verbose PR to modify an unlikely and unimportant edge case. We don’t expect to process unconfirmed transactions during IBD for a couple of reasons:
So a peer would need to send us an unsolicited tx ignoring our feefilter for us to even reach this logic. It seems inconsistent to process that transaction as normal, but not do the normal orphan processing logic too.
We could modify the transaction processing logic to ignore all tx messages during IBD, which seems reasonable and at least consistent. I don’t think we need a 10 line comment and 100 line test case to justify that - just say that we don’t process unconfirmed transactions during IBD.
It seems inconsistent to process that transaction as normal, but not do the normal orphan processing logic too.
Thanks to answering my wonder about jumping directly to stop processing unrequested transaction instead of only scoping to orphan. Updated branch with this behavior.
I don’t think we need a 10 line comment and 100 line test case to justify that - just say that we don’t process unconfirmed transactions during IBD.
I hear you on that, some reviewers might like comments on rational not only on code behaviors, even for edge cases, hard to all agree on. Note that “100 line test case” includes a new test (test_inv_ibd) for old behavior, not processing inv-tx during IBD.
Note to reviewers, you might verify test_inv_ibd coverage with:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index c21c9e3ee..5def5178e 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -2687,7 +2687,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
5 LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId());
6 pfrom.fDisconnect = true;
7 return;
8- } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
9+ } else if (!fAlreadyHave) {
10 AddTxAnnouncement(pfrom, gtxid, current_time);
11 }
12 } else {
And test_unrequested_tx_ibd:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index c21c9e3ee..32cdba5f2 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -2928,8 +2928,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
5 // and mitigate potential DoS risks.
6 if (!pfrom.HasPermission(PF_RELAY) && ::ChainstateActive().IsInitialBlockDownload())
7 {
8- LogPrint(BCLog::NET, "unrequested transaction while in IBD from peer=%d\n", pfrom.GetId());
9- return;
10+ //LogPrint(BCLog::NET, "unrequested transaction while in IBD from peer=%d\n", pfrom.GetId());
11+ //return;
12 }
13
14 CTransactionRef ptx;
2923@@ -2924,6 +2924,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2924 return;
2925 }
2926
2927+ // If we are in IBD, exit from unrequested transaction processing early to save bandwidth
2928+ // and mitigate potential DoS risks.
2929+ if (!pfrom.HasPermission(PF_RELAY) && ::ChainstateActive().IsInitialBlockDownload())
0 if (!pfrom.HasPermission(PF_RELAY) && m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
style-nit: Please don’t use the deprecated wrappers. This is going to silently conflict with ongoing work to remove them
Also clang-format for new code
70+ script_pubkey = CScript([OP_0, witness_hash])
71+
72+ unrequested_tx = CTransaction()
73+ unrequested_tx.vin.append(CTxIn(outpoint=COutPoint(uint256_from_str(b"23c70ed7c0506e9178fc1a987f40a33946d4ad4c962b5ae3a52546da53af0c5c"),
74+ 0)))
75+ unrequested_tx.vout.append(CTxOut(nValue=50 * COIN - 12000, scriptPubKey=script_pubkey))
0 or default constructors for CTxIn/Out should be sufficient.
74+ 0)))
75+ unrequested_tx.vout.append(CTxOut(nValue=50 * COIN - 12000, scriptPubKey=script_pubkey))
76+ unrequested_tx.calc_sha256()
77+
78+ peer = self.nodes[0].add_p2p_connection(P2PInterface())
79+ peer.send_message(msg_tx(unrequested_tx))
0 peer.send_and_ping(msg_tx(unrequested_tx))
Otherwise you are using the debug.log to synchronize. And the assert_equal(len(self.nodes[0].getrawmempool()), 0) doesn’t actually check anything.
sync_with_ping inside the with statement. Test is breaking as expected with new code muted. Let me know if it achieves what you intended.
5@@ -6,8 +6,27 @@
6
2923@@ -2924,6 +2924,14 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2924 return;
2925 }
2926
2927+ // If we are in IBD, exit from unrequested transaction processing early to save bandwidth
2928+ // and mitigate potential DoS risks.
63+ unrequested_tx.vout.append(CTxOut())
64+ unrequested_tx.calc_sha256()
65+
66+ peer = self.nodes[0].add_p2p_connection(P2PInterface())
67+ peer.send_message(msg_tx(unrequested_tx))
68+ with self.nodes[0].assert_debug_log(expected_msgs=['unrequested transaction while in IBD from peer=2'], unexpected_msgs=['from peer=2 was not accepted: scriptpubkey']):
Thanks for the tip, removed it. I’ve searched for a while for this but I don’t think we have better than checking the logs to test that feeding the node with some input (an unrequested tx) is effectively rejected by the targeted code check (the new check L2928) and not by the mempool checks further down in the code path ?
I’m still ramping up on our p2p functional test framework :)
~Concept ACK. Much prefer this more general approach to the original branch.~
EDIT: this was for a different version of this PR.
If you want the ci to run on this, you might have to rebase on current master.
Edit: Or maybe not
Do you have any thoughts on my suggestion from before, about just always ignoring unrequested transactions? #20277 (comment).
It’s not really clear to me why we’re more concerned about this behavior during IBD than outside of IBD.
@sdaftuar To answer I didn’t propose to stop processing of unrequested transactions even out-of-IBD because I was worried to break bitcoin clients which don’t respect the canonical tx-request sequence and thus smashing out their txn broadcast ?
If we think such clients are non-existent, extending TxRequestTracker with a bool ExpectedResponse() should be possible.
Rebased at 886d05d.
See IRC discussion with concerns about potentially breaking bitcoin software if we go with Suhas suggestion: http://gnusha.org/bitcoin-core-dev/2020-12-29.log (starting at 23:10)
See IRC discussion with concerns about potentially breaking bitcoin software if we go with Suhas suggestion: http://gnusha.org/bitcoin-core-dev/2020-12-29.log (starting at 23:10)
From that IRC conversation:
15:24 < phantomcircuit> im also not sure the core concept of ignoring those transactions makes a lot of sense 15:25 < phantomcircuit> messages from peers are handled round robin so the worst an attacker can do is add latency and make one core run at 100%
Yes, an attacker can add latency – a substantial amount, for free, by maintaining multiple inbound connections to a listening node and sending expensive to validate transactions that are not accepted to the mempool (eg for policy failures during script validation). The last time I looked at this, it seemed like spending on the order of 1 second (on my modern workstation) to validate a transaction that would ultimately fail mempool acceptance was easily achievable; multiply that by the number of inbound connections we allow and you get a pretty bad state of affairs…
By requiring an INV/getdata sequence before processing, we would be able to deprioritize announcements from peers that are DoSing us in this way. A first step to that is establishing that processing unrequested transactions is not a required or expected behavior on the network.
15:42 < sipa> i’m not so much worried about such clients on the public network (we’d know, or at least be able to add monitoring for that to figure out), but there could be software communicating locally with bitcoin core (things like joinmarket, lightning clients, indexers, …), for which we wouldn’t really know 15:43 < sipa> of course, perhaps one generally sets PF_RELAY for those or so
Agreed that PF_RELAY could be used to bypass what I’m suggesting.
We also move some constants from net_processing in p2p.py
65+ Previously, a transaction message was always submitted for processing, even if a
66+ getdata for this identifier have not been previously issued. Henceforth, such
67+ unrequested transaction will be rejected without any processing to mitigate about
68+ DoS risks. Bitcoin clients on the network relying on unannounced transaction broadcast
69+ should adapt their tx-relay to the compliant tx-request sequence. Peers might be
70+ authorized to bypass this check with the PF_RELAY permission. (#20277)
I agree on the mailing list communication. By the way, my concern about breaking tx-broadcast capabilities of such clients was one of the reason for all the back-and-forth on this PR. I expect those clients to be connected to at least few different peers and among them you can assume a subset will be prior versions still supporting processing of unrequested transactions. I would say deployment time give them a window of adaptation.
That said, do you think I should announce on the mailing list now, even before this PR gets merged or only after ?
270+ with self.nodes[0].assert_debug_log(expected_msgs=['from peer=0 was not accepted: scriptpubkey']):
271+ peer.sync_with_ping()
272+ assert_equal(len(self.nodes[0].getrawmempool()), 0)
273+ peer.peer_disconnect()
274+ peer.wait_for_disconnect()
275+
Seems that assert_equal(len(self.nodes[0].getrawmempool()), 0) isn’t really doing anything since the tx being used is always invalid so we don’t ever expect it to be accepted to the mempool.
Suggest using a valid tx instead and then testing the changes to the mempool directly (count=0 if tx msg is received before being requested and count=1 if tx msg is received after tx is requested) instead of doing an implicit check via the logs using assert_debug_log.
That’s a good suggestion. Initially I was using valid transactions to feed the node and changed it after this comment. I guess the getrawmempool is a leftover. Will remove it on #21224.
I think it’s still fine to use invalid transactions as the behavior under check is “Does this transaction reach mempool acceptance ?”, which is probed through the assert_debug_log. “Is this transaction mempool-valid ?” is already a distinct checked behavior, of which we’re not interested here ?
44+ def test_inv_ibd(self):
45+ self.log.info("Check that nodes don't request txn while still in IBD")
46+ peer = self.nodes[0].add_p2p_connection(P2PInterface())
47+ peer.send_message(msg_inv([CInv(t=MSG_WTX, h=0xffaa)]))
48+ # As tx-announcing connection is inbound, wait for the inbound delay applied by requester.
49+ sleep(NONPREF_PEER_TX_DELAY)
ACK ec04fa50c703e726f2315195892d22c4486bf31a
The test makes sense. I’d say it’s gonna be easier to get this merged if you open a separate clean PR. As someone who looked at it for the first time, I found it distracting going through all irrelevant legacy comments.
326@@ -324,6 +327,9 @@ def __init__(self, support_addrv2=False, wtxidrelay=True):
327 # If the peer supports wtxid-relay
328 self.wtxidrelay = wtxidrelay
329
330+ # Getdata counter for tx-relay related tests
331+ self.tx_getdata_count = 0
5b2ab873f7c793ea8ce871260b35 With this change can remove these lines?
0--- a/test/functional/p2p_tx_download.py
1+++ b/test/functional/p2p_tx_download.py
2@@ -117,9 +117,6 @@ class TxDownloadTest(BitcoinTestFramework):
3
4 p = self.nodes[0].p2ps[0]
5
6- with p2p_lock:
7- p.tx_getdata_count = 0
8-
9 mock_time = int(time.time() + 1)
68@@ -37,6 +69,9 @@ def run_test(self):
69 assert not node.getblockchaininfo()['initialblockdownload']
70 self.wait_until(lambda: all(peer['minfeefilter'] == NORMAL_FEE_FILTER for peer in node.getpeerinfo()))
71
72+ def run_test(self):
73+ self.test_inv_ibd()
p2p_tx_download.py where we have similar testing?tx_getdata_count is/can be greater than zero once out of IBD?A few PEP8 suggestions and (maybe, IIUC) a clarifying constant:
0--- a/test/functional/p2p_ibd_txrelay.py
1+++ b/test/functional/p2p_ibd_txrelay.py
2@@ -8,6 +8,7 @@
3 """
4 from decimal import Decimal
5+import time
6
7@@ -25,10 +26,10 @@ from test_framework.util import (
8-import time
9
10 MAX_FEE_FILTER = Decimal(9170997) / COIN
11 NORMAL_FEE_FILTER = Decimal(100) / COIN
12+WTXID = 0xffaa
13
14@@ -43,7 +44,7 @@ class P2PIBDTxRelayTest(BitcoinTestFramework):
15 peer = self.nodes[0].add_p2p_connection(P2PInterface())
16- peer.send_message(msg_inv([CInv(t=MSG_WTX, h=0xffaa)]))
17+ peer.send_message(msg_inv([CInv(t=MSG_WTX, h=WTXID)]))
18
19@@ -73,5 +74,6 @@ class P2PIBDTxRelayTest(BitcoinTestFramework):
20 self.test_feefilter_ibd()
21
22+
23 if __name__ == '__main__':
0--- a/test/functional/test_framework/p2p.py
1+++ b/test/functional/test_framework/p2p.py
2@@ -122,7 +122,7 @@ MAGIC_BYTES = {
3
4 # Constants from net_processing
5 NONPREF_PEER_TX_DELAY = 2 # seconds
6-TXID_RELAY_DELAY = 2 # seconds
7+TXID_RELAY_DELAY = 2 # seconds
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.
