glozow
commented at 3:38 pm on May 24, 2023:
member
WORK IN PROGRESS. Please do not run it for any use other than testing.
This PR is not meant for merge. This branch exists to help reviewers see what package relay would look like all together. I will open separate PRs for these components and expect to add more tests, docs, and polish along the way. This PR contains all of the functionality built in a linear manner. However, since there are pieces in various areas of the codebase and they can make progress in parallel, commits don’t necessarily need to be merged in this order.
See #27463 for what PR(s) are open for review right now.
Note to Reviewers
The major purpose of this PR is to solicit “approach” review.
This is a large project, and the first few p2p commits essentially define the interface. I’d like to get rough consensus on approach before we start looking at code details and merging PRs, because I believe it will help us “get useful stuff in” faster and avoid premature optimizations.
Here are some questions I hope are answered before we try to merge anything (originally #27742 (comment)):
Does the proposed protocol look sound?
Are these 3 milestones appropriate?
Is there important functionality that is in the “todo improvements” section but should be included in one of the 3 milestones? Alternatively, is there not-that-important stuff in the milestones that we should defer until later?
Does it make sense to have this PeerManager <-> TxDownloadMan interface?
Comments about naming, typos, code details, etc. are also appreciated but please note I may wait until their respective PRs are open to incorporate your comments. Thank you for your patience.
This project is split into 3 milestones, each of which gives us something useful.
Modularize and improve orphan-handling (also some refactoring).
Introduce a TxDownloadManager class, responsible for downloading transactions that have been announced to us.
Use all announcers as potential candidates for resolving an orphan. Add a TxRequestTracker Orphan Request Tracker which helps track orphans we need to resolve. Preferentially request orphan resolution from outbounds, preferred relay, etc., over inbounds.
When package relay peers are available, use ancpkginfo instead of missing parents when handling orphans.
Add sendpackages negotiation logic.
Add a separate rejections filter for transactions that are eligible for reconsideration when validated together as a package, so that children of low-feerate transactions are still considered.
Send getdata(MSG_ANCPKGINFO) to package relay peers for orphan resolution. Use ancpkginfo to request missing ancestors through normal individual transaction relay.
Download and validate ancestor packages using getpkgtxns and pkgtxns.
Add support for getpkgtxns and pkgtxns. Send a pkgtxns using the list of missing transactions from ancpkginfo.
Ensure we can process “normal” orphans even if a peer is trying to overwhelm/churn our orphanage. Do this by “opportunistically” protecting orphans from random eviction if they were sent by package relay peers, and redownloading orphans if we cannot afford to protect them. Each peer is allocated a certain amount of orphans they can protect at a time (“token bucket” style but the number of tokens is static for now). Outbound peers get more than inbounds.
If a transaction’s parent(s) are below the fee filter, don’t announce it (save the bandwidth of legacy nodes).
Todo improvements
These could be added to the milestones or deferred until basic functionality is deployed.
Store orphans serialized instead of as CTransactionRefs to significantly reduce their memory usage.
Perhaps try to keep orphans in disk and/or process them asynchronously, given the incredibly DoSable nature of orphan handling.
Dynamically allocate tokens for orphan protection. For example, if a long-standing inbound peer continuously provides good packages for orphans, they should have more tokens. If a peer is obviously serving us garbage, reduce their tokens.
Detect when we have received all the transactions for a package, regardless of how (individual or block or other), and return PackageToValidate in GetTxToReconsider.
New format for mempool.dat, packages instead of transactions.
DrahtBot
commented at 3:38 pm on May 24, 2023:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
A summary of reviews will appear here.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#28107 (rfc: Type-safe transaction identifiers by dergoegge)
#28066 (fuzz: Generate process_message targets individually by MarcoFalke)
#28043 (fuzz: Test headers pre-sync through p2p interface by dergoegge)
#28031 (Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module by glozow)
#27837 (net: introduce block tracker to retry to download blocks after failure by furszy)
#27675 (p2p: Drop m_recently_announced_invs bloom filter by ajtowns)
#27509 (Relay own transactions only via short-lived Tor or I2P connections by vasild)
#27499 (net processing, refactor: Decouple PeerManager from gArgs by dergoegge)
#27452 (test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py by pinheadmz)
#27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)
#26711 (validate package transactions with their in-package ancestor sets by glozow)
#26697 (logging: use bitset for categories by LarryRuane)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#26451 (Enforce incentive compatibility for all RBF replacements by sdaftuar)
#25572 (refactor: Introduce EvictionManager and use it for the inbound eviction logic by dergoegge)
#25268 (refactor: Introduce EvictionManager by dergoegge)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)
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.
glozow added the label
P2P
on May 24, 2023
DrahtBot added the label
CI failed
on May 25, 2023
glozow force-pushed
on May 30, 2023
in
src/net_processing.cpp:2935
in
22e93f08a5outdated
in
src/node/txpackagetracker.cpp:16
in
4c72113677outdated
10+#include <txrequest.h>
11+#include <util/hasher.h>
1213 namespace node {
14+ /** How long to wait before requesting orphan ancpkginfo/parents from an additional peer.
15+ * Same as GETDATA_TX_INTERVAL. */
in
src/node/txpackagetracker.cpp:163
in
4c72113677outdated
24 TxOrphanage m_orphanage;
25+
26+ mutable Mutex m_mutex;
27+
28+ /** Tracks orphans for which we need to request ancestor information. All hashes stored are
29+ * wtxids, i.e., the wtxid of the orphan. However, the is_wtxid field is used to indicate
As in Announcement::m_is_wtxid, sorry. Clarified (in other PR).
in
src/node/txpackagetracker.h:83
in
4c72113677outdated
58@@ -58,6 +59,36 @@ class TxPackageTracker {
59 /** Return how many entries exist in the orphange */
60 size_t OrphanageSize();
6162+ /** Received an announcement from this peer for a tx we already know is an orphan; should be
63+ * called for every peer that announces the tx, even if they are not a package relay peer.
64+ * The orphan request tracker will decide when to request what from which peer - use
65+ * GetOrphanRequests().
66+ */
67+ void AddOrphanTx(NodeId nodeid, const uint256& wtxid, const CTransactionRef& tx, bool is_preferred, std::chrono::microseconds reqtime);
in
src/node/txpackagetracker.cpp:77
in
4c72113677outdated
75+ AssertLockNotHeld(m_mutex);
76+ LOCK(m_mutex);
77+ // Skip if we weren't provided the tx and can't find the wtxid in the orphanage.
78+ if (tx == nullptr && !m_orphanage.HaveTx(GenTxid::Wtxid(wtxid))) return;
79+
80+ // Even though this stores the orphan wtxid, is_wtxid=false because we will be requesting the parents via txid.
Ah I mean of the GenTxid. Clarified comment (in other PR), thanks.
in
src/node/txpackagetracker.h:93
in
4c72113677outdated
72+ * validation queue. */
73+ size_t Count(NodeId nodeid) const;
74+
75+ /** Number of packages we are currently working on with this peer (i.e. reserving memory for
76+ * storing orphan(s)). Includes in-flight package info requests, tx data download, and packages
77+ * in the validation queue. Excludes entries in the orphan tracker that are just candidates. */
My understanding is that instead of adding all parents of an orphan to the main txrequest tracker (AddTxAnnouncement), we now just add the orphan to the orphanage tracker. We then need to request its parents somewhere to preserve behavior, which is done here.
But I also found this commit hard to understand, would be good to expand the commit msg.
since we’re touching this, would you mind annotating the bool as /* best */ to make it clearer what this is doing? New to tx tracking; would be helpful
Basically arbitrary. The relevance is basically which peer’s message processing is blocked to process this, and would get a misbehaving score if the tx is invalid. Anyone who announced it is equally deserving of this burden imo. I suppose a more fair way to pick is uniformly randomly.
in
src/txorphanage.h:27
in
fecb522a3foutdated
23@@ -24,7 +24,7 @@ static constexpr size_t MAX_ORPHAN_TOTAL_SIZE{100 * MAX_STANDARD_TX_WEIGHT};
24 */
25 class TxOrphanage {
26 public:
27- /** Add a new orphan transaction */
28+ /** Add a new orphan transaction. If the tx already exists, add this peer to its list of announcers. */
in
src/net_processing.cpp:1532
in
0671e1c178outdated
1458+{
1459+ const auto current_time{GetTime<std::chrono::microseconds>()};
1460+
1461+ // The originator will not show up in GetCandidatePeers() since we already requested from them.
1462+ AddOrphanAnnouncer(originator, orphan->GetWitnessHash(), orphan, current_time);
1463+ // We prefer to request the orphan's ancestors via package relay rather than txids
in
src/net_processing.cpp:1536
in
0671e1c178outdated
1462+ AddOrphanAnnouncer(originator, orphan->GetWitnessHash(), orphan, current_time);
1463+ // We prefer to request the orphan's ancestors via package relay rather than txids
1464+ // of missing inputs. Also, if the first request fails, we should try again.
1465+ // Get all peers that announced this transaction and prioritize accordingly...
1466+ for (const auto nodeid : m_txrequest.GetCandidatePeers(orphan->GetWitnessHash())) {
1467+ AddOrphanAnnouncer(nodeid, orphan->GetWitnessHash(), orphan, current_time);
Double checking that this is a no-op if the peer is already one the list of announcers? These seem to be called quite a bit as you get new announcements.
Yes. This is called in a big batch when we realize a tx is an orphan, and then for each inv of a tx that is present in our orphanage.
in
src/txorphanage.cpp:350
in
76c5647616outdated
338+ // Protected by more than one peer. Remove one of the protections.
339+ if (it->second->second.list_pos < -1) {
340+ it->second->second.list_pos += 1;
341+ return;
342+ }
343+ // Add to the end or m_orphan_list
Say in comment the obvious “protected by a single peer” or add an Assume to that effect. Took me too long to figure this out even though it’s obvious now.
DrahtBot removed the label
CI failed
on Jun 1, 2023
instagibbs
commented at 2:12 pm on June 1, 2023:
member
Approach review is very welcome on this PR
didn’t real whole OP until now. In my not expert opinion, the orphan handling seems reasonable, and I think it makes to open it ASAP to take it out of draft
glozow
commented at 2:51 pm on June 1, 2023:
member
Offline feedback suggested I clarify what I mean by “approach feedback welcome” before “I open separate PRs.”
This is a large project, and the first few p2p commits essentially define the interface. I’d like to get rough consensus on approach before we start looking at code details and merging PRs, because I believe it will help us “get useful stuff in” faster and avoid premature optimizations.
Here are some approach questions I think we should answer before diving in:
Does the proposed protocol look sound?
Are these 3 milestones appropriate?
Is there important functionality that is in the “todo improvements” section but should be included in one of the 3 milestones? Alternatively, is there not-that-important stuff in the milestones that we should defer until later?
Does it make sense to have this PeerManager <-> TxPackageTracker <-> TxOrphanage relationship?
instagibbs
commented at 5:50 pm on June 1, 2023:
member
redownloading orphans if we cannot afford to protect them
Is the idea here that protection is a bandiwdth optimization to avoid fetching an orphan twice in a row in the presence of malicious peers or random churn? Reasonable if so, just want this to be explicitly stated if so.
in
src/node/txpackagetracker.cpp:305
in
d76bb9dbb8outdated
298- m_orphanage.AddTx(m_orphanage.GetTx(wtxid), nodeid);
299- }
300-
301+ m_orphanage.AddTx(ptx, nodeid);
302+ // Protection may or may not be possible. If the orphan is unprotected, it's possible it
303+ // will be evicted by the time we try to download ancestors. If we cannot protect now but
in
src/node/txpackagetracker.cpp:27
in
d76bb9dbb8outdated
2324+ /** Maximum amount of orphans we may protect for an inbound peer when they first connect.
25+ * The number of "protection tokens" an inbound peer starts with. */
26+ static constexpr size_t MAX_ORPHANAGE_PROTECTION_INBOUND{50000};
27+ /** Maximum amount of orphans we may protect for an outbound peer when they first connect.
28+ * The number of "protection tokens" an inbound peer starts with. Equal to one maximum-size orphan. */
make it clear that all these units are in serialized transaction bytes, not WU or vsize.
in
src/node/txpackagetracker.cpp:284
in
d76bb9dbb8outdated
271272 // Skip if already requested in the (recent-ish) past.
273 if (packageinfo_requested.contains(GetPackageInfoRequestId(nodeid, wtxid, PKG_RELAY_ANCPKG))) return;
274275+ // Skip if this peer is already using a lot of space in the orphanage.
276+ if (m_orphanage.BytesFromPeer(nodeid) > MAX_ORPHANAGE_USAGE) return;
If we connect to enough peers and protect transactions beyond MAX_ORPHAN_TOTAL_SIZE, this will result in an assert with randrange arg of 0 once m_orphan_list is empty? Should either check m_orphan_list is non-empty, or only count non-protected bytes with m_total_unprotected_orphan_bytes?
Ah yes, that’s a bug! This while condition is very strange since one part applies to the unprotected orphanage and the other applies to the total orphanage. I think we should change the second condition to be on m_total_unprotected_orphan_bytes, yes.
55+ * The maximum does not apply to protected transactions, i.e., LimitOrphans(100) ensures
56+ * that the number of non-protected orphan entries does not exceed 100. Afterward, Size() may
57+ * return a number greater than 100. It is the caller's responsibility to ensure that not too
58+ * many orphans are protected.
59+ */
60 void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
glozow
commented at 10:38 am on June 2, 2023:
member
redownloading orphans if we cannot afford to protect them
Is the idea here that protection is a bandiwdth optimization to avoid fetching an orphan twice in a row in the presence of malicious peers or random churn? Reasonable if so, just want this to be explicitly stated if so.
Yes exactly. Worse is we want to avoid a situation where we requested the rest of the package while the orphan was in our orphanage, but then it fell out while we were waiting for the rest of the transactions. Then, we should re-download the orphan, but what do we do with the rest of them? We basically have to redownload the whole thing again, otherwise we might end up in an endless cycle of redownloading if we put them into an orphanage but don’t necessarily protect them from eviction.
105- // Unless we're deleting the last entry in m_orphan_list, move the last
106- // entry to the position we're deleting.
107- auto it_last = m_orphan_list.back();
108- m_orphan_list[old_pos] = it_last;
109- it_last->second.list_pos = old_pos;
110+ if (it->second.list_pos >= 0) {
Should this function ever be called with a protected orphan? Tests don’t cover these lines for protected peers(added an assert, never hit), and seems suspect that it would delete things below this block if it’s protected.
Should this function ever be called with a protected orphan?
Yes. It’s called when orphans expire as well.
Tests don’t cover these lines for protected peers
Sounds like I neglected to write a test for this. Expiry is pretty long, but theoretically we can hit this if we have multiple peers stalling us on the resolution of this orphan. We might happen to be trying package relay with one of the peers when the expiration is reached.
instagibbs
commented at 2:42 pm on June 2, 2023:
member
Along with the gist, focused on orphan protection and bucketing for concept understanding. Dumping comments related to those, and left a note on the gist.
instagibbs
commented at 2:42 pm on June 2, 2023:
member
Along with the gist, focused on orphan protection and bucketing for concept understanding. Dumping comments related to those, and left a note on the gist.
I think this introduces an interdependency between the number of outgoing transaction bytes we can receive from a peer and the maximum flow of LN commitment transaction+CPFP in weight a direct transaction-relay peer can announce to us and therefore we can validate. An option_anchors_zero_fee_htlc_tx LN commitment transaction weight is 900 WU with no pending HTLCs. As the orphan memory usage is implemented, it’s unclear if there is any processing guarantees for a peer announcing transaction to us, such processing guarantees are hard to enforce as we would be dependent on the full-node host performance.
Note current MAX_ORPHANAGE_USAGE is defined as MAX_ORPHANAGE_USAGE{10*MAX_ORPHANAGE_PROTECTION_OUTBOUND} though it sounds applied on processing inbound announcement (L276 in src/node/txpackagetracker.cpp).
Orphan usage would effect the CPFP transactions, IIUC, not the commitment transactions. The CPFP is deemed high enough, sent to the peer, the peer holds the child tx in orphan set and requests the full package, which isn’t size-bound(beyond current mempool limits).
Just to be sure we’re in sync on BytesFromPeer and MAX_ORPHANAGE_USAGE, it’s correct this a per-peer limit on the space in vbytes occupied by received orphans ?
If yes, I think this still introduce an interdependency between the number of outgoing transactions bytes and the maximum flow of LN commitment transactions and this limit should be documented (a la doc/policy/), or even better announced as part of sendpackages info.
in
src/node/txpackagetracker.h:79
in
1c13f6465coutdated
58@@ -58,6 +59,36 @@ class TxPackageTracker {
59 /** Return how many entries exist in the orphange */
60 size_t OrphanageSize();
6162+ /** Received an announcement from this peer for a tx we already know is an orphan; should be
63+ * called for every peer that announces the tx, even if they are not a package relay peer.
There is a argument that can be made if memory and throughputs limits (e.g MAX_PEER_ANNOUNCEMENTS) are applied in function of the type of transactions received (e.g v3 transactions) and that can be used to nudge multi-party and contracting protocols transactions broadcasters to use fee-bumping efficient mechanism such as one CPFP covering multiple parents due to the package limits. This would be a discount incentive by analogy with SegWit spending discounted by consensus validation rules.
On the other hand, such incentive might have “bad incentives” as such package topology might be unsafe (i.e if all the parents confirmation are time-sensitive) and this can become a further complication of our transaction-relay stack.
There is still a last open question if such lack of dissociation between type of traffic (e.g packages from simple transactions) could be abused when package validation is deployed, and CPU performance processing asymmetries (i.e AcceptSingleTransaction vs AcceptMultipleTransactions vs AcceptPackageWrappingSingle vs AcceptAncestorPackage) between our code paths could be exploited by a transaction-relay jamming adversary.
in
src/node/txpackagetracker.h:86
in
1c13f6465coutdated
65+ * GetOrphanRequests().
66+ */
67+ void AddOrphanTx(NodeId nodeid, const uint256& wtxid, const CTransactionRef& tx, bool is_preferred, std::chrono::microseconds reqtime);
68+
69+ /** Number of packages we are working on with this peer. Includes any entries in the orphan
70+ * tracker, in-flight orphan parent requests (1 per orphan regardless of how many missing
If I understand correctly the rate-limiting accounting introduced here, whatever the number of orphan parent missing, there will be an in-flight orphan parent requests issued (i.e if we’re missing 10 parents, we’re going to issue 10 in-flight orphan parent requests).
I think this should be documented as multi-party applications and contracting protocols transactions issuers might have to assemble their packages in function.
total number of inflights doesn’t appear to be changed here, it’s still MAX_PEER_TX_ANNOUNCEMENTS
If the protected buckets are taken by attackers, an honest party can still insert an orphan into the unprotected bucket(under the MAX_PEER_TX_ANNOUNCEMENTS limit), and the whole package including the child should get fetched after a short delay.
So from a time-sensitive contract perspective, things should be strictly better than today?
So from a time-sensitive contract perspective, things should be strictly better than today?
I think this is a different concern I’m aiming to underscore. It’s not related about “buckets”-capture by the attackers, rather than if you have a package of {A, B, C, D} with E as a CPFP and we have rate-limiting based on the way the parents are fetched (i.e individiually rather than in batch) the equivalent package of A+E will propagate better.
So if you would like A to confirm as soon as you can because it’s a 0-conf dual-funding, you wanna probably go for A+E.
We are not changing the limits, just accounting for orphan parent requests in the normal rate limits. One can argue there’s a new restriction, but I’d just say this is simply more accurate enforcement of existing limits. I wouldn’t be worried unless you’re currently broadcasting descendants before ancestors on purpose to trigger orphan handling and send an extra transaction a few seconds earlier.
Okay so I’ve been looking into the code, and yes to my understanding there is no processing penalty at GETPKGTXNS reception in ProcessMessage than our child has 1 or N parents, we’re calling FindTxForGetdata to compose the response PKGTXNS. This invalidate this specific source of concern.
we have rate-limiting based on the way the parents are fetched (i.e individiually rather than in batch) the equivalent package of A+E will propagate better.
I’ll maintain there is still an interpedency between package rate-limits and package compositions, though telling what to do a LN node is unclear and can be deferred to ulterior documentation.
in
src/node/txpackagetracker.h:107
in
1c13f6465coutdated
86+ * confirmed in a block, conflicted due to a block, or added to the mempool already.
87+ * Should be called on new block: valid=block transactions, invalid=conflicts.
88+ * Should be called when tx is added to mempool.
89+ * Should not be called when a tx fails validation.
90+ * */
91+ void FinalizeTransactions(const std::set<uint256>& valid, const std::set<uint256>& invalid);
This is unclear what level of enforcement is granted to a “final” decision, if the transactions are added to any transaction-relay download filters such as m_recent_rejects or m_recently_announced_invs. Otherwise, in case of blocks re-orgs dropping out the first package component and a second package component still stuck in filters, this can be source of package propagation failure.
Note that the rejection filters are cleared every time the chain tip changes, so I don’t think we can get failures this way
Yes, I think we should be careful to clean the whole package state in all filters at every block change to avoid “half” state interfering. m_recent_rejects_reconsiderable_were_rejected say “reset this filter when the chain tip changes”, of course it includes backward change like re-orgs.
I’ve deleted this function as it seems to be a confusing part of the interface. Replaced with separate functions that do similar things. (in other PR)
in
src/txorphanage.h:46
in
fecb522a3foutdated
42@@ -43,7 +43,8 @@ class TxOrphanage {
43 /** Erase an orphan by wtxid */
44 int EraseTx(const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
4546- /** Erase all orphans announced by a peer (eg, after that peer disconnects) */
47+ /** Maybe erase all orphans announced by a peer (eg, after that peer disconnects). If an orphan
I think there can be a source of concerns if all the announced orphans are second components from a single common parent. All our peers would announce those second-stage components, only one of them will succeed in the mempool validation and other will be refused as conflicts, I think.
Can you elaborate on this concern? The orphan that isn’t received yet may still be in orphanage, and will be connected to its parents normally
So if you have 3 peers and they announce A, B, C all with different wtxids while they’re spending the same parent txid, do we have a concern of the parent being fetched from multiple peers, or download should be dedup when we receive the ancpkginfos ?
download should be dedup when we receive the ancpkginfos ?
If it’s in your orphanage it won’t be fetched again, and will be marked as protected IIUC. See ReceivedAncPkgInfo
in
src/node/txpackagetracker.h:73
in
26e11e877eoutdated
65+
66+ // We expect this to be called only once
67+ void ReceivedVersion(NodeId nodeid);
68+ void ReceivedSendpackages(NodeId nodeid, PackageRelayVersions versions);
69+ // Finalize the registration state.
70+ bool ReceivedVerack(NodeId nodeid, bool inbound, bool txrelay, bool wtxidrelay);
I think it’s weird to set -blocksonly=1 -packagerelay=1, but also not the end of the world. In that case we’d want to not send “sendpackages.” Does that answer the question?
in
test/functional/p2p_package_relay.py:248
in
daa3802272outdated
80+ assert_equal(node.getpeerinfo()[0]["bytesrecv_per_msg"]["sendpackages"], 28)
81+ assert_equal(node.getpeerinfo()[0]["bytessent_per_msg"]["sendpackages"], 28)
82+ assert_equal(peer_no_wtxidrelay.sendpackages_received, [PKG_RELAY_ANCPKG])
83+ node.disconnect_p2ps()
84+
85+ self.log.info("Test sendpackages is sent even so")
a regression test with two elements checking it’s actually sorted lexigraphically would be good since this is a p2p message. I added a std::reverse to GetCombinedHash and passed tests.
in
src/net_processing.cpp:2162
in
985b36ab71outdated
Seems like this is wrong? for an inv of type MSG_ANCPKGINFO, we’ll be looking up the transaction as though it’s a txid, not wtxid. Perhaps this isn’t causing issues since m_recently_announced_invs contains both hashes even if m_mempool.info() would miss it?
in
src/node/txpackagetracker.cpp:280
in
f5f4b6cd0boutdated
164@@ -159,14 +165,26 @@ class TxPackageTracker::Impl {
165 // Skip if we weren't provided the tx and can't find the wtxid in the orphanage.
166 if (tx == nullptr && !m_orphanage.HaveTx(GenTxid::Wtxid(wtxid))) return;
167168- // Even though this stores the orphan wtxid, is_wtxid=false because we will be requesting the parents via txid.
169- orphan_request_tracker.ReceivedInv(nodeid, GenTxid::Txid(wtxid), is_preferred, reqtime);
170+ // Skip if already requested in the (recent-ish) past.
in
src/node/txpackagetracker.h:113
in
f5f4b6cd0boutdated
98@@ -99,6 +99,16 @@ class TxPackageTracker {
99 * Should not be called when a tx fails validation.
100 * */
101 void FinalizeTransactions(const std::set<uint256>& valid, const std::set<uint256>& invalid);
102+
103+ /** Whether a package info message is allowed (version-agnostic):
104+ * - We agreed to relay packages of this version with this peer.
105+ * - We solicited this package info somewhat recently.
106+ * Returns false if the message is not allowed and the peer should be disconnected. */
107+ bool PkgInfoAllowed(NodeId nodeid, const uint256& wtxid, PackageRelayVersions version);
for the two _later checks, would be nice to see GetOrphanRequests returns empty if reqtime wasn’t reached yet, even if they’re otherwise the best peer for it post-disconnect
in
src/net_processing.cpp:5317
in
39476ab805outdated
5045@@ -4992,6 +5046,13 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
5046 // If we receive a NOTFOUND message for a tx we requested, mark the announcement for it as
5047 // completed in TxRequestTracker.
5048 m_txrequest.ReceivedResponse(pfrom.GetId(), inv.hash);
5049+ } else if (inv.IsMsgAncPkgInfo() && m_enable_package_relay) {
5050+ if (!m_txpackagetracker->PkgInfoAllowed(pfrom.GetId(), inv.hash, node::PKG_RELAY_ANCPKG)) {
5051+ LogPrint(BCLog::NET, "\nUnsolicited ancpkginfo sent, disconnecting peer=%d\n", pfrom.GetId());
5052+ // Unsolicited ancpkginfo or peer is not registered for package relay.
Empty ancpkginfo seems invalid, peer should have sent a notfound?
“The wtxid of the requested transaction must be the last item in the list”
in
src/net_processing.cpp:4407
in
39476ab805outdated
4201+ }
4202+ LOCK(::cs_main);
4203+ if (m_chainman.ActiveChainstate().IsInitialBlockDownload()) return;
4204+ // We have already validated this exact set of transactions recently, so don't do it again.
4205+ if (m_recent_rejects_reconsiderable.contains(GetCombinedHash(package_wtxids))) {
4206+ LogPrint(BCLog::NET, "discarding package info for tx %s, this package has already been rejected\n",
in
src/net_processing.cpp:4413
in
cdb5b163d6outdated
4287+ rep_wtxid.ToString());
4288+ m_txpackagetracker->ForgetPkgInfo(pfrom.GetId(), package_wtxids.back(), node::PKG_RELAY_ANCPKG);
4289+ return;
4290+ }
4291+ for (const auto& wtxid : package_wtxids) {
4292+ // If a transaction is in m_recent_rejects and not m_recent_rejects_reconsiderable, that
0// If a transaction is in m_recent_rejects and package not m_recent_rejects_reconsiderable, that
in
src/net_processing.cpp:871
in
cdb5b163d6outdated
869+ *
870+ * Parameters are picked to be identical to that of m_recent_rejects, with the same rationale.
871+ * Memory used: 1.3 MB
872+ * FIXME: this filter can probably be smaller, but how much smaller?
873+ */
874+ CRollingBloomFilter m_recent_rejects_reconsiderable GUARDED_BY(::cs_main){120'000, 0.000'001};
in
src/node/txpackagetracker.cpp:169
in
cdb5b163d6outdated
164+ * whether we would request the ancestor information by wtxid (via package relay) or by txid
165+ * (via prevouts of the missing inputs). */
166+ TxRequestTracker orphan_request_tracker GUARDED_BY(m_mutex);
167+
168+ /** Cache of package info requests sent. Used to identify unsolicited package info messages. */
169+ CRollingBloomFilter packageinfo_requested GUARDED_BY(m_mutex){50000, 0.000001};
maybe too obvious to note, but bloom filters never have false-negatives, which means this wouldn’t cause additional disconnects if someone was flooding this shared resource. I had it backwards and thought it would allow a peer to influence who you disconnect from, vs tricking you into thinking you asked for the package info
instagibbs
commented at 4:15 pm on June 14, 2023:
member
another bucket of comments to respond to later
in
src/net_processing.cpp:4424
in
cdb5b163d6outdated
what’s the important of not checking orphanage here, then checking the orphanage inside ReceivedAncPkgInfo?
DrahtBot added the label
Needs rebase
on Jun 21, 2023
in
src/init.cpp:500
in
cdb5b163d6outdated
492@@ -492,6 +493,7 @@ void SetupServerArgs(ArgsManager& argsman)
493 argsman.AddArg("-i2psam=<ip:port>", "I2P SAM proxy to reach I2P peers and accept I2P connections (default: none)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
494 argsman.AddArg("-i2pacceptincoming", strprintf("Whether to accept inbound I2P connections (default: %i). Ignored if -i2psam is not set. Listening for inbound I2P connections is done through the SAM proxy, not by binding to a local address and port.", DEFAULT_I2P_ACCEPT_INCOMING), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
495 argsman.AddArg("-onlynet=<net>", "Make automatic outbound connections only to network <net> (" + Join(GetNetworkNames(), ", ") + "). Inbound and manual connections are not affected by this option. It can be specified multiple times to allow multiple networks.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
496+ argsman.AddArg("-packagerelay", strprintf("[EXPERIMENTAL] Support relaying transaction packages (default: %u)", node::DEFAULT_ENABLE_PACKAGE_RELAY), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
I think the comment can indicate the type of p2p packages (ancestor package relay v1 BIP 331), and indicate this is enabling package relay at the p2p level only to dissociate from mempool-level policy like nVersion=3 (e.g any descendant of a an unconfirmed V3 transaction must also be V3 or any descendant of an unconfirmed V3 transaction must also be V3).
In the future, we might release multiple versions of packages both at the p2p level and policy rules at the mempool-level (e.g nversion=4 to accomodate the flow of more L2s or annex DoS limits).
Yes adding a mention in BIP331 than a p2p packages can support multiple versions of policy regimes e.g nversion=3.
For flexibility towards the node operators in their resource management (especially for DoS protection on low-performance hosts), I think we should have a acceptnversion=3setting, that way a node operator can opted in package relay though not in the most computationally expensive type of policy regimes. The utility of such setting is theoretical as long as we have one policy regime for p2p packages, like right now.
in
src/node/txpackagetracker.cpp:140
in
26928b9c76outdated
133@@ -134,7 +134,11 @@ class TxPackageTracker::Impl {
134 for (const auto& txid : unique_parents) {
135 results.emplace_back(GenTxid::Txid(txid));
136 }
137- // Mark the orphan as requested
138+ // Mark the orphan as requested. Limitation: we aren't tracking these txids in
139+ // relation to the orphan's wtxid anywhere. If we get a NOTFOUND for the parent(s),
140+ // we won't automatically know that it corresponds to this orphan (i.e. won't be
141+ // able to call ReceivedResponse()). We will need to wait until it expires before
If the waiting time is dependent on timers (e.g TXID_RELAY_DELAY or NONPREF_PEER_TX_DELAY) or iterating over the inbound/outbound slots (i.e our CNode), I think we should document what is this waiting to be able to estimate if we modify max network throughput. Note there is an opened PR modifying tx relay rate, i.e #27630.
in
src/txorphanage.h:53
in
76c5647616outdated
49@@ -50,7 +50,12 @@ class TxOrphanage {
50 /** Erase all orphans included in or invalidated by a new block. Returns wtxids of erased txns. */
51 std::vector<uint256> EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
5253- /** Limit the orphanage to the given maximum */
54+ /** Limit the orphanage to the given maximum. Delete orphans whose expiry has been reached.
I think the expiry orphans could be announced at the protocol-level, as part of the “package-link”, i.e somewhere in sendpackages or in ancpkinfo. Those limits could be announced from the peer’s PeerManagerImpl with a push-interface (e.g the ZMQNotificationInterface ), that way a Lightning node could adapt its package broadcasting strategy accordingly.
in
src/txorphanage.h:76
in
76c5647616outdated
69@@ -65,6 +70,16 @@ class TxOrphanage {
70 LOCK(m_mutex);
71 return m_orphans.size();
72 }
73+ /** Protect an orphan from eviction from the orphanage getting full. The orphan may still be
74+ * removed due to expiry. If the orphan is already protected (by any peer), nothing happens.
75+ */
76+ void ProtectOrphan(const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
Should we implement blessing of all the orphans of a peer with “relay” permissions ? I can definitely see Lightning deployment where you have an inner Bitcoin Core node and multiple external ones, where you load-balance your flows to avoid transaction-relay fingerprinting based on obvious patterns like HTLC-preimage linked to off-chain channel XYZ.
in
src/txorphanage.h:18
in
d178c0fb37outdated
1314 #include <map>
15 #include <set>
1617+/** Maximum total size of orphan transactions stored, in bytes. */
18+static constexpr size_t MAX_ORPHAN_TOTAL_SIZE{100 * MAX_STANDARD_TX_WEIGHT};
commit d178c0fb372fc50857438b1b82d344fed0634a10, [txorphanage] impose a maximum total size of orphans]:
could this be maxorphantx * MAX_STANDARD_TX_WEIGHT, considering that the maximum number of orphans can be changed by the user? Seems a bit weird to just allow the user to change one of the two.
Hmm, I think the maximum possible value is currently 101 * MAX_STANDARD_TX_WEIGHT (after that, LimitOrphans() would kick in), so an unsigned int should suffice in theory? Maybe use a uint64_t if this was to be made configurable by users (see comment above, but not sure if that makes sense).
in
src/txorphanage.cpp:168
in
986f7622daoutdated
126 }
127 }
128 if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer);
129+ // Either the peer didn't have any orphans, or the amount erased is equal to what the map was storing.
130+ Assume(bytes_counted == 0);
131+ m_peer_bytes_used.erase(peer);
commit 986f7622dabd422d6dc21bfc3319a0d04d4c4ecc, [txorphanage] track bytes provided per peer:
can we do without this line and without bytes_counted and instead Assume that m_peer_bytes_used has no entry for the peer (because _EraseTx should have removed all entries for this peer and then also cleared its entry in m_peer_bytes_used).
The condition is supposed to be = orphanage didn’t have it before, but does have it now (after we called AddOrphanResolutionCandidate). Maybe I’m misunderstanding what you mean, but it looks correct to me?
I’m trying to catch the case where we add the new orphan to the orphanage, but it reaches capacity and happens to be the one evicted.
commit 0671e1c178270350aadeba625da8b95d6ef2ff00, [p2p] use all orphan announcers as potential parent sources
this line doesn’t seem to belong here, ancpkginfo hasn’t been introduced yet.
in
src/txorphanage.h:100
in
fecb522a3foutdated
79@@ -79,6 +80,10 @@ class TxOrphanage {
80 return peer_bytes_it == m_peer_bytes_used.end() ? 0 : peer_bytes_it->second;
81 }
8283+ /** Remove a peer from an orphan's announcers list, erasing the orphan if this is the only peer
84+ * who announced it. If the orphan doesn't exist or does not list this peer as an announcer, do nothing. */
85+ void EraseOrphanOfPeer(const uint256& wtxid, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
mzumsande
commented at 10:58 pm on June 22, 2023:
contributor
I reviewed part 1 (orphan handling), will continue to the p2p part next week.
These changes make sense to me on a high level, I think it would be ok to open a separate non-draft PR for this -
maybe after adding some more context to commit messages, for example it would make review easier if there is a short explanation why something is changed, even if it becomes apparent in later commits.
[rpc] allow submitpackage to be called outside of regtest1e65d3d013
[doc] add release note for submitpackagee5ec3d325d
[validation] call AcceptSingleTransaction when only 1 package tx left
Avoid calling PackageMempoolChecks() when there is only 1 transaction.
Note to reviewers: there is a slight change in the error type returned,
as shown in the txpackage_tests change. When a transaction is the last
one left in the package and its fee is too low, this returns a PCKG_TX
instead of PCKG_POLICY. This interface is clearer;
"package-fee-too-low" for 1 transaction would be a bit misleading.
8b724ee92e
MOVEONLY: move package checks into helper functionsca7507b5d5
scripted-diff: rename CheckPackage to IsPackageWellFormed
[packages] AncestorPackage sorts and builds ancestor subsets
We cannot require that peers send topologically sorted lists, because we
cannot check for this property without ensuring we have the same chain
tip and ensuring we have the full ancestor set. Instead, add the ability
to handle arbitrarily ordered transaction lists.
5c5edd2b0d
[fuzz] AncestorPackage14b7a7302e
[validation] skip and pre-fill txns with irreversibly invalid ancestors
Packageifier calculates the in-package ancestors. We already know
this tx will fail because it depends on something invalid (specifically
for a non-policy reason). We also know that it is missing at least one
input (the tx that did not and will not make it into our mempool).
Note to reviewers: slight behavior change. If this tx has multiple
errors, there may be a difference in which one is returned. For
example, if the tx has a dust output in addition to relying on
the invalid tx, we will return TX_MISSING_INPUTS when we previously
would have returned TX_NOT_STANDARD. This is simply because dust is
checked earlier within PreChecks().
Add a test that we don't quit *too* early and reject transactions we
should keep.
5311490e0d
[validation] skip last tx in AcceptPackage "submit individually" loop
Prior to this commit, if the last tx is invalid for a policy, missing
inputs, or "mempool full", we validate it twice (once inside this loop,
and then again afterwards). This is unnecessary.
If we come to the last transaction in the package, break.
This needs to be after the !subpackage quit early condition so that
that the child is present in the results map.
subfill the child state for missing inputs
f0e74c0d10
[validation] validate packages by submitting each tx's ancestor sub-packages
This results in the incentive-compatible transactions ending up in our
mempool. Prior to this commit, if parents within the package relied on
each other, we could end up (1) accepting a low-feerate child or (2)
rejecting high-feerate parents. Instead of validating each transaction
*individually* in turn, validate each one with their in-package ancestor
set. This means parents with inter-dependencies are validated correctly,
while we continue using aggregate totals for package feerate.
7f1d42ac10
[policy] allow any ancestor package, not just child-with-unconfirmed-parents
We can safely allow any ancestor package since Packageifier can handle
things that are out of order. Remove the check that "all unconfirmed
parents are present" because, even if a tx is missing inputs, the other
transactions may be worth validating.
ad9d6cc651
MOVEONLY: rename AcceptPackage to AcceptAncestorPackageb2e737ddf1
-- Part 1: Orphan Resolution Module --
Create TxPackageTracker module, responsible for handling orphan
resolution. Enable node to retry orphan parent downloading from multiple
peers who announced the tx. Use preferred peers and avoid overloading
peers with too many requests.
Making the orphanage more reliable is also necessary because it becomes
part of the "critical path" for some transactions. A 0-fee parent
bumped by a child *must* be relayed via package relay; if the orphanage
overflows or is churned by a malicious peer, the package would be
censored from the network.
[p2p] use all orphan announcers as potential parent sources
Makes orphan handling more robust. We will also use the orphan
resolution tracker for requesting ancpkginfo. The timeout in
p2p_segwit.py has been increased to account for the fact that parents
are no longer immediately requested.
bb38ec655a
[doc] no notfound handling for orphan resolution by parent txid
Note this is status quo.
2a745eebb7
[functional test] orphan handling with multiple announcers and dosers47fc8a3994
[txorphanage] add ability to protect from random eviction00d1b3d7d8
[unit test] orphanage protectionf069d4f5a0
-- Part 2: Negotiate Package Relay, Request Ancestor Package Info For Orphans --544b02a3a9
[txpackagetracker] negotiation logic59ad158e21
[unit test] txpackagetracker testsa4c023feb7
[p2p] signal support for package relaye32dd99e42
[rpc] expose package relay on rpc0031997b29
[packages] hash multiple transactionse7cfd56cdb
[validation/p2p] separate TxValidationResult and rejects filter for low fees
We wouldn't want to add a low-feerate transaction to m_recent_rejects
because usually that means we won't give the child a chance
(`fRejectedParents`); we instead want to fetch this orphan's low-feerate
parents and potentially accept them. However, we also shouldn't
revalidate transactions/packages that have already been rejected.
Continue to cache low-fee rejections, but separately.
Note: when ephemeral anchor transaction fails due to a missing spender,
we should similarly put it in the reconsiderable filter.
3494f96eba
[p2p] respond to getdata(ancpkginfo) requests with ancpkginfo
Conditions for responding to an ancpkginfo request are identical to
responding to a tx. That is, we only provide ancpkginfo if we would have
provided tx data as well.
848781c2d6
[txpackagetracker] create ancpkginfo requests using orphan resolution trackerae2664793b
[p2p] use ancpkginfo to make getpkgtxns requests and validate packages96900b3f3e
[p2p] don't inv fee-bumping children to non-pkgrelay peers
These transactions can be more common with package relay, but
non-package-relay peers will only waste bandwidth and orphanage space,
then end up not accepting them. Save their bandwidth by dropping
announcements of transactions that have below-fee-filter parents.
31b36500ca
[functional test] package relay works despite peers DoSing orphanage2c014e6140
fixup p2p: require PKG_RELAY_PKGTXNS negotiated for getpkgtxnsed5f2fe61e
fixup: swap PKG_RELAY bits9804659981
glozow force-pushed
on Jul 4, 2023
glozow
commented at 11:03 am on July 4, 2023:
member
Given the positive approach feedback, I’m going to polish up the Milestone 1 branch (incorporating comments from here as well) and open a non-draft PR for it.
DrahtBot removed the label
Needs rebase
on Jul 4, 2023
DrahtBot added the label
CI failed
on Jul 4, 2023
glozow
commented at 3:31 pm on July 5, 2023:
member
Opened #28031 for milestone 1, addressing the comments here pertaining to those commits.
in
src/net_processing.cpp:3187
in
9804659981outdated
3183+{
3184+ AssertLockHeld(g_msgproc_mutex);
3185+ LOCK(cs_main);
3186+ // We won't re-validate the exact same transaction or package again.
3187+ if (m_recent_rejects_reconsiderable.contains(GetPackageHash(package.m_unvalidated_txns))) {
3188+ // Should we do anything else here?
So m_recent_rejects_reconsiderable logs transaction on its wtxid (which is good to avoid witness inflation changing the result) however isn’t that two restrictive if you have PKG1 submitted with A+B, A is too low with B and then you have A+B’ where A is good enough ? Or is that TX_LOW_FEE when the transaction isn’t good enough to meet “mempool min fee not meet” / “min relay fee not met” ?
If it’s correct, I would still favor to exempt package transaction from min relay fee checks as it’s some awful interdependency for pre-signed lightning transaction :)
Yes, that’s the idea of “reconsiderable.” If you get A again with a different package, you reconsider it. If you get A+B again, you don’t. In this set of changes, TX_LOW_FEE is used to decide to put something in the reconsiderable cache.
Okay “reconsiderable” makes sense to me, still have to review it’s correctly implemented.
If it’s correct, I would still favor to exempt package transaction from min relay fee checks as it’s some awful interdependency for pre-signed lightning transaction :)
For this concern, in fact it’s already addressed by the latest state of bip 331 code and nversion=3 documentation, confusion is mine, I thought mempool min fee checks on parent was still present from previous package / acceptance relay reviews.
in
src/validation.cpp:1488
in
9804659981outdated
1507+
1508+ const auto single_res_it = subpackage_result.m_tx_results.find(wtxid);
1509+ if (single_res_it != subpackage_result.m_tx_results.end()) {
1510+ const auto single_res = single_res_it->second;
1511+ if (single_res.m_state.GetResult() != TxValidationResult::TX_MEMPOOL_POLICY &&
1512+ single_res.m_state.GetResult() != TxValidationResult::TX_LOW_FEE &&
I think this check is correct in what it aims to achieve, i.e not apply the two mempool min fee checks on ancestor package transaction, if it received as such from AcceptAncestorPackage / ProcessNewPackage, however this is unclear if this exemption apply for all BIP331 package, indifferently of their transactions versions, or only for nversion=3 ones.
3494f96ebacf53cbd5fd09f6421d9dabec87361e:
should update this line too because fee is no longer included in it
in
src/net_processing.cpp:867
in
3494f96ebaoutdated
862+ *
863+ * Parameters are picked to be identical to that of m_recent_rejects, with the same rationale.
864+ * Memory used: 1.3 MB
865+ * FIXME: this filter can probably be smaller, but how much smaller?
866+ */
867+ CRollingBloomFilter m_recent_rejects_reconsiderable GUARDED_BY(::cs_main){120'000, 0.000'001};
3494f96ebacf53cbd5fd09f6421d9dabec87361e:
Do we need to adjust the logic in ProcessOrphanTx() too, by inserting there into m_recent_rejects_reconsiderable instead of m_recent_rejects if the reason was low-fee? It currently adds the wtxid of a reconsidered orphan to m_recent_rejects if it fails for policy/fee-related reasons. But if that resolved orphan is itself one of the parent of another orphan we wouldn’t consider that (possibly high-fee) child because a parent was rejected.
in
src/node/txpackagetracker.cpp:178
in
ae2664793boutdated
175+ LogPrint(BCLog::TXPACKAGES, "\nPotential orphan resolution for %s from package relay peer=%d\n", wtxid.ToString(), nodeid);
176+ orphan_request_tracker.ReceivedInv(nodeid, GenTxid::Wtxid(wtxid), is_preferred, reqtime);
177+ } else {
178+ // Even though this stores the orphan wtxid, is_wtxid=false because we will be requesting the parents via txid.
179+ LogPrint(BCLog::TXPACKAGES, "\nPotential orphan resolution for %s from legacy peer=%d\n", wtxid.ToString(), nodeid);
180+ orphan_request_tracker.ReceivedInv(nodeid, GenTxid::Txid(wtxid), is_preferred, reqtime);
The approach to add a wtxid disguised as a txid to distinguish between legacy orphan processing and package relay seems a bit like a hack to me.
I’m not strictly against it, but I guess I don’t completely understand yet why it’s necessary - the information whether a peer supports package relay does not change, so why can’t we just always use GenTxid::Wtxid(wtxid) and check again in GetOrphanRequests() and elsewhere whether we do package relay with the peer instead of checking whether it’s a txid / wtxid?
right - I’ll copy the comment so that we can discuss there - this one can be resolved.
in
src/net_processing.cpp:5136
in
dd6fb139b8outdated
5130@@ -5077,6 +5131,13 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
5131 // If we receive a NOTFOUND message for a tx we requested, mark the announcement for it as
5132 // completed in TxRequestTracker.
5133 m_txrequest.ReceivedResponse(pfrom.GetId(), inv.hash);
5134+ } else if (inv.IsMsgAncPkgInfo() && m_enable_package_relay) {
5135+ if (!m_txpackagetracker->PkgInfoAllowed(pfrom.GetId(), inv.hash, node::PKG_RELAY_ANCPKG)) {
5136+ LogPrint(BCLog::NET, "\nUnsolicited ancpkginfo sent, disconnecting peer=%d\n", pfrom.GetId());
PkgInfoAllowed() calling ReceivedResponse() as a side effect is surprising to me. Wouldn’t it be better if the caller would also have to explicitly call ForgetPkgInfo() if they want to forget about the package? This is also not mentioned in its doc.
in
src/node/txpackagetracker.cpp:220
in
ae2664793boutdated
224- continue;
225+ if (gtxid.IsWtxid()) {
226+ Assume(info_per_peer.find(nodeid) != info_per_peer.end());
227+ // Add the orphan's wtxid as-is.
228+ LogPrint(BCLog::TXPACKAGES, "\nResolving orphan %s, requesting by ancpkginfo from peer=%d\n", gtxid.GetHash().ToString(), nodeid);
229+ results.emplace_back(gtxid);
I think that the splitting into commits is not ideal here: In ae2664793b1d0d86bfa32c3e635d83b1ee083e60, we add logic to return the wtxid, expecting it to be requested by MSG_ANCPKGINFO. But the net_processing code to actually do this is only added in dd6fb139b8887f3400a65dc0d4c4ee54cf53d0bc, so that at ae2664793b1d0d86bfa32c3e635d83b1ee083e60, we’d send out a MSG_TX getdata, thus re-requesting a TX we already know. These two things should go together into one commit.
in
src/net_processing.cpp:3462
in
e32dd99e42outdated
3456@@ -3440,6 +3457,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
34573458 if (greatest_common_version >= WTXID_RELAY_VERSION) {
3459 m_connman.PushMessage(&pfrom, msg_maker.Make(NetMsgType::WTXIDRELAY));
3460+ if (m_enable_package_relay) {
3461+ m_txpackagetracker->ReceivedVersion(peer->m_id);
3462+ if (!m_ignore_incoming_txs) {
We should also disable sending “sendpackages” for outgoing block-relay-only connections and probably also feelers, would suggest to just use RejectIncomingTxs().
Also, I think this should be moved up and combined with the m_enable_package_relay check two lines above, because I see no reason to call m_txpackagetracker->ReceivedVersion() in these scenarios.
mzumsande
commented at 8:27 pm on July 17, 2023:
contributor
Some initial comments on part 2 (Negotiate Package Relay, Request Ancestor Package Info For Orphans) - will move on to part 3 soon.
DrahtBot
commented at 10:52 am on July 20, 2023:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
DrahtBot added the label
Needs rebase
on Jul 20, 2023
instagibbs
commented at 6:00 pm on August 28, 2023:
member
Are you sure? We still require an ancpkginfo to have (non-configurable) MAX_PACKAGE_COUNT or fewer transactions, otherwise we exit “discarding packageinfo, too many transactions”. But I can add a check that the list is <= MAX_PKGTXNS_COUNT prior to sending.
fixup: make sure we never send getpkgtxns size > MAX_PKGTXNS_COUNT0ca60b79fd
fixup: use single-sha256 for MSG_PKGTXNS combined hash2614777aa1
glozow
commented at 11:00 am on August 29, 2023:
member
Added check that outbound getpkgtxns is <= MAX_PKGTXNS_COUNT. Also changed the combined hash from double-sha256 to single-sha256 (which was changed in the BIP recently).
instagibbs
commented at 1:07 pm on September 5, 2023:
member
Dropping note from offline discussion: probably worthwhile to just to advise users that setting ancestor count higher than 25 may end in weird behavior with chains longer than what package relay can support. I don’t know of anyone who sets this value higher manually for production usage, and the value can be updated later easily as an implementation detail.
in
src/net_processing.cpp:813
in
2614777aa1
812+ * Filter for transactions that were recently rejected by the mempool for reasons other than too
813+ * low fee. This filter only contains wtxids and txids of individual transactions.
814+ *
815+ * Upon receiving an announcement for a transaction, if it exists in this filter, do not
816+ * download the txdata. Upon receiving a package info, if it contains a transaction in this
817+ * filter, do not download the tx data.
ariard
commented at 4:56 pm on September 18, 2023:
I think it can say do not download “the whole package data”, if this is referencing to the check L4415 in src/net_processing.cpp as pkg info itself is discarded.
DrahtBot
commented at 0:37 am on December 18, 2023:
contributor
There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
glozow
commented at 9:45 am on December 18, 2023:
member
Not intended for merge. Not planning on rebasing right now since this should be built on top of cluster mempool and other WIP projects.
DrahtBot
commented at 0:09 am on March 16, 2024:
contributor
⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
achow101 referenced this in commit
d813ba1bc4
on Apr 30, 2024
DrahtBot
commented at 0:38 am on June 13, 2024:
contributor
⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
DrahtBot
commented at 0:50 am on September 10, 2024:
contributor
⌛ There hasn’t been much activity lately and the patch still needs rebase. What is the status here?
Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-09-28 22:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
