Only allow getdata of recently announced invs #19109
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Concept ACK. What's the additional memory overhead per peer? |
|
EDIT: Around 37 KiB now |
|
If I've got the maths right, in a 15 minute interval we'll announce up to 15,750 txs to an outbound peer (35 every 2 seconds), and 6,300 txs to inbound peers (35 every 5 seconds). If RBF and the like didn't exist, 10k txs would be about 4 blocks worth on average based on some recent blocks. Relaying tx's from the mempool in addition to mapRelay was introduced in #16851 and helps obtain missing unconfirmed parents when a child transaction enters the mempool (ie, poor man's package relay via the orphan pool). Without some alternative working form of package relay, I'm not sure it's ideal to drop that? Presuming we want to support mempool filtering at all, I think it's useful to be able to obtain old transactions -- if someone tries to send you a payment but uses a low feerate that isn't confirming, you want to be able to pick up that transaction from your peers' mempools so that you can CPFP it, even if you only manage to get online a few hours or a day after it was relayed across the network. One way to preserve that behaviour might be to retain the auto txinfo = mempool.info(txid);
if (txinfo.tx && txinfo.m_time <= longlived_mempool_time) {
return txinfo.tx;
} else {
LOCK(cs_main);
if (state->m_recently_relayed_invs.contains(txid)) {
if (txinfo.tx) return txinfo.tx;
// In order to reduce NOTFOUND spam, we retain recently announced txs
// in mapRelay even if they've been removed from the mempool
auto mi = mapRelay.find(txid);
if (mi != mapRelay.end()) return mi->second;
}
}That way if you'll still relay "old" txs (and even do it without locking If Could be an idea to set nElements to |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
@ajtowns Good points. We should be moving towards relying more on the mempool where it can provide that data. Your code looks like the right approach. So I guess a question is parameters. With Poisson distributed intervals, 70 s is enough for a 1 in a million chance to not have had any broadcast events. 104 s is enough for 1 in a billion. Unless there is another reason to extend, 2 minutes should be plenty (both for modelling the longlived_mempool variable, the bloom filter size, and for limiting the relay pool). If we're talking about such short windows, perhaps memory usage is really no concern. The bloom filter could also be given a 1/billion fprate, and still be just a few kB. |
|
Perhaps the first question is: after how much time are we okay with attackers knowing everything that was in the mempool at the time? If that number is small, perhaps it is sufficient to just allow responding to all tx requests, as long as the tx entered the mempool before the last inv sent out to that peer (a variable which could replace the last BIP35 response to that peer one). That would avoid the need for a Bloom filter entirely. |
|
Updated to @ajtowns' approach suggested above, with a delay constant of 2 minutes. |
|
Pinging a few people who may be interested in reviewing: @naumenkogs @amiti @jonatack @MarcoFalke |
|
I'm running with this code, plus this logging patch: @@ -1540,6 +1540,10 @@ CTransactionRef static FindTxForGetData(CNode* peer, const uint256& txid, const
}
}
+ if (txinfo.tx) {
+ LogPrint(BCLog::NET, "peer requested premature tx %s peer=%d\n", txid.ToString(), peer->GetId());
+ }
+
return {};
}And it's occasionally logging premature entries (a few over 24 hours), which I wasn't seeing with the code currently in master. |
I got some examples of that too; but the peer that was doing it looked very spy-ish. However, I think a legit client could trigger it, if the sequence was "here's tx X; peer connects; here's tx Y which spends X:n; peers asks for Y; peer doesn't have X so asks for X", all within the 2 minute timeframe. |
|
@ajtowns Almost all cases I'm seeing are within 90s of the connection being opened, which is evidence for the dependent transactions theory. I guess a potential solution is inserting the in-mempool parents of relayed transactions to the Bloom filter (as in a sense, they've been "inved"). |
sipa
force-pushed
the
202005_bloom_relay
branch
2 times, most recently
from
151ef3d
to
3883453
Compare
|
I added a commit to add unconfirmed parents of relayed transactions into the recently-relayed Bloom filter. I'm not seeing any premature requests anymore. |
|
For reviewers who might be confused by the logging patch above, where CNode is a pointer rather than a reference (ISTM this was recently changed to a reference) and the code is located ~120 lines higher up than now, I think the logging patch is the following -- correct me if wrong. @@ -1658,6 +1658,10 @@ CTransactionRef static FindTxForGetData(CNode& peer, const uint256& txid, const
}
}
+ if (txinfo.tx) {
+ LogPrint(BCLog::NET, "peer requested premature tx %s peer=%d\n", txid.ToString(), peer.GetId());
+ }
+
return {};
} |
|
Running a build at 19825f9 it took a half day to see any premature requests. So far 4 peers did so. total premature tx requests (from 4 peers)
peer 1693 summary, 10 premature requests after 60s for another 33s, connection time 21m
peer 2357 all activity, 2 premature requests after 25s, connection time 1m20s
peer 5334, 4 premature requests after 13s for another 6s, connection time 48m
EDIT: added a 4th peer (5336) seen after 36 hours. Now running the same logging patch on the last commit 3883453 |
naumenkogs
reviewed
Jun 10, 2020
|
Concept ACK.
|
|
|
After running 36 hours with this commit, I'm not seeing them anymore either, as opposed to at the previous commit as described above #19109 (comment). |
jnewbery
reviewed
Jun 15, 2020
laanwj
added a commit
that referenced
this pull request
Aug 10, 2020
…and missing parents of orphan transactions 4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar) Pull request description: I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter. This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan. This addresses a couple of [related](#19109 (comment)) [comments](#19109 (comment)) left in #19109. ACKs for top commit: laanwj: Code review ACK 4c0731f jonatack: ACK 4c0731f ajtowns: ACK 4c0731f Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 11, 2020
…ctions and missing parents of orphan transactions 4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar) Pull request description: I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter. This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan. This addresses a couple of [related](bitcoin#19109 (comment)) [comments](bitcoin#19109 (comment)) left in bitcoin#19109. ACKs for top commit: laanwj: Code review ACK 4c0731f jonatack: ACK 4c0731f ajtowns: ACK 4c0731f Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 21, 2021
Summary: This is in preparation to using the mempool entering time as part of the decision for relay, but does not change behavior on itself. This is a partial backport of [[bitcoin/bitcoin#19109 | core#19109]] [1/5] bitcoin/bitcoin@a9bc563 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9434
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 21, 2021
…hing Summary: This constant is set to 2 minutes, rather than 15. This is still many times larger than the transaction broadcast interval (2s for outbound, 5s for inbound), so it should be acceptable for peers to know what our contents of the mempool was that long ago. This is a partial backport of [[bitcoin/bitcoin#19109 | core#19109]] [2/5] bitcoin/bitcoin@b24a17f Depends on D9434 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9435
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 21, 2021
Summary: ... unless they're UNCONDITIONAL_RELAY_DELAY old, or there has been a response to a MEMPOOL request in the mean time. This is accomplished using a rolling Bloom filter for the last 3500 announced transactions. The probability of seeing more than 100 broadcast events (which can be up to 35 txids each) in 2 minutes for an outbound peer (where the average frequency is one per minute), is less than 1 in a million. This is a partial backport of [[bitcoin/bitcoin#19109 | core#19109]] [3/5] bitcoin/bitcoin@43f02cc Depends on D9435 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9436
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 21, 2021
Summary: This is a partial backport of [[bitcoin/bitcoin#19109 | core#19109]] [4/5] bitcoin/bitcoin@c4626bc Depends on D9436 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9437
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Apr 21, 2021
Summary: This concludes backport of [[bitcoin/bitcoin#19109 | core#19109]] [5/5] bitcoin/bitcoin@f32c408 Depends on D9437 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9438
glozow
reviewed
May 2, 2023
Comment on lines
+1639
to
+1644
| // If a TX could have been INVed in reply to a MEMPOOL request, | ||
| // or is older than UNCONDITIONAL_RELAY_DELAY, permit the request | ||
| // unconditionally. | ||
| if ((mempool_req.count() && txinfo.m_time <= mempool_req) || txinfo.m_time <= now - UNCONDITIONAL_RELAY_DELAY) { | ||
| return std::move(txinfo.tx); | ||
| } |
There was a problem hiding this comment.
Hope you don't mind me digging up an old PR - why do responses to mempool requests bypass the recently announced filter? And could it make sense to remove this special case? I get the idea is to give the peer access to full mempool contents, but it still seems better to only serve the stuff we announced. Concerned about -peerbloomfilters=1 nodes getting fingerprinted through sending mempool + getdata for arbitrary transactions.
There was a problem hiding this comment.
I believe that was just done not to break existing use cases of BIP35. The thinking was perhaps that since it requires special setting/permission anyway for that peer, it can bypass the fingerprinting protections.
There was a problem hiding this comment.
Just repeating a comment made on IRC here for posterity.
It seems to be the case that no special setting/permission is required to solicity a response to a mempool msg query, only that the local node has set NODE_BLOOM (or we have whitelisted it with the mempool netpermission): https://github.com/bitcoin/bitcoin/blob/d89aca1bdbe52406f000e3fa8dda12c46dca9bdd/src/net_processing.cpp#LL4603C52-L4603
This was referenced May 16, 2023
fanquake
added a commit
that referenced
this pull request
Aug 17, 2023
fb02ba3 mempool_entry: improve struct packing (Anthony Towns) 1a11806 net_processing: Clean up INVENTORY_BROADCAST_MAX constants (Anthony Towns) 6fa4993 test: Check tx from disconnected block is immediately requestable (glozow) e4ffabb net_processing: don't add txids to m_tx_inventory_known_filter (Anthony Towns) 6ec1809 net_processing: drop m_recently_announced_invs bloom filter (Anthony Towns) a70beaf validation: when adding txs due to a block reorg, allow immediate relay (Anthony Towns) 1e9684f mempool_entry: add mempool entry sequence number (Anthony Towns) Pull request description: This PR replaces the `m_recently_announced_invs` bloom filter with a simple sequence number tracking the mempool state when we last considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally). The philosophy here (compare with #18861 and #19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protecting privacy, and therefore after you receive an INV message, it's immediately fair game to request any transaction that was in the mempool at the time the INV message was sent. We likewise consider the BIP 133 feefilter and BIP 37 bloom filters to be bandwidth optimisations here, and treat transactions as requestable if they would have been announced without those filters. Given that philosophy, tracking the timestamp of the last INV message and comparing that against the mempool entry time allows removal of each of `m_recently_announced_invs`, `m_last_mempool_req` and `UNCONDITIONAL_RELAY_DELAY` and associated logic. ACKs for top commit: naumenkogs: ACK fb02ba3 amitiuttarwar: review ACK fb02ba3 glozow: reACK fb02ba3 Tree-SHA512: cbba5ee04c86df26b6057f3654c00a2b45ec94d354f4f157a769cecdaa0b509edaac02b3128afba39b023e82473fc5e28c915a787f84457ffe66638c6ac9c2d4
knst
pushed a commit
to knst/dash
that referenced
this pull request
Jan 24, 2024
f32c408 Make sure unconfirmed parents are requestable (Pieter Wuille) c4626bc Drop setInventoryTxToSend based filtering (Pieter Wuille) 43f02cc Only respond to requests for recently announced transactions (Pieter Wuille) b24a17f Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille) a9bc563 Swap relay pool and mempool lookup (Pieter Wuille) Pull request description: This implements the follow-up suggested here: bitcoin#18861 (comment) . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15. This: * Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see bitcoin#18861 (comment)). * Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see bitcoin#18861 (comment)). It adds 37 KiB of filter per peer. This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see bitcoin#17303), but that is still blocked by dealing properly with NOTFOUNDs (see bitcoin#18238). ACKs for top commit: jnewbery: reACK f32c408 jonatack: re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review. ajtowns: re-ACK f32c408 Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
knst
pushed a commit
to knst/dash
that referenced
this pull request
Jan 24, 2024
f32c408 Make sure unconfirmed parents are requestable (Pieter Wuille) c4626bc Drop setInventoryTxToSend based filtering (Pieter Wuille) 43f02cc Only respond to requests for recently announced transactions (Pieter Wuille) b24a17f Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille) a9bc563 Swap relay pool and mempool lookup (Pieter Wuille) Pull request description: This implements the follow-up suggested here: bitcoin#18861 (comment) . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15. This: * Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see bitcoin#18861 (comment)). * Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see bitcoin#18861 (comment)). It adds 37 KiB of filter per peer. This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see bitcoin#17303), but that is still blocked by dealing properly with NOTFOUNDs (see bitcoin#18238). ACKs for top commit: jnewbery: reACK f32c408 jonatack: re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review. ajtowns: re-ACK f32c408 Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
5 tasks
knst
pushed a commit
to knst/dash
that referenced
this pull request
Jan 24, 2024
f32c408 Make sure unconfirmed parents are requestable (Pieter Wuille) c4626bc Drop setInventoryTxToSend based filtering (Pieter Wuille) 43f02cc Only respond to requests for recently announced transactions (Pieter Wuille) b24a17f Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille) a9bc563 Swap relay pool and mempool lookup (Pieter Wuille) Pull request description: This implements the follow-up suggested here: bitcoin#18861 (comment) . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15. This: * Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see bitcoin#18861 (comment)). * Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see bitcoin#18861 (comment)). It adds 37 KiB of filter per peer. This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see bitcoin#17303), but that is still blocked by dealing properly with NOTFOUNDs (see bitcoin#18238). ACKs for top commit: jnewbery: reACK f32c408 jonatack: re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review. ajtowns: re-ACK f32c408 Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
knst
pushed a commit
to knst/dash
that referenced
this pull request
Jan 25, 2024
f32c408 Make sure unconfirmed parents are requestable (Pieter Wuille) c4626bc Drop setInventoryTxToSend based filtering (Pieter Wuille) 43f02cc Only respond to requests for recently announced transactions (Pieter Wuille) b24a17f Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille) a9bc563 Swap relay pool and mempool lookup (Pieter Wuille) Pull request description: This implements the follow-up suggested here: bitcoin#18861 (comment) . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15. This: * Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see bitcoin#18861 (comment)). * Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see bitcoin#18861 (comment)). It adds 37 KiB of filter per peer. This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see bitcoin#17303), but that is still blocked by dealing properly with NOTFOUNDs (see bitcoin#18238). ACKs for top commit: jnewbery: reACK f32c408 jonatack: re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review. ajtowns: re-ACK f32c408 Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
PastaPastaPasta
pushed a commit
to knst/dash
that referenced
this pull request
Jan 28, 2024
f32c408 Make sure unconfirmed parents are requestable (Pieter Wuille) c4626bc Drop setInventoryTxToSend based filtering (Pieter Wuille) 43f02cc Only respond to requests for recently announced transactions (Pieter Wuille) b24a17f Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille) a9bc563 Swap relay pool and mempool lookup (Pieter Wuille) Pull request description: This implements the follow-up suggested here: bitcoin#18861 (comment) . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15. This: * Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see bitcoin#18861 (comment)). * Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see bitcoin#18861 (comment)). It adds 37 KiB of filter per peer. This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see bitcoin#17303), but that is still blocked by dealing properly with NOTFOUNDs (see bitcoin#18238). ACKs for top commit: jnewbery: reACK f32c408 jonatack: re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review. ajtowns: re-ACK f32c408 Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
Jan 28, 2024
knst
pushed a commit
to knst/dash
that referenced
this pull request
Feb 24, 2024
…ctions and missing parents of orphan transactions 4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar) Pull request description: I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter. This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan. This addresses a couple of [related](bitcoin#19109 (comment)) [comments](bitcoin#19109 (comment)) left in bitcoin#19109. ACKs for top commit: laanwj: Code review ACK 4c0731f jonatack: ACK 4c0731f ajtowns: ACK 4c0731f Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
knst
pushed a commit
to knst/dash
that referenced
this pull request
Feb 26, 2024
…ctions and missing parents of orphan transactions 4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar) Pull request description: I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter. This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan. This addresses a couple of [related](bitcoin#19109 (comment)) [comments](bitcoin#19109 (comment)) left in bitcoin#19109. ACKs for top commit: laanwj: Code review ACK 4c0731f jonatack: ACK 4c0731f ajtowns: ACK 4c0731f Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
knst
pushed a commit
to knst/dash
that referenced
this pull request
Mar 3, 2024
…ctions and missing parents of orphan transactions 4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar) Pull request description: I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter. This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan. This addresses a couple of [related](bitcoin#19109 (comment)) [comments](bitcoin#19109 (comment)) left in bitcoin#19109. ACKs for top commit: laanwj: Code review ACK 4c0731f jonatack: ACK 4c0731f ajtowns: ACK 4c0731f Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
knst
pushed a commit
to knst/dash
that referenced
this pull request
Mar 4, 2024
…ctions and missing parents of orphan transactions 4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar) Pull request description: I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter. This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan. This addresses a couple of [related](bitcoin#19109 (comment)) [comments](bitcoin#19109 (comment)) left in bitcoin#19109. ACKs for top commit: laanwj: Code review ACK 4c0731f jonatack: ACK 4c0731f ajtowns: ACK 4c0731f Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
knst
pushed a commit
to knst/dash
that referenced
this pull request
Mar 4, 2024
…ctions and missing parents of orphan transactions 4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar) Pull request description: I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter. This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan. This addresses a couple of [related](bitcoin#19109 (comment)) [comments](bitcoin#19109 (comment)) left in bitcoin#19109. ACKs for top commit: laanwj: Code review ACK 4c0731f jonatack: ACK 4c0731f ajtowns: ACK 4c0731f Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
knst
pushed a commit
to knst/dash
that referenced
this pull request
Mar 5, 2024
…ctions and missing parents of orphan transactions 4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar) Pull request description: I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter. This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan. This addresses a couple of [related](bitcoin#19109 (comment)) [comments](bitcoin#19109 (comment)) left in bitcoin#19109. ACKs for top commit: laanwj: Code review ACK 4c0731f jonatack: ACK 4c0731f ajtowns: ACK 4c0731f Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
knst
pushed a commit
to knst/dash
that referenced
this pull request
Mar 5, 2024
…ctions and missing parents of orphan transactions 4c0731f Deduplicate missing parents of orphan transactions (Suhas Daftuar) 8196176 Rewrite parent txid loop of requested transactions (Suhas Daftuar) Pull request description: I noticed a couple of places recently where we loop over all inputs of a transaction in order to do some processing on the txids we find in those inputs. There may be thousands of inputs in a transaction, and the same txid may appear many times. In a couple of places in particular, we loop over those txids and add them to a rolling bloom filter; doing that multiple times for the same txid wastes entries in that filter. This PR fixes that in two places relating to transaction relay: one on the server side, where we look for parent transactions of a tx that we are delivering to a peer to ensure that getdata requests for those parents will succeed; and the other on the client side, where when we process an orphan tx we want to loop over the parent txids and ensure that all are eventually requested from the peer who provided the orphan. This addresses a couple of [related](bitcoin#19109 (comment)) [comments](bitcoin#19109 (comment)) left in bitcoin#19109. ACKs for top commit: laanwj: Code review ACK 4c0731f jonatack: ACK 4c0731f ajtowns: ACK 4c0731f Tree-SHA512: 8af9df7f56c6e54b5915519d7d5465e081473ceb1bcc89bbebf83e78722cf51ff58145e588cf57126bce17071a8053273f4bcef0ad8166bec83ba14352e40f5d
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements the follow-up suggested here: #18861 (comment) . Instead of checking
setInventoryTxToSend, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15.This:
filterInventoryKnownrolls over can still be requested (pointed out by luke-jr, see Do not answer GETDATA for to-be-announced tx #18861 (comment)).It adds 37 KiB of filter per peer.
This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see #17303), but that is still blocked by dealing properly with NOTFOUNDs (see #18238).