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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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;
Does the scope resolution operator need to be used here like everywhere else? if (::ChainstateActive().IsInitialBlockDownload()) return;
Good point! I'll update once PR got Concept ACK/NACK.
d65dca43 partial WIP suggestions, though I think it could still be more clearly written
- // To save bandwidth during IBD, stop orphan processing early. With the state of
- // our UTXO set far from tip, we might not have yet the unspent output spend by this
- // transaction or this transaction is already double-spend but not yet seen block.
- // Further, we can't assert accuracy of its feerate.
+ // If we are in IBD, exit from orphan processing early to save bandwidth. With the
+ // state of our UTXO set far from the tip, we might not yet have the unspent output
+ // of this transaction, or the transaction is already double-spent but not yet
+ // included in a block. We also cannot assert the accuracy of its feerate.
//
- // Orphan transaction is discarded. Note, we don't both to add id to recentRejects
- // as it's expected to be quickly clean up during IBD.
- if (ChainstateActive().IsInitialBlockDownload()) return;
+ // Orphan transaction is discarded. We don't bother to add it to recentRejects, as
+ // it's expected to be quickly cleaned up during IBD.
+ if (::ChainstateActive().IsInitialBlockDownload()) return;
"it's expected" is somewhat ambiguous, could you use an explicit subject?
Yes this is the recentRejects filter. I did modify the comment to underscore that during IBD blocks are received fast and thus the filter quickly cleanup in consequence.
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?
TXID_RELAY_DELAY = 2 # seconds
Switched to NONPREF_PEER_TX_RELAY.
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
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/
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 | +)
2ada3103 tabulation is off for all of the added imports, should be 4 spaces
Concept ACK. Thanks for adding the no-tx-request-in-IBD test. Verified the no-parent-orphan-in-IBD test fails without commit "p2p: Don't resolve orphans during IBD".
How much difference is this going to make in practice? For a node without incoming connections it seems unlikely to have peers that don't support the feefilter message. For a node with incoming connections, minor traffic waste doesn't seem to be a concern.
Concept ACK
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.
I tend to agree with @jnewbery that just ignoring all unrequested transactions during IBD would make more sense, but I'd go further and suggest we generally ignore all unrequested transactions (perhaps excepting peers with PF_RELAY permission), to mitigate potential CPU DoS.
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:
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c21c9e3ee..5def5178e 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2687,7 +2687,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
LogPrint(BCLog::NET, "transaction (%s) inv sent in violation of protocol, disconnecting peer=%d\n", inv.hash.ToString(), pfrom.GetId());
pfrom.fDisconnect = true;
return;
- } else if (!fAlreadyHave && !m_chainman.ActiveChainstate().IsInitialBlockDownload()) {
+ } else if (!fAlreadyHave) {
AddTxAnnouncement(pfrom, gtxid, current_time);
}
} else {
And test_unrequested_tx_ibd:
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index c21c9e3ee..32cdba5f2 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2928,8 +2928,8 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
// and mitigate potential DoS risks.
if (!pfrom.HasPermission(PF_RELAY) && ::ChainstateActive().IsInitialBlockDownload())
{
- LogPrint(BCLog::NET, "unrequested transaction while in IBD from peer=%d\n", pfrom.GetId());
- return;
+ //LogPrint(BCLog::NET, "unrequested transaction while in IBD from peer=%d\n", pfrom.GetId());
+ //return;
}
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())
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))
I don't think the tx needs to be valid (and it can't be valid because the utxo set is empty). Just using 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))
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.
I introduced a 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.
Thanks @MarcoFalke, updated at 75b45eb.
5 | @@ -6,8 +6,27 @@
6 |
The docstring in this file could be updated. It's no longer just testing fee filters.
Yes updated the docstring, lmk about the formatting by bullet point.
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.
This doesn't save bandwidth since the tx is already downloaded. I think "Do not process unrequested transactions during IBD" is sufficient as a comment.
There is a edge case where the transaction is an orphan and thus trigger further download but that's not worthy to document as bandwidth saving. Updated.
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']):
The unexpected_msgs here is very specific. I understand that you're trying to check that the tx wasn't rejected by the mempool, but if the log format ever changes, then the unexpected_msgs string isn't actually testing anything, and would just be dead code. I'd suggest removing it and relying on the expected_msgs string.
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
@MarcoFalke Ah should I rebase on master to clean out the time out about libfuzzer+valgrind or it's cirrus-ci's VM runtime ?
Rebased at d893b47.
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.
Needs rebase!
@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 don't think light clients can set the flag on the server they connect to. So this will break any such clients that opted to send txs directly. I know that there is no BIP (for obvious reasons) that explains how transaction relay should work, but I'd be cautious breaking a relay mechanism that some software relies on because it has been working in the past. At the very least this breaking change should be communicated to the mailing list.
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 ?
Thanks for taking the suggestion -- I think it would make sense to mention on the mailing list now that this is a proposed behavior change for Bitcoin Core, so that if others have feedback on whether this is breaking they can speak up.
See mailing list thread, moving this PR as draft until I gather feedback on this proposal, or at least no one speaks up.
As a minor procedural point, I think it would have been better to open a new PR for the current branch. This PR has now been through three significantly different iterations, and it's impossible to tell which versions the review comments/ACKs are referring to.
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)
Could speed this up? I think txrequest works with mocktime, too
Yes I can confirm txrequests works with mocktime. Updated with your suggestion.
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?
--- a/test/functional/p2p_tx_download.py
+++ b/test/functional/p2p_tx_download.py
@@ -117,9 +117,6 @@ class TxDownloadTest(BitcoinTestFramework):
p = self.nodes[0].p2ps[0]
- with p2p_lock:
- p.tx_getdata_count = 0
-
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:
--- a/test/functional/p2p_ibd_txrelay.py
+++ b/test/functional/p2p_ibd_txrelay.py
@@ -8,6 +8,7 @@
"""
from decimal import Decimal
+import time
@@ -25,10 +26,10 @@ from test_framework.util import (
-import time
MAX_FEE_FILTER = Decimal(9170997) / COIN
NORMAL_FEE_FILTER = Decimal(100) / COIN
+WTXID = 0xffaa
@@ -43,7 +44,7 @@ class P2PIBDTxRelayTest(BitcoinTestFramework):
peer = self.nodes[0].add_p2p_connection(P2PInterface())
- peer.send_message(msg_inv([CInv(t=MSG_WTX, h=0xffaa)]))
+ peer.send_message(msg_inv([CInv(t=MSG_WTX, h=WTXID)]))
@@ -73,5 +74,6 @@ class P2PIBDTxRelayTest(BitcoinTestFramework):
self.test_feefilter_ibd()
+
if __name__ == '__main__':
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -122,7 +122,7 @@ MAGIC_BYTES = {
# Constants from net_processing
NONPREF_PEER_TX_DELAY = 2 # seconds
-TXID_RELAY_DELAY = 2 # seconds
+TXID_RELAY_DELAY = 2 # seconds
ACK ec04fa50c703e726f2315195892d22c4486bf31a with some questions and suggestions
<!--cf906140f33d8803c4a75a2196329ecb-->
🐙 This pull request conflicts with the target branch and needs rebase.
<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?
@jnewbery @mzumsande @naumenkogs you might be interested in re-opening / following up with this change?