This is basically a mini, IBD-only version of #21224
Incoming transactions aren’t really relevant until we’re caught up. That’s why we send a giant feefilter and don’t send tx getdatas, but we also shouldn’t process them if peers send them anyway. Simply ignore them.
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.
3205@@ -3206,6 +3206,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3206 return;
3207 }
3208
3209+ // Also stop processing the transaction early if we are still in IBD since we don't
3210+ // have enough information to validate it yet. Not a protocol violation.
3211+ if (::ChainstateActive().IsInitialBlockDownload()) return;
m_chainman.ActiveChainstate().IsInitialBlockDownload()
NetMsgType::INV processing.
Could log the rejection to the debug NET category?
we considered adding logging, but it seemed like it could be annoying if you’re connected to a client who keeps sending TXs? seems like if you get 1 you might get many?
I think you will already log that a message of type tx has been received, so adding another line wouldn’t make things worse than they are now?
My thinking is that this code path should rarely be hit, because outbound connections generally don’t announce txs via raw tx messages and inbound connections would only connect to you once your address is well propagated on the network, in which case you’ll have left IBD.
If a peer wants to annoy you, they can already send a message that will log at least one line in the NET debug category (before and after this pull).
setban the faultive peer because that’s a hint of a peer non-compliant with the inv-getdata sequence ?
grep I’d still prefer a specific log statement, but I guess I can add this myself.
3205@@ -3206,6 +3206,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3206 return;
3207 }
3208
3209+ // Also stop processing the transaction early if we are still in IBD since we don't
4@@ -5,9 +5,23 @@
5 """Test fee filters during and after IBD."""
46+ peer_inver = self.nodes[0].add_p2p_connection(P2PDataStore())
47+ txid = 0xdeadbeef
48+ peer_inver.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=txid)]))
49+ # assert peer_inver doesn't receive a getdata for the txid
50+ with p2p_lock:
51+ assert txid not in peer_inver.getdata_requests
AddTxAnnouncement()), so this assert would pass even if the node wasn’t in IBD.
30 from test_framework.test_framework import BitcoinTestFramework
31+from test_framework.util import hex_str_to_bytes
32
33 MAX_FEE_FILTER = Decimal(9170997) / COIN
34 NORMAL_FEE_FILTER = Decimal(100) / COIN
35+NONPREF_PEER_TX_DELAY = 2
NONPREF_PEER_TX_DELAY, it’s easier to have corresponding test constants in one place.
Thanks for tackling this, I exercised new test coverage, works as expected!
W.r.t to Marco point, I don’t think logging this minor rejection case is that important.
3205@@ -3206,6 +3206,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3206 return;
3207 }
3208
3209+ // Stop processing the transaction early if we are still in IBD since we don't
3210+ // have enough information to validate it yet. Not a protocol violation.
This comment is confusing to me – have we decided whether it’s protocol-violating to not process unrequested transactions? That was exactly what I proposed we do elsewhere, in general, but there was debate over whether such a change would be breaking or whether it was useful.
If it’s not useful for our software to do this in general, I don’t know why it’s useful to do in this smaller specific case? And on the flip side, if it’s not a protocol violation here, why exactly would it be a protocol violation to do this in general?
Note also that being in IBD happens not just when we’re syncing the whole blockchain, but whenever a node is started up again after being off for a few days. So I believe that if there are any software projects that send unrequested transactions at all, this would be a potentially observable change for them – as far as I can tell, even while in IBD we can at least sometimes validate and relay a transaction just fine, if a transaction were only spending utxos that are already confirmed… (Unless I’m missing some other interaction that would prevent that from working?)
Not to say that this change can’t be made, I’m just looking for reasoning and a comment that explains our actual thinking, because so far I think the reasoning has been contradictory. If it’s not a protocol violation to ignore unrequested transactions, I don’t see the harm in us always doing it and avoiding gating yet another behavior on whether we are in IBD (which has always been a source of headaches when developers try to understand why something isn’t working as expected). If it is a protocol violation or potentially breaking, we should note that here and have a better justification for doing it than has been provided so far (particularly if a transaction were coming from a PF_RELAY or PF_FORCERELAY peer, where you might expect us to try harder to pass the transaction on).
For some context, the comment is meant to convey that sending us unsolicited transactions is not a protocol violation, and I wanted to distinguish from the case above in which we disconnect the peer in addition to ignoring the transaction for violating no-tx-relay. Something like “since the peer has not violated protocol, don’t punish” is what I meant. The comment could definitely be clearer.
My goal is just to fail early when we don’t expect to be able to validate a transaction. It’s definitely a good point that we could be perfectly able to validate despite being in IBD. I wonder if we could improve the condition with a better heuristic than “if in IBD?”
Ah, yes I completely misparsed that protocol violation comment, thanks for explaining that.
I’m still looking to understand why this change is beneficial, but a broader change that includes IBD as a subset and also allows for PF_RELAY peers to bypass it, would not be.
I think we can consider this change as beneficial as it’s a small resources saving during IBD, a period during which our resources should be allocated on a more prioritized task. W.r.t to a broader change adopting this behavior beyond IBD (e.g #21224) we have an explicit concern about silently harming other bitcoin softwares deployed on the tx-relay network. As it’s a reasonable assumption to expect only a minority fraction of nodes doing IBD at anytime, this change might be considered as a far lesser disruption.
Though, we could also say that disruption would be limited for a broader change as we expect deployment cycles to be gradual.
I wonder if we could improve the condition with a better heuristic than “if in IBD?”
You can definitely validate while in IBD. But “already confirmed utxos” at your-IBD-tip doesn’t mean those have not been spent at network tip. If your last synced block is 100, your valid transaction might already have been double-spent at block 200, so validation cost would have been lost. IMO, utxo set view should be assumed to fluctuate quickly during IBD.
Also, you won’t request transactions so your mempool is likely to be empty and you won’t be able to accurately weight transaction feerate. Relying on those heuristics, better to save on resources until your node is more able to assert with certainty if the transaction is a good candidate for propagation and block inclusion.
Ah, yes I completely misparsed that protocol violation comment, thanks for explaining that.
I just had the same confusion.
Issues like those being explored in #21106 — where IBD may kick in at unintuitive times, complicating our understanding of the network due to special-case interactions throughout our codebase — are why I think we should hesitate before adding more checks gated on IBD and ensure we have a good reason for doing so.
In this case, I think our code is simpler to understand if we either always or never processed unrequested transactions (and the benefit to skipping this while in IBD seems small in comparison).
tACK d6314481efa4ff054dc036e980b4923ee5d1d736
Ubuntu 20.04
Just pushed some changes addressing @ariard’s review comment and clarifying the comment about “not a protocol violation.”
On why add a gate for unsolicited transactions in IBD but not the general case:
There can perhaps be an observable difference if we stop processing unsolicited transactions in the general case - someone mentioned in #21224 that there are clients that wouldn’t be able to broadcast their transactions because they don’t follow the inv/getdata dialogue. I’m not sure what the status is on that discussion so I won’t speak to it too much.
On the other hand, I don’t think there’s any downside to ignoring unsolicited transactions in IBD. We just have less wasted effort: Right now, if we can’t validate the transaction yet, we waste time putting it into orphan pool. If it conflicts with a block tx we see later on, we need to go through removing it from our mempool. If it’s valid and we can validate it, doing so while in IBD doesn’t benefit us - even if we come out of IBD really quickly (e.g. on a restart), we can download it again in a rebroadcast (or, later on, a reconciliation with Erlay). My personal opinion is that getting to exit early is worth the complexity of an extra IsIBD check, even if we later change it to ignore all unsolicited transactions.
47@@ -28,6 +48,32 @@ def run_test(self):
48 assert node.getblockchaininfo()['initialblockdownload']
49 self.wait_until(lambda: all(peer['minfeefilter'] == MAX_FEE_FILTER for peer in node.getpeerinfo()))
50
51+ self.log.info("Check that nodes don't send getdatas for transactions while still in IBD")
52+ peer_inver = self.nodes[0].add_p2p_connection(P2PDataStore())
53+ txid = 0xdeadbeef
54+ peer_inver.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=txid)]))
55+ # The node should not send a getdata, but if it did, it would first delay 2 seconds
56+ time.sleep(NONPREF_PEER_TX_DELAY)
66+ "04402203f16c6f40162ab686621ef3000b04e75418a0c0cb2d8aebeac894ae360ac1e780220ddc15ecdfc3507ac48e168" + \
67+ "1a33eb60996631bf6bf5bc0a0682c4db743ce7ca2b01ffffffff0140420f00000000001976a914660d4ef3a743e3e696a" + \
68+ "d990364e555c271ad504b88ac00000000"
69+ assert self.nodes[1].decoderawtransaction(rawhex) # returns a dict, should not throw
70+ tx = CTransaction()
71+ tx.deserialize(BytesIO(hex_str_to_bytes(rawhex)))
0 tx = FromHex(CTransaction(), rawhex)
51+ peer_inver = self.nodes[0].add_p2p_connection(P2PDataStore())
52+ txid = 0xdeadbeef
53+ peer_inver.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=txid)]))
54+ # The node should not send a getdata, but if it did, it would first delay 2 seconds
55+ self.nodes[0].setmocktime(int(time.time() + NONPREF_PEER_TX_DELAY))
56+ peer_inver.sync_with_ping()
0 peer_inver.sync_send_with_ping()
To account for the possibility that pongs are sent before getdata?
send_and_ping()?
There is, but it doesn’t matter. The pong in this line (after mocktime) will be sent out here:
0 // Receive messages
1 bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode, flagInterruptMsgProc);
At this point no messages have been sent out (no getdata etc). Sending will happen right after, but may take a long time if the thread is put aside or the buffer is full.
15-from test_framework.messages import COIN
16+from test_framework.messages import (
17+ CInv,
18+ COIN,
19+ CTransaction,
20+ FromHex,
from_hex
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
