p2p: opportunistically accept 1-parent-1-child packages

glozow commented at 4:25 pm on November 29, 2023: member

This enables 1p1c packages to propagate in the “happy case” (i.e. not reliable if there are adversaries) and contains a lot of package relay-related code. See #27463 for overall package relay tracking.

Rationale: This is “non-robust 1-parent-1-child package relay” which is immediately useful.

Relaying 1-parent-1-child CPFP when mempool min feerate is high would be a subset of all package relay use cases, but a pretty significant improvement over what we have today, where such transactions don’t propagate at all. [1]
Today, a miner can run this with a normal/small maxmempool to get revenue from 1p1c CPFP’d transactions without losing out on the ones with parents below mempool minimum feerate.
The majority of this code is useful for building more featureful/robust package relay e.g. see the code in #27742.

The first 2 commits are followups from #29619:

Q: What makes this short of a more full package relay feature?

(1) it only supports packages in which 1 of the parents needs to be CPFP’d by the child. That includes 1-parent-1-child packages and situations in which the other parents already pay for themselves (and are thus in mempool already when the package is submitted). More general package relay is a future improvement that requires more engineering in mempool and validation - see #27463.

(2) We rely on having kept the child in orphanage, and don’t make any attempt to protect it while we wait to receive the parent. If we are experiencing a lot of orphanage churn (e.g. an adversary is purposefully sending us a lot of transactions with missing inputs), we will fail to submit packages. This limitation has been around for 12+ years, see #27742 which adds a token bucket scheme for protecting package-related orphans at a limited rate per peer.

(3) Our orphan-handling logic is somewhat opportunistic; we don’t make much effort to resolve an orphan beyond asking the child’s sender for the parents. This means we may miss packages if the first sender fails to give us the parent (intentionally or unintentionally). To make this more robust, we need receiver-side logic to retry orphan resolution with multiple peers. This is also an existing problem which has a proposed solution in #28031.

[1]: see this writeup and its links https://github.com/bitcoin/bips/blob/02ec218c7857ef60914e9a3d383b68caf987f70b/bip-0331.mediawiki#propagate-high-feerate-transactions

glozow added the label P2P on Nov 29, 2023

DrahtBot commented at 4:25 pm on November 29, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, sr-gi, theStack, dergoegge, achow101
Concept ACK	murchandamus, sdaftuar

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)

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 renamed this:
~~p2p: opportunistically accept 1-parent-1-child packages~~
[WIP] p2p: opportunistically accept 1-parent-1-child packages
on Nov 29, 2023

DrahtBot added the label CI failed on Nov 29, 2023

DrahtBot added the label Needs rebase on Dec 11, 2023

glozow force-pushed on Dec 11, 2023

DrahtBot removed the label CI failed on Dec 11, 2023

DrahtBot removed the label Needs rebase on Dec 11, 2023

glozow commented at 7:31 pm on December 11, 2023: member

Rebased and fixed CI. This is in draft because I’m focusing on v3 stuff, can be reviewed for its approach.

glozow force-pushed on Dec 18, 2023

DrahtBot added the label CI failed on Jan 16, 2024

instagibbs commented at 2:43 pm on February 22, 2024: member

ready for un-draft?

glozow force-pushed on Feb 22, 2024

DrahtBot removed the label CI failed on Feb 22, 2024

glozow renamed this:
~~[WIP] p2p: opportunistically accept 1-parent-1-child packages~~
p2p: opportunistically accept 1-parent-1-child packages
on Feb 23, 2024

glozow marked this as ready for review on Feb 27, 2024

in src/net_processing.cpp:3079 in bbf1b836fa outdated

3085-            m_orphanage.AddChildrenToWorkSet(*porphanTx);
3086-            m_orphanage.EraseTx(orphanHash);
3087-            for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) {
3088-                AddToCompactExtraTransactions(removedTx);
3089-            }
3090+            ProcessValidTx(porphanTx, peer.m_id, result.m_replaced_transactions.value());

instagibbs commented at 4:26 pm on February 28, 2024:

I know it’s populated due to this being accepted, but I think Assume(result.m_replaced_transactions.has_value()) would be a good belt and suspenders to avoid UB in case of regression e.g., we redefined std::nulltopt mean no replacements

in src/net_processing.cpp:590 in 3eddfb2147 outdated

581@@ -582,6 +582,15 @@ class PeerManagerImpl final : public PeerManager
582      */
583     bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer);
584 
585+    enum class InvalidTxTask : uint8_t {
586+        NONE,
587+        ADD_ORPHAN,
588+    };
589+    /** Handle a transaction whose result was MempoolAcceptResult::ResultType::INVALID.
590+     * @returns true if this transaction is an orphan we should try to resolve. */

instagibbs commented at 4:34 pm on February 28, 2024:

this doesn’t return a boolean

in src/net_processing.cpp:4377 in 3eddfb2147 outdated

4384-            unique_parents.reserve(tx.vin.size());
4385-            for (const CTxIn& txin : tx.vin) {
4386-                // We start with all parents, and then remove duplicates below.
4387-                unique_parents.push_back(txin.prevout.hash);
4388+            for (const CTransactionRef& removedTx : result.m_replaced_transactions.value()) {
4389+                AddToCompactExtraTransactions(removedTx);

instagibbs commented at 4:38 pm on February 28, 2024:

copy/paste error? isn’t this already done in ProcessValidTx just above?

glozow commented at 10:19 am on March 11, 2024:

fixed

in src/net_processing.cpp:3109 in 3eddfb2147 outdated

3238-                    // We only add the txid if it differs from the wtxid, to
3239-                    // avoid wasting entries in the rolling bloom filter.
3240-                    m_recent_rejects.insert(porphanTx->GetHash().ToUint256());
3241-                }
3242-            }
3243-            m_orphanage.EraseTx(orphanHash);

instagibbs commented at 2:43 pm on March 4, 2024:

previously MISSING_INPUTS would have resulted in a continuation of the loop. A side-effect of this change is that we will detect if the orphan was rejected due to m_recent_rejects entries of its parents, which seems like a strict improvement?

glozow commented at 2:30 pm on March 8, 2024:

Rewrote to be more strict refactor to give reviewers less to think about. I do think it makes sense that if a parent has been rejected since the last time we looked at this orphan, we get rid of it, but I suppose we can think about that later.

in src/net_processing.cpp:3104 in 3eddfb2147 outdated

3099+            // parents so avoid re-requesting it from other peers.
3100+            // Here we add both the txid and the wtxid, as we know that
3101+            // regardless of what witness is provided, we will not accept
3102+            // this, so we don't need to allow for redownload of this txid
3103+            // from any of our non-wtxidrelay peers.
3104+            m_recent_rejects.insert(tx->GetHash().ToUint256());

instagibbs commented at 2:45 pm on March 4, 2024:

future work: might make sense to delete this transaction from the orphanage as well in this case if it exists so we don’t try it with other peer?

in src/net_processing.cpp:3060 in 3eddfb2147 outdated

3055+             nodeid,
3056+             state.ToString());
3057+    // Maybe punish peer that gave us an tx
3058+    MaybePunishNodeForTx(nodeid, state);
3059+
3060+    switch (state.GetResult()) {

instagibbs commented at 3:02 pm on March 4, 2024:

we used to call m_orphanage.EraseTx(orphanHash); for each non-TX_MISSING_INPUTS possibility, but due to early return we don’t. Can we sync this back up or justify each case this has changed for?

glozow commented at 2:30 pm on March 8, 2024:

Old behavior should be preserved now

in src/net_processing.cpp:4415 in 3eddfb2147 outdated

4424-                // from any of our non-wtxidrelay peers.
4425-                m_recent_rejects.insert(tx.GetHash().ToUint256());
4426-                m_recent_rejects.insert(tx.GetWitnessHash().ToUint256());
4427-                m_txrequest.ForgetTxHash(tx.GetHash());
4428-                m_txrequest.ForgetTxHash(tx.GetWitnessHash());
4429+            } else if (RecursiveDynamicUsage(*ptx) < 100000) {

instagibbs commented at 3:13 pm on March 4, 2024:

with changes, we now add to extra txns even if witness stripped; seems wrong

glozow commented at 2:31 pm on March 8, 2024:

fixed

in test/functional/p2p_1p1c_package_relay.py:185 in 19015e3c9a outdated

176+        self.test_individual_logic()
177+
178+        self.log.info("Check end-to-end package relay across multiple nodes")
179+        self.packages_to_submit = []
180+        self.transactions_to_presend = [[]] * self.num_nodes
181+        self.replacement_packages = []

instagibbs commented at 3:24 pm on March 4, 2024:

this is never set

glozow commented at 12:01 pm on March 13, 2024:

Oops, forgot to delete from a package RBF test I built on top. Fixed

instagibbs commented at 3:25 pm on March 4, 2024: member

I focused on the first two refactoring commits. Might be helpful zeroing in on these since we probably need to justify each small behavior change or make sure we’re aligning with old behavior.

in test/functional/p2p_1p1c_package_relay.py:40 in 19015e3c9a outdated

34+FEERATE_1SAT_VB = Decimal("0.00001000")
35+
36+class PackageRelayTest(BitcoinTestFramework):
37+    def set_test_params(self):
38+        self.setup_clean_chain = True
39+        self.num_nodes = 4

brunoerg commented at 6:36 pm on March 4, 2024:

In 19015e3c9aed5709e776ef15bf9e73f126c7ea29: It seems to be using 3 nodes instead of 4. Or am I missing something?

E.g.:

 0diff --git a/test/functional/p2p_1p1c_package_relay.py b/test/functional/p2p_1p1c_package_relay.py
 1index 6a3ba7c3df..dda671dede 100755
 2--- a/test/functional/p2p_1p1c_package_relay.py
 3+++ b/test/functional/p2p_1p1c_package_relay.py
 4@@ -36,7 +36,7 @@ FEERATE_1SAT_VB = Decimal("0.00001000")
 5 class PackageRelayTest(BitcoinTestFramework):
 6     def set_test_params(self):
 7         self.setup_clean_chain = True
 8-        self.num_nodes = 4
 9+        self.num_nodes = 3
10         self.extra_args = [[
11             "-datacarriersize=100000",
12             "-maxmempool=5",
13@@ -85,7 +85,7 @@ class PackageRelayTest(BitcoinTestFramework):
14         # Child should already be in orphanage
15         self.transactions_to_presend[1] = [high_fee_child["tx"]]
16         # Parent would have been previously rejected
17-        self.transactions_to_presend[3] = [low_fee_parent["tx"]]
18+        self.transactions_to_presend[2] = [low_fee_parent["tx"]]
19 
20     def test_individual_logic(self):
21         node = self.nodes[0]
22@@ -213,7 +213,7 @@ class PackageRelayTest(BitcoinTestFramework):
23 
24         self.log.info("Submit replacement package to node3")
25         for package_hex in self.replacement_packages:
26-            self.nodes[3].submitpackage(package_hex)
27+            self.nodes[2].submitpackage(package_hex)
28 
29         self.log.info("Wait for mempools to sync")
30         self.sync_mempools(timeout=20)

glozow commented at 11:23 am on March 5, 2024:

The idea is this is a “network” test to see that the package propagates across multiple hops. 1 node is the sender, 1 node pre-receives the parent, 1 node pre-receives the child, and 1 node pre-receives nothing.

brunoerg commented at 11:48 am on March 5, 2024:

Got it, thank you.

glozow commented at 2:45 pm on March 20, 2024:

added a comment on top of where the package is created

glozow commented at 11:24 am on March 5, 2024: member

Working on pulling the first 2 commits out into a separate PR.

in src/net_processing.cpp:3168 in 3eddfb2147 outdated

3076+        // Do not add txids of witness transactions or witness-stripped
3077+        // transactions to the filter, as they can have been malleated;
3078+        // adding such txids to the reject filter would potentially
3079+        // interfere with relay of valid transactions from peers that
3080+        // do not support wtxid-based relay. See
3081+        // https://github.com/bitcoin/bitcoin/issues/8279 for details.

instagibbs commented at 3:11 pm on March 6, 2024:

@sdaftuar honestly this comment/link doesn’t make sense to me. If it’s witness-stripped, we can’t add it to m_recent_rejects since that would blind us to any real version of the tx?

sdaftuar commented at 3:34 pm on March 6, 2024:

I had to remind myself of how this works, but the concern contemplated in that comment is that if the only copy of a non-witness-stripped version of a transaction is from a node that has not yet upgraded to wtxid-based-relay, then an adversary could blind us to that transaction by relaying a witness-stripped version of it (and then, since txid==wtxid for witness-stripped transactions, the hash that we’d use to fetch the transaction from the non-wtxid-relay peer who has the correct version would be in our m_recent_rejects, and we wouldn’t ever request it).

At the time that I wrote that comment, I think I also said that once wtxid-based-relay is sufficiently deployed, that we could stop worrying about this… I don’t recall when wtxid-relay was deployed but maybe we’re at that point already?

There’s another link to a comment thread in the PR that implemented wtxid-relay which discusses the potential impact on downstream projects, so maybe if we were to make a behavior change we should communicate that in advance so that no one is surprised.

glozow commented at 4:00 pm on March 6, 2024:

It seems like the comment thread is concerned with what happens if: (1) the sender is using non-wtxidrelay (and thus only announces transactions by txid), (2) all of its peers are caching witness-stripped rejections by wtxid (i.e. txid), (3) there is an attacker trying to censor the sender’s tx by sending witness-stripped versions of the tx ahead of the sender.

This isn’t a problem if (1) isn’t true, i.e. the sender is using wtxidrelay, e.g. any non-eol version of Bitcoin Core. If we see that the vast majority of nodes (that send transactions) are sending a wtxidrelay message, perhaps it would be safe to remove the special casing (with a message to downstream projects)?

Edit: perhaps not, if btcd is an example of (1)? https://github.com/btcsuite/btcd/blob/a4f447006e7538f9e3d5ae90f54700c97fea9c3d/peer/peer.go#L2214

glozow commented at 10:42 am on March 21, 2024:

Resolving as out of scope for this PR. Also fwiw, btcd doing announcements by txid seems to be a reason to keep it for now…

glozow force-pushed on Mar 8, 2024

DrahtBot added the label CI failed on Mar 8, 2024

DrahtBot commented at 2:23 pm on March 8, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/22437958726

DrahtBot removed the label CI failed on Mar 8, 2024

glozow commented at 10:54 am on March 11, 2024: member

Opened #29619 for the first 2 commits, moving this to draft.

glozow marked this as a draft on Mar 11, 2024

achow101 referenced this in commit 264ca9db24 on Mar 13, 2024

glozow force-pushed on Mar 13, 2024

glozow commented at 12:08 pm on March 13, 2024: member

rebased and added followups from #29619

glozow marked this as ready for review on Mar 13, 2024

in src/net_processing.cpp:861 in 1435bfac51 outdated

856+     * Upon receiving an announcement for a transaction, if it exists in this filter, do not
857+     * download the txdata.
858+     *
859+     * Reset this filter when the chain tip changes.
860+     *
861+     * Parameters are picked to be the same false positive rate but half the capacity as

instagibbs commented at 7:20 pm on March 13, 2024:

there any science to these choices?

glozow commented at 9:26 am on March 19, 2024:

Not much science, no. It makes sense to me to use the same false positive rate. As for size, I’ll run my node for a bit longer and maybe use the ratio of m_recent_rejects / m_recent_rejects_reconsiderable usage to give a more scientific number.

glozow commented at 4:47 pm on March 20, 2024:

Actually, rethinking this, if the capacity is informed by maximum churn, it should just be the same as m_recent_rejects.

in src/net_processing.cpp:857 in 1435bfac51 outdated

805@@ -806,7 +806,7 @@ class PeerManagerImpl final : public PeerManager
806     /** Stalling timeout for blocks in IBD */
807     std::atomic<std::chrono::seconds> m_block_stalling_timeout{BLOCK_STALLING_TIMEOUT_DEFAULT};
808 
809-    bool AlreadyHaveTx(const GenTxid& gtxid)
810+    bool AlreadyHaveTx(const GenTxid& gtxid, bool include_reconsiderable)

instagibbs commented at 7:36 pm on March 13, 2024:

include_reconsiderable seems to be true on every call-site. Perhaps this is vestigial from “real” package relay PRs of times past? Might be worth noting/making it default true, or dropping altogether.

glozow commented at 9:26 am on March 19, 2024:

~~Vestigial from a previous iteration of this PR and future changes. Will drop~~ EDIT: I forgot to flip some

glozow commented at 5:32 pm on March 20, 2024:

Turns out I actually just forgot to switch from true to false in a few places, and hadn’t included a test for which these were important. I’ve added them back now and wrote a test (see “package basic nonsegwit”). Basically, we need to make sure we don’t include reconsiderable when we’re filtering for rejected parents on a TX_MISSING_INPUTS and when we’re sending the GETDATAs, in case our parent was a low-feerate nonsegwit tx that we saw previously.

instagibbs commented at 1:11 pm on March 27, 2024:

can confirm, changes seem logical, and there’s test coverage for each AlreadyHaveTx instance

instagibbs commented at 10:12 am on April 3, 2024:

Good time to add documentation to this function

glozow commented at 3:43 pm on April 5, 2024:

Added. Also WOW, I realized we somehow lost the line where m_recent_rejects_reconsiderable is reset. Fixed that.

instagibbs commented at 4:37 pm on April 5, 2024:

we should have tests(in master?) for this I hope.

something below minfee is rejected, block comes in, node should respond to an INV for the same thing again

glozow commented at 4:53 pm on April 5, 2024:

we should have tests(in master?) for this I hope.

apparently not?

glozow commented at 11:06 am on April 16, 2024:

marking as resolved since #29827 does this

in src/net_processing.cpp:2229 in 1435bfac51 outdated

2225@@ -2209,6 +2226,8 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid)
2226 
2227     if (m_orphanage.HaveTx(gtxid)) return true;
2228 
2229+    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(gtxid.GetHash())) return true;

instagibbs commented at 7:37 pm on March 13, 2024:

0    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true;

glozow commented at 5:42 pm on March 20, 2024:

done

in src/net_processing.cpp:611 in 47efa940c4 outdated

595@@ -596,6 +596,12 @@ class PeerManagerImpl final : public PeerManager
596     void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
597         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
598 
599+    void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, NodeId nodeid)
600+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
601+
602+    void MaybeProcess1P1CPackage(const CTransactionRef& ptx, NodeId nodeid)

instagibbs commented at 7:39 pm on March 13, 2024:

please give some documentation

glozow commented at 4:01 pm on March 20, 2024:

added

in src/net_processing.cpp:602 in 47efa940c4 outdated

595@@ -596,6 +596,12 @@ class PeerManagerImpl final : public PeerManager
596     void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
597         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
598 
599+    void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, NodeId nodeid)

instagibbs commented at 7:40 pm on March 13, 2024:

please give some documentation

glozow commented at 4:01 pm on March 20, 2024:

added

in src/net_processing.cpp:4651 in 47efa940c4 outdated

4551@@ -4450,6 +4552,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4552         if (state.IsInvalid()) {
4553             ProcessInvalidTx(pfrom.GetId(), ptx, state, /*maybe_add_extra_compact_tx=*/true);
4554         }
4555+        if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) {

instagibbs commented at 6:38 pm on March 14, 2024:

0        if (m_recent_rejects_reconsiderable.contains(wtxid)) {

think this is the same check and would match other call of the function

glozow commented at 3:00 pm on March 20, 2024:

I prefer how it is right now as

it’s clear that it’s based on the result we just got from mempool validation
it doesn’t require peerman having direct access to m_recent_rejects_reconsiderable (which I would want to move into txdownloadman module)

instagibbs commented at 7:44 pm on March 26, 2024:

with the added Assume() in MaybeProcess1P1CPackage I think it’s ok now from this reader’s standpoint, thanks

in src/net_processing.cpp:4429 in 47efa940c4 outdated

4425@@ -4334,6 +4426,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4426         m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
4427         if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
4428 
4429+        // If we find this transaction in m_recent_rejects_reconsiderable, we shouldn't try it by

instagibbs commented at 7:13 pm on March 14, 2024:

Mention that this is “merely” an optimization(and also reduces the churn in the bloom filter as well)?

glozow commented at 3:20 pm on March 20, 2024:

What do you mean by optimization? It’s the same idea as m_recent_rejects, we don’t want to waste bandwidth even though we are willing to retry low feerate things

in src/net_processing.cpp:4432 in 47efa940c4 outdated

4425@@ -4334,6 +4426,16 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4426         m_txrequest.ReceivedResponse(pfrom.GetId(), txid);
4427         if (tx.HasWitness()) m_txrequest.ReceivedResponse(pfrom.GetId(), wtxid);
4428 
4429+        // If we find this transaction in m_recent_rejects_reconsiderable, we shouldn't try it by
4430+        // itself again. However, look for a matching child in the orphanage and maybe submit it
4431+        // again as a package.
4432+        if (m_recent_rejects_reconsiderable.contains(wtxid)) {

instagibbs commented at 7:31 pm on March 14, 2024:

if MaybeProcess1P1CPackage handles the (non)existence in the m_recent_rejects_reconsiderable filter, you could stick this inside the AlreadyHaveTx block, which would allow existing logging to continue happening, and seems pretty natural?

glozow commented at 3:21 pm on March 20, 2024:

Ah good point as I just realize the previous thing was breaking the forcerelay stuff for low feerate txns. Moved inside that block.

in src/net_processing.cpp:3271 in 47efa940c4 outdated

3229+            }
3230+        }
3231+    }
3232+}
3233+
3234+void PeerManagerImpl::MaybeProcess1P1CPackage(const CTransactionRef& ptx, NodeId nodeid)

instagibbs commented at 12:44 pm on March 15, 2024:

0void PeerManagerImpl::MaybeProcess1P1CPackage(const CTransactionRef& parent_ptx, NodeId nodeid)

glozow commented at 5:13 pm on March 20, 2024:

Well, it might not be a parent :shrug: most likely it’s just a low feerate transaction actually. I didn’t change but don’t feel strongly

in src/net_processing.cpp:3243 in 47efa940c4 outdated

3238+    AssertLockHeld(cs_main);
3239+
3240+    const auto cpfp_candidates{m_orphanage.GetChildren(ptx, nodeid)};
3241+    bool tried_package_cpfp{false};
3242+
3243+    const auto& txid{ptx->GetHash()};

instagibbs commented at 12:46 pm on March 15, 2024:

0    const auto& parent_txid{parent_ptx->GetHash()};

as well as wtxid just below

glozow commented at 5:43 pm on March 20, 2024:

done

in src/net_processing.cpp:3306 in 47efa940c4 outdated

3240+    const auto cpfp_candidates{m_orphanage.GetChildren(ptx, nodeid)};
3241+    bool tried_package_cpfp{false};
3242+
3243+    const auto& txid{ptx->GetHash()};
3244+    const auto& wtxid{ptx->GetWitnessHash()};
3245+

instagibbs commented at 12:50 pm on March 15, 2024:

0
1    Assume(m_recent_rejects_reconsiderable.contains(parent_wtxid.ToUint256()));

glozow commented at 5:29 pm on March 20, 2024:

added

in src/net_processing.cpp:3205 in 47efa940c4 outdated

3200+    // Iterate backwards to erase in-package descendants from the orphanage before they become
3201+    // relevant in AddChildrenToWorkSet.
3202+    for (auto package_it = package.rbegin(); package_it != package.rend(); ++package_it) {
3203+        const auto& tx = *package_it;
3204+        const auto it_result{package_result.m_tx_results.find(tx->GetWitnessHash())};
3205+        if (it_result != package_result.m_tx_results.end()) {

instagibbs commented at 5:02 pm on March 15, 2024:

0        if (Assume(it_result != package_result.m_tx_results.end())) {

in src/net_processing.cpp:3284 in 47efa940c4 outdated

3218+                    // Don't add to vExtraTxnForCompact, as these transactions would already be
3219+                    // there as an orphan or too low feerate tx.
3220+                    ProcessInvalidTx(nodeid, tx, tx_result.m_state, /*maybe_add_extra_compact_tx=*/false);
3221+                    break;
3222+                }
3223+                case MempoolAcceptResult::ResultType::MEMPOOL_ENTRY:

instagibbs commented at 6:08 pm on March 15, 2024:

have you considered moving this above INVALID and letting it fall through to call ProcessInvalidTx as well?

glozow commented at 5:13 pm on March 20, 2024:

Why? There wouldn’t be anything interesting in state

instagibbs commented at 7:44 pm on March 26, 2024:

slight readability preference, feel free to ignore

in src/net_processing.cpp:3218 in 47efa940c4 outdated

3213+                    break;
3214+                }
3215+                case MempoolAcceptResult::ResultType::INVALID:
3216+                case MempoolAcceptResult::ResultType::DIFFERENT_WITNESS:
3217+                {
3218+                    // Don't add to vExtraTxnForCompact, as these transactions would already be

instagibbs commented at 6:25 pm on March 15, 2024:

We don’t add orphans to vExtraTxnForCompact though, just too low aka reconsiderable?

maybe something like:

// All packages currently considered are 1p1c, which means // any entrants to be added in vExtraTxnForCompact have already been added

glozow commented at 5:11 pm on March 20, 2024:

we do add orphans to vExtraTxnForCompact though?

in test/functional/p2p_1p1c_package_relay.py:172 in 2f25d2e084 outdated

167+        peer1.peer_disconnect()
168+        peer2.peer_disconnect()
169+        self.sync_all()
170+
171+    def run_test(self):
172+        self.ctr = 0

instagibbs commented at 7:45 pm on March 15, 2024:

unused

in test/functional/p2p_1p1c_package_relay.py:204 in 2f25d2e084 outdated

201+                peer.send_and_ping(msg_tx(tx))
202+            # This disconnect removes any sent orphans from the orphanage (EraseForPeer) and times
203+            # out the in-flight requests.  It is currently required for the test to pass right now,
204+            # because the node will not (re)try requesting orphan parents from multiple peers if the
205+            # first one fails.
206+            # TODO: remove this and test that the node retries orphan resolution with other peers

instagibbs commented at 7:57 pm on March 15, 2024:

unsure what the test is covering if it’s being deleted on disconnect?

glozow commented at 5:44 pm on March 20, 2024:

I guess not a lot. But it is worth checking that it having seen (and rejected) the child before shouldn’t impact its acceptance of the package later?

in test/functional/p2p_1p1c_package_relay.py:217 in 2f25d2e084 outdated

212+            self.nodes[0].submitpackage(package_hex)
213+
214+        self.log.info("Wait for mempools to sync")
215+        self.sync_mempools(timeout=20)
216+
217+        self.log.info("Wait for mempools to sync")

instagibbs commented at 7:58 pm on March 15, 2024:

double-sync for what?

glozow commented at 2:57 pm on March 20, 2024:

(originally there was an original broadcast, then a package RBF broadcast) no reason, deleted

in test/functional/p2p_1p1c_package_relay.py:115 in 2f25d2e084 outdated

101+        peer2 = node.add_p2p_connection(P2PInterface())
102+
103+        self.log.info("Check that tx caches low feerate rejections")
104+        parent_wtxid_int = int(low_fee_parent["tx"].getwtxid(), 16)
105+        peer1.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=parent_wtxid_int)]))
106+        peer1.wait_for_getdata([parent_wtxid_int])

instagibbs commented at 8:16 pm on March 15, 2024:

had this timeout locally once

instagibbs commented at 5:44 pm on March 26, 2024:

can’t seem to recreate anymore :shrug:

theStack commented at 0:55 am on March 27, 2024:

Seems I had beginner’s luck, got the timeout on this line after a few runs on my machine. Here is the compressed dir of the failed test run: https://github.com/theStack/bitcoin/raw/pr28970_failed_testrun/pr28970_waitforgetdata_timeout.tar.gz

Apparently the node has seen the wtx in the inv already before (see node0/regtest/debug.log:11237) :eyes: . I guess this is could be caused by a different MiniWallet instance spending the same UTXO in an earlier sub-test, resulting in the same transaction?

glozow commented at 10:47 am on March 27, 2024:

Nice @theStack :D I was having trouble recreating it, duplicate tx sounds most plausible to me.

I split the 4 node test and 1 node test into separate files (grabbed commit from #29735 to reuse setup) which hopefully gets rid of this.

in test/functional/p2p_1p1c_package_relay.py:182 in 2f25d2e084 outdated

177+        self.raise_network_minfee()
178+
179+        self.log.info("Check 1p1c validation logic on a single node")
180+        self.test_individual_logic()
181+
182+        self.log.info("Check end-to-end package relay across multiple nodes")

instagibbs commented at 8:18 pm on March 15, 2024:

could this subtest be in its own subroutine

glozow commented at 10:38 am on March 21, 2024:

done

instagibbs commented at 1:58 pm on March 18, 2024: member

some comments, running patch on a listening node longer-term to test behavior

glozow force-pushed on Mar 20, 2024

glozow commented at 5:55 pm on March 20, 2024: member

Rebased for silent conflict with #28950

glozow force-pushed on Mar 20, 2024

DrahtBot added the label CI failed on Mar 20, 2024

DrahtBot commented at 5:55 pm on March 20, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/22896614286

DrahtBot removed the label CI failed on Mar 20, 2024

dergoegge commented at 12:21 pm on March 25, 2024: member

Enable 1-parent-1-child package relay 🚀 (just using the existing protocol messages).

Imo, this isn’t really what this PR does because “package relay” (at least to me) is something that is robust and users can rely on. As you note yourself, there is more work required to make this robust.

To make this more robust, we need receiver-side logic to retry orphan resolution with multiple peers. To make this robust, we need to make efforts to protect some amount of orphans from eviction.

Why are we not making things more robust in or before this PR?

instagibbs commented at 12:28 pm on March 25, 2024: member

Imo, this isn’t really what this PR does because “package relay” (at least to me) is something that is robust and users can rely on. As you note yourself, there is more work required to make this robust.

that sounds like confusing semantic games to me? We can add “non-robust” to the title to be clearer if we think it can get merged…

Why are we not making things more robust in or before this PR?

Why would we tie up an improvement on relay on other improvements to relay? This feature is useful by itself in non-adversarial network conditions, which ends up being deliverable security and a bit of increased miner fee revenue.

Of course, if we have a orphanage hardening PR ready for merge, we should merge that.

glozow commented at 12:33 pm on March 25, 2024: member

Why are we not making things more robust in or before this PR?

Why have we been okay with handling any transactions with missing parents despite all of these orphanage problems, for more than a decade? Clearly because it’s useful even if not 100% reliable. There are various improvements we can make to relay. I have already implemented them, and decided I would prefer to propose the more impactful improvements before the less impactful ones.

dergoegge commented at 1:41 pm on March 25, 2024: member

that sounds like confusing semantic games to me? We can add “non-robust” to the title to be clearer if we think it can get merged…

I think my issue is with the PR description and how this will be marketed, not the title. The title describes it quite well: we will sometimes accept 1p1c packages (although it could also mention that these packages are constructed from the orphanage). That is different from package relay (again, to me “package relay” implies a level of reliability).

This feature is useful by itself in non-adversarial network conditions, which ends up being deliverable security

“deliverable security”: Is this referring to the increased cost of having to mess with relay to prevent pkgs from relaying? as opposed to the current state where these pkgs just don’t relay at all?

Why have we been okay with handling any transactions with missing parents despite all of these orphanage problems, for more than a decade?

Because it saves bandwidth in the honest case. Afaict, relay would be reliable without the orphanage, although much more bandwidth intensive.

I guess my wording was confusing? I have no problem with the approach in this PR nor was I suggesting to block this on other relay improvements. I was just commenting on the PR description and asking a question.

There clearly are good answers to my question but I don’t appreciate the condescending tone after being asked to review this PR.

instagibbs commented at 1:47 pm on March 25, 2024: member

“deliverable security”: Is this referring to the increased cost of having to mess with relay to prevent pkgs from relaying? as opposed to the current state where these pkgs just don’t relay at all?

Assuming there’s a path of nodes to a miner in which they are not having their orphanages aggressively churned by an adversary, these size 2 packages should be able to propagate. So if say a LN node goes offline or refuses to sign an updated commitment transaction that raises the transaction fee, minfee rises, the counterparty can still go to chain. It raises the bar a bit for an adversary, and the more benign cases can be resolved in the average case, letting people deploy liquidity elsewhere.

FWIW I ran this branch over the last week, and when my minfee wasn’t 1 I was getting about one extra package relayed every half hour.

glozow commented at 2:28 pm on March 25, 2024: member

I’ve updated the PR description to be more descriptive about what this PR does and doesn’t do. The 1p1c “package relay” thing wasn’t meant as false advertising; I added it to the PR description 1 month after opening because it seemed like people were underestimating how useful this is (someone was surprised when I said this PR propagates 1p1c packages).

I apologize for the tone - the comment read to me like a blocking criticism since it wasn’t accompanied by any other review comments. Hopefully the suggestion has been adequately addressed with the description update now.

Roasbeef commented at 0:19 am on March 26, 2024: none

Popping in here to mention that even w/ the limitations described above, this would be super useful for LN as is.

Today we’re able to use the anchor outputs on commitment transactions to CPFP a force close (* caveats re pinning, RBF, etc, etc). This works OK during calm mempool conditions, but if a rapid spike occurs that causes the bottom of the mempool to fall out, then CPFP no longer works as the parent has been evicted. During one of the recent more persistent fee spikes, we saw this happening rather frequently, which then led to cascading force closes (outgoing HTLC can’t timeout due to non confirmation, incoming times out, repeat) and a lots of pain and user confusion (can only wait or use a transaction “accelerator”).

IIUC, with this PR, upgraded peers will optimistically attempt to fetch the parent (the commitment txn) when they see a transaction that appears to be fee bumping it (anchor output spend). With this behavior, on a best effort basis, if we had another massive spike, then commitment transactions would be able to confirm as upgraded peers would fetch the parent to consider/propagate. I think this is very attractive as it doesn’t necessarily require a new p2p upgrade to gain the benefits, instead as nodes start to progressively update, then more of these otherwise unrecognized packages will start to propagate.

If major LN node operators are made aware of this patch, and start to run it with the node backing their LN nodes, then assuming a relay path to miners (who seem to be willing to mine/relay just about anything these days), this would be a rather nice stop gap while the greater v3/cluster work proceeds in the background.

in test/functional/p2p_1p1c_package_relay.py:80 in 5405171740 outdated

75+        self.log.debug("Check that all nodes' mempool minimum feerate is above min relay feerate")
76+        for node in self.nodes:
77+            assert_equal(node.getmempoolinfo()['minrelaytxfee'], FEERATE_1SAT_VB)
78+            assert_greater_than(node.getmempoolinfo()['mempoolminfee'], FEERATE_1SAT_VB)
79+
80+    def create_packages(self):

instagibbs commented at 4:20 pm on March 26, 2024:

nit: I’d rather this return the packages then spookily populate them

glozow commented at 5:23 pm on March 28, 2024:

not sure if I achieved spooky, but I’m having it return the lists now :ghost:

in src/txorphanage.cpp:257 in f4d8fe713a outdated

248+    std::vector<CTransactionRef> children_found;
249+
250+    for (unsigned int i = 0; i < parent->vout.size(); i++) {
251+        const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(parent->GetHash(), i));
252+        if (it_by_prev != m_outpoint_to_orphan_it.end()) {
253+            for (const auto& elem : it_by_prev->second) {

instagibbs commented at 7:39 pm on March 26, 2024:

to be clear we may have multiple orphans per outpoint lookup, yes?

glozow commented at 10:42 am on March 27, 2024:

Yep, we can have conflicting orphans. Should not break here if we find one.

in src/txorphanage.h:55 in f4d8fe713a outdated

50@@ -51,6 +51,10 @@ class TxOrphanage {
51     /** Does this peer have any work to do? */
52     bool HaveTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);;
53 
54+    /** Get all children of this parent. */
55+    std::vector<CTransactionRef> GetChildren(const CTransactionRef& parent, NodeId peer) const

instagibbs commented at 7:39 pm on March 26, 2024:

could stand a unit test and some basic fuzz coverage

glozow commented at 5:38 pm on March 28, 2024:

added unit tests and fuzz coverage

in src/net_processing.cpp:4539 in 774f6d0ff5 outdated

4531@@ -4420,7 +4532,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4532                     // protocol for getting all unconfirmed parents.
4533                     const auto gtxid{GenTxid::Txid(parent_txid)};
4534                     AddKnownTx(*peer, parent_txid);
4535-                    if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) AddTxAnnouncement(pfrom, gtxid, current_time);
4536+                    // Skip m_recent_rejects_reconsiderable because the missing parent may have been
4537+                    // previously rejected for being too low feerate, and this orphan may be able to
4538+                    // CPFP it if we consider them as a package.

instagibbs commented at 8:05 pm on March 26, 2024:

this change also only matters for the non-segwit case since we’re checking via txid, correct?

glozow commented at 10:32 am on March 27, 2024:

Correct. If it’s a witness tx, we would have cached the error by wtxid but the request is by txid, so this would miss it.

in src/net_processing.cpp:4485 in 774f6d0ff5 outdated

4481@@ -4378,6 +4482,14 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4482             // peer simply for relaying a tx that our m_recent_rejects has caught,
4483             // regardless of false positives.
4484             return;
4485+        } else if (m_recent_rejects_reconsiderable.contains(wtxid)) {

instagibbs commented at 8:09 pm on March 26, 2024:

tiny preference for putting this case first

glozow commented at 4:10 pm on March 28, 2024:

Ok. I’ve actually ended up nesting it inside the AlreadyHave to preserve the ForceRelay behavior… wanted to avoid a behavior change in the case

tx is low feerate (goes into m_recent_rejects_reconsiderable)
tx is later accepted (e.g. due to 1p1c)
forcrelay peer sends it to us again -> we should RelayTransaction

instagibbs commented at 8:12 pm on March 26, 2024: member

no issues running the patch so far, some more comments

theStack referenced this in commit a5212a2d0b on Mar 27, 2024

in src/txorphanage.cpp:245 in f4d8fe713a outdated

240@@ -241,3 +241,19 @@ void TxOrphanage::EraseForBlock(const CBlock& block)
241         LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx included or conflicted by block\n", nErased);
242     }
243 }
244+
245+std::vector<CTransactionRef> TxOrphanage::GetChildren(const CTransactionRef& parent, NodeId peer) const

brunoerg commented at 1:19 pm on March 28, 2024:

In f4d8fe713a036c4d2c1e7c2329077c34f75c8608: It seems NodeId peer is not used in GetChildren.

glozow commented at 4:33 pm on March 28, 2024:

removed

in test/functional/p2p_1p1c_package_relay.py:44 in 5405171740 outdated

39+        self.setup_clean_chain = True
40+        self.num_nodes = 4
41+        self.extra_args = [[
42+            "-datacarriersize=100000",
43+            "-maxmempool=5",
44+            "-whitelist=noban@127.0.0.1",  # immediate tx relay

brunoerg commented at 1:23 pm on March 28, 2024:

In 5405171740aa77e1eb1110fa8be97318edba380a: nit: could set noban_tx_relay

glozow commented at 5:18 pm on March 28, 2024:

done

glozow force-pushed on Mar 28, 2024

glozow commented at 6:03 pm on March 28, 2024: member

Thanks for the review, addressed comments and

fixed a bug in GetChildren
expanded + added more docs to the individual logic tests
got rid of the assert_debug_log lines in the functional tests. The only “coverage” that’s lost is us skipping children with which we’ve already tried+failed the package submission. I have a branch with them if you want to use it to test the code, but log asserts didn’t feel appropriate to have in the functional test.
added more logging, realized it was annoyingly hard to look up success/fail package evals while searching logs on my node

fwiw running this on a -maxmempool=150 node, for the past few days, on average:

222 attempted package validations per day
85 packages (so 170 txns) accepted per day
57% of the package transactions accepted ended up in a block. These are all transactions that we would have otherwise rejected, so this logic is definitely useful at helping us maximize the fees in our limited mempool space.

DrahtBot added the label CI failed on Mar 28, 2024

DrahtBot commented at 8:11 pm on March 28, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/23211258926

in test/functional/p2p_opportunistic_1p1c.py:44 in 5d7fa1f684 outdated

39+            "-datacarriersize=100000",
40+            "-maxmempool=5",
41+        ]]
42+        self.supports_cli = False
43+
44+    def raise_network_minfee(self):

theStack commented at 6:37 pm on March 30, 2024:

nit: raise_network_minfee is unused in this file, can be removed

glozow commented at 12:53 pm on April 2, 2024:

removed, thanks

in test/functional/p2p_opportunistic_1p1c.py:61 in 5d7fa1f684 outdated

56+        self.log.info("Check that opportunistic 1p1c logic works when child is received before parent")
57+
58+        low_fee_parent = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
59+        high_fee_child = self.wallet.create_self_transfer(utxo_to_spend=low_fee_parent["new_utxo"], fee_rate=20*FEERATE_1SAT_VB)
60+
61+        peer_sender = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=1, connection_type="outbound-full-relay")

theStack commented at 6:42 pm on March 30, 2024:

nit: could create the peers as inbound connections instead, here and in other instances:

0        peer_sender = node.add_p2p_connection(P2PInterface())

(it doesn’t really matter though, the code would just be a bit shorter)

glozow commented at 12:57 pm on April 2, 2024:

Changed thanks. Before I added noban, I was using it to make tests run faster.

in src/test/orphanage_tests.cpp:174 in 3842f9a884 outdated

169+    const NodeId node{0};
170+    std::vector<COutPoint> empty_outpoints;
171+
172+    auto parent1 = MakeTransactionSpending(empty_outpoints, det_rand);
173+    auto parent2 = MakeTransactionSpending(empty_outpoints, det_rand);
174+    auto parent3 = MakeTransactionSpending(empty_outpoints, det_rand);

theStack commented at 6:45 pm on March 30, 2024:

nit: parent3 is unused, can remove this line and the two comparisons below

glozow commented at 12:52 pm on April 2, 2024:

fixed

in src/net_processing.cpp:2285 in 97e16c20ca outdated

2240@@ -2228,7 +2241,7 @@ bool PeerManagerImpl::AlreadyHaveTx(const GenTxid& gtxid, bool include_reconside
2241 
2242     if (m_orphanage.HaveTx(gtxid)) return true;
2243 
2244-    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(gtxid.GetHash())) return true;
2245+    if (include_reconsiderable && m_recent_rejects_reconsiderable.contains(hash)) return true;

theStack commented at 6:48 pm on March 30, 2024:

nit: could use hash directly in the previous commit where this line is introduced (edea2269416ad2bcd017e1d6ee9ae30bd225c1e9), for smaller diff

glozow commented at 8:01 am on April 3, 2024:

done

theStack commented at 6:52 pm on March 30, 2024: contributor

Concept ACK

Found some nitty stuff mostly in tests so far, planning to do another review round next week.

glozow force-pushed on Apr 2, 2024

in src/txorphanage.cpp:252 in d3d7dcf004 outdated

247+    LOCK(m_mutex);
248+
249+    // First construct a set of iterators to ensure we do not return duplicates of the same tx.
250+    std::set<OrphanMap::iterator, IteratorComparator> set_child_iterators;
251+
252+    // For each input, get all entries spending this prevout.

instagibbs commented at 9:47 am on April 3, 2024:

0    // For each output, get all entries spending this prevout.

glozow commented at 3:47 pm on April 5, 2024:

done

in src/test/orphanage_tests.cpp:176 in b49b634556 outdated

171+
172+    auto parent1 = MakeTransactionSpending(empty_outpoints, det_rand);
173+    auto parent2 = MakeTransactionSpending(empty_outpoints, det_rand);
174+
175+    // Make sure these parents have different txids otherwise this test won't make sense.
176+    BOOST_CHECK(parent1->GetHash() != parent2->GetHash());

instagibbs commented at 9:59 am on April 3, 2024:

probably better to have it while loop making tx until this is true, rather than fail randomly

glozow commented at 3:46 pm on April 5, 2024:

done

in src/test/orphanage_tests.cpp:233 in b49b634556 outdated

199+    }
200+    auto parent2_children{orphanage.GetChildren(parent2)};
201+    BOOST_CHECK_EQUAL(parent2_children.size(), expected_parent2_children.size());
202+    for (const auto& child : parent2_children) {
203+        BOOST_CHECK(expected_parent2_children.count(child->GetWitnessHash()) > 0);
204+    }

instagibbs commented at 10:04 am on April 3, 2024:

quick test case that returns empty would be good too

glozow commented at 3:46 pm on April 5, 2024:

added

in src/net_processing.cpp:4568 in 53f1e65f30 outdated

4353@@ -4354,7 +4354,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4354         const TxValidationState& state = result.m_state;
4355 
4356         if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
4357-            ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value());
4358+            Assume(result.m_replaced_transactions.has_value());
4359+            std::list<CTransactionRef> empty_replacement_list;
4360+            ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value_or(empty_replacement_list));

murchandamus commented at 6:31 pm on April 3, 2024:

In guard against MempoolAcceptResult::m_replaced_transactions (53f1e65f30a0a6b931e97743113e0227748680df): I am not well-acquainted with net_processing.cpp, but I figured I could still mention that it is unclear to me from the commit message and the code change how this change fits in the context. Were we previously assuming that we would always have a non-empty list for m_replaced_transactions in the context of this call?

theStack commented at 8:16 pm on April 3, 2024:

@murchandamus: I was asking myself the same a few days ago and started with some review notes for each commit. The one for 53f1e65f30a0a6b931e97743113e0227748680df might fit to your question (note that it’s not about empty vs. non-empty, but more about set-to-nothing vs. set-to-something, since it’s an std::optional):

0The only way to create an ATMP result of type `MempoolAcceptResult::ResultType::VALID` is using the
1static method `MempoolAccepptResult::Success`, which in turn calls the private successful case constructor of
2`MempoolAcceptResult`. This one always sets `m_replaced_transactions`, therefore
3`result.m_replaced_transactions.has_value()` in the modified code path should always be true.

(maybe it makes sense to include it in the commit message, if that is correct) The same three lines of code are introduced in PeerManagerImpl::ProcessPackageResult (commit d6df19000118768678c7afd4b9330b4604bb37ce), I assume that this is the reason to also do it on the other place in the code for consistency.

glozow commented at 8:42 pm on April 3, 2024:

it is unclear to me from the commit message and the code change how this change fits in the context.

(Note that this commit is a followup from #29619, I’ve now linked to the comments in the PR description)

Yes, it should always have a value when the result is VALID. This is just adding a belt-and-suspenders juuust in case.

murchandamus commented at 9:01 pm on April 3, 2024:

Thanks! Great, I’ll attempt another more thorough review at a later time.

mzumsande commented at 3:04 pm on April 15, 2024:

I asked myself the same question, found this discussion too late. I also think it would be good to mention this in the commit message (so it becomes clear that it has nothing to do with whether m_replaced_transactions has entries or is empty).

glozow commented at 11:05 am on April 16, 2024:

Elaborated in commit message

in src/test/orphanage_tests.cpp:198 in b49b634556 outdated

193+    std::set<Wtxid> expected_parent2_children{child_p2n1->GetWitnessHash(), child_p1n0_p2n0->GetWitnessHash()};
194+
195+    auto parent1_children{orphanage.GetChildren(parent1)};
196+    BOOST_CHECK_EQUAL(parent1_children.size(), expected_parent1_children.size());
197+    for (const auto& child : parent1_children) {
198+        BOOST_CHECK(expected_parent1_children.count(child->GetWitnessHash()) > 0);

murchandamus commented at 7:28 pm on April 3, 2024:

Nit: Could use std::set::contains here and below

0        BOOST_CHECK(expected_parent1_children.contains(child->GetWitnessHash()));

glozow commented at 3:46 pm on April 5, 2024:

done, yay for 20

murchandamus commented at 7:51 pm on April 3, 2024: contributor

Did a quick pass. I am surprised we are able to get this big of an improvement without p2p changes, seems like a big win. Big Concept ACK, but I must admit this part of the codebase is a bit out of my wheelhouse.

in src/net_processing.cpp:6189 in d6df190001 outdated

6185@@ -6061,7 +6186,9 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
6186                 entry.second.GetHash().ToString(), entry.first);
6187         }
6188         for (const GenTxid& gtxid : requestable) {
6189-            if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) {
6190+            // Skip m_recent_rejects_reconsiderable because we may be requesting a missing parent

instagibbs commented at 9:07 am on April 4, 2024:

I have to admit I’m struggling on these “skip” comments and cases. Every time I read this I have trouble re-deriving the logic.

glozow commented at 3:46 pm on April 5, 2024:

Ok hm. I’ve simplified the wording a bit. Maybe that helps?

instagibbs commented at 4:37 pm on April 5, 2024:

think you forgot to push?

glozow commented at 4:49 pm on April 5, 2024:

think you forgot to push?

Github published everything instead of adding my comment to the group -_- yes, pushing in a second.

instagibbs commented at 9:55 am on April 4, 2024: member

reviewed through 81912ae6649fac5a0d671d2cabe344f787193997

LGTM so far

will review tests

in test/functional/p2p_opportunistic_1p1c.py:377 in c5e196b8e7 outdated

172+        assert low_fee_parent["txid"] in node_mempool
173+        assert high_fee_child["txid"] in node_mempool
174+
175+        node.disconnect_p2ps()
176+
177+    def run_test(self):

instagibbs commented at 10:25 am on April 4, 2024:

add a case where:

CONSENSUS-invalid child is propagated to peer
low_fee parent is given
package evaluation is attempted
peer is disconnected

to ensure we’re handling invalid tx properly

glozow commented at 4:21 pm on April 5, 2024:

Wait oof, we don’t require the orphanage child to be provided by the same peer who sent the low-feerate tx. I think this would mean you can get other people disconnected by sending a bogus child of the package they’re sending.

instagibbs commented at 4:45 pm on April 5, 2024:

looks like peer id needs to be used again: #28970 (review)

glozow commented at 4:57 pm on April 5, 2024:

Hm no, I think we should return pairs of tx and the fromPeer, and just attribute the error to the right peer. If we restrict it to same sender, then we can also block packages by sending the children fast and refusing to send the parent.

glozow commented at 5:17 pm on April 5, 2024:

Last push should fix this with the above approach. Still need to write a test case for the peers being different.

instagibbs commented at 10:27 am on April 4, 2024: member

reviewed through c5e196b8e7b610cc9e6321b76fee4f0c45c1448e

DrahtBot removed the label CI failed on Apr 4, 2024

glozow force-pushed on Apr 5, 2024

DrahtBot added the label CI failed on Apr 6, 2024

glozow force-pushed on Apr 9, 2024

in src/net_processing.cpp:603 in e313001b1b outdated

595@@ -596,6 +596,19 @@ class PeerManagerImpl final : public PeerManager
596     void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& replaced_transactions)
597         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
598 
599+    /** Handle the results of package validation: calls ProcessValidTx and ProcessInvalidTx for
600+     * individual transactions, and caches rejection for the package as a group.
601+     *   */

instagibbs commented at 9:28 am on April 9, 2024:

0     * The senders arg should be populated in same order as individual transactions
1     * in the package_result argument.
2     */

glozow commented at 12:57 pm on April 11, 2024:

added something similar

in src/net_processing.cpp:3224 in e313001b1b outdated

3219+    }
3220+    if (!Assume(!package.empty())) return;
3221+
3222+    // Iterate backwards to erase in-package descendants from the orphanage before they become
3223+    // relevant in AddChildrenToWorkSet.
3224+    for (auto i{package.size() - 1}; i >= 0; --i) {

instagibbs commented at 7:20 am on April 10, 2024:

0net_processing.cpp:3224:40: warning: comparison of unsigned expression in ‘>= 0’ is always true [-Wtype-limits]
1 3224 |     for (auto i{package.size() - 1}; i >= 0; --i) {
2      |

instagibbs commented at 7:27 am on April 10, 2024:

this is also causing the sanitizer run failure

glozow commented at 12:34 pm on April 10, 2024:

Woops yes will fix

in src/policy/packages.cpp:156 in c502734fa5 outdated

151+uint256 GetCombinedHash(const std::vector<Wtxid>& wtxids)
152+{
153+    std::vector<Wtxid> wtxids_copy(wtxids.cbegin(), wtxids.cend());
154+    std::sort(wtxids_copy.begin(), wtxids_copy.end());
155+    return (HashWriter() << wtxids_copy).GetHash();
156+}

instagibbs commented at 10:10 am on April 10, 2024:

newline?

glozow commented at 3:55 pm on April 18, 2024:

(deleted GetCombinedHash)

in src/policy/packages.h:94 in c502734fa5 outdated

87@@ -88,4 +88,10 @@ bool IsChildWithParents(const Package& package);
88  * other (the package is a "tree").
89  */
90 bool IsChildWithParentsTree(const Package& package);
91+
92+/** Get the hash of these wtxids, concatenated in lexicographical order. */
93+uint256 GetCombinedHash(const std::vector<Wtxid>& wtxids);
94+/** Get the hash of these transactions' wtxids, concatenated in lexicographical order. */

instagibbs commented at 10:11 am on April 10, 2024:

newline?

glozow commented at 3:55 pm on April 18, 2024:

(deleted GetCombinedHash)

glozow force-pushed on Apr 11, 2024

glozow commented at 12:58 pm on April 11, 2024: member

Rebased for #29735 and fixed failure.

DrahtBot removed the label CI failed on Apr 11, 2024

glozow commented at 9:55 am on April 15, 2024: member

Update since my last comment #28970 (comment) Running for 20 days:

My node made 5218 package evaluation attempts (x2 = 10,436 transactions). That’s ~260 attempts per day.
Of those attempts, 2260 were successful (x2 = 4520 transactions). That’s ~43% acceptance rate.
Some were repeats. I have 4202 unique txids accepted through package evaluation. I don’t know if they were repeated packages or not.
Of the accepted transactions, 3232 (77%) were later mined in a block.

in src/policy/packages.h:92 in 75855a4d33 outdated

87@@ -88,4 +88,10 @@ bool IsChildWithParents(const Package& package);
88  * other (the package is a "tree").
89  */
90 bool IsChildWithParentsTree(const Package& package);
91+
92+/** Get the hash of these wtxids, concatenated in lexicographical order. */

mzumsande commented at 3:59 pm on April 15, 2024:

I think that they are sorted by internal order, not reversed-byte order (because they are sorted by Wtxid / uint256, not by uint256.GetHex()). Is that on purpose? In any case, maybe it would be useful to specify in the doc which order is used to avoid possbile confusion.

Also, the tests added in this commit only test for relative order between multiple transactions but not if they are actually sorted in lexicographical order, so that could also be done.

glozow commented at 10:49 am on April 16, 2024:

I’ve added more explicit tests and changed this to be ordered based on uint256.GetHex() instead. I don’t know enough to say which sorting is better here, but this seems like the natural ordering when I’m reading the hex strings as a human.

in src/net_processing.cpp:3179 in 3535115973 outdated

3129@@ -3097,7 +3130,14 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx
3130         // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
3131         // for concerns around weakening security of unupgraded nodes
3132         // if we start doing this too early.
3133-        m_recent_rejects.insert(ptx->GetWitnessHash().ToUint256());
3134+        if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) {
3135+            // If the result is TX_RECONSIDERABLE, add it to m_recent_rejects_reconsiderable
3136+            // because we should not download or submit this transaction by itself again, but may
3137+            // submit it as part of a package later.
3138+            m_recent_rejects_reconsiderable.insert(ptx->GetWitnessHash().ToUint256());

mzumsande commented at 5:56 pm on April 15, 2024:

Should update ProcessInvalidTx doc above too (it lists all member variables that the function updates).

glozow commented at 11:06 am on April 16, 2024:

updated

in src/net_processing.cpp:3272 in f5989537e7 outdated

3267+{
3268+    AssertLockNotHeld(m_peer_mutex);
3269+    AssertLockHeld(g_msgproc_mutex);
3270+    AssertLockHeld(cs_main);
3271+
3272+    const auto cpfp_candidates{m_orphanage.GetChildren(ptx)};

mzumsande commented at 6:59 pm on April 15, 2024:

have you considered returning early here if cpfp_candidates is empty? It should work regardless, but seems conceptually simpler than checking all the steps below can handle that case (and may be slightly faster too).

glozow commented at 11:06 am on April 16, 2024:

Done, and edited the logging a bit

in src/net_processing.cpp:3222 in f5989537e7 outdated

3217+    AssertLockHeld(cs_main);
3218+
3219+    if (package_result.m_state.IsInvalid()) {
3220+        m_recent_rejects_reconsiderable.insert(GetPackageHash(package));
3221+    }
3222+    if (!Assume(!package.empty())) return;

mzumsande commented at 7:27 pm on April 15, 2024:

Could we assume more strictly here that the package size is 2 (for now)?

glozow commented at 11:06 am on April 16, 2024:

added

in src/net_processing.cpp:3317 in f5989537e7 outdated

3287+
3288+    for (const auto index : tx_indices) {
3289+        // If we already tried a package and failed for any reason, the combined hash was
3290+        // cached in m_recent_rejects_reconsiderable.
3291+        Package maybe_cpfp_package{ptx, cpfp_candidates.at(index).first};
3292+        if (!m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {

mzumsande commented at 8:15 pm on April 15, 2024:

Why put the package hash into m_recent_rejects_reconsiderable instead of m_recent_rejects? We never reconsider a failed package after all as far as I understand it.

glozow commented at 8:03 am on April 16, 2024:

Yes I think we could use either m_recent_rejects_reconsiderable and m_recent_rejects right now to get the same behavior.

I suppose one mild benefit of using m_recent_rejects_reconsiderable is that our m_recent_rejects bloom filter churns less frequently.

The other benefit is extensibility in the future. In more general ancestor package relay, we could reject a parent+child for being too low feerate, but later accept it as parent+child+grandchild (where the grandchild is very high feerate).

instagibbs commented at 11:36 am on April 16, 2024:

The other benefit is extensibility in the future. In more general ancestor package relay, we could reject a parent+child for being too low feerate, but later accept it as parent+child+grandchild (where the grandchild is very high feerate).

Perhaps this doesn’t matter but I’m not sure I understand the distinction here. We need the combined hash committed somewhere in a bloom filter to not fetch the same ancestor package again. If it’s different at all, we’ll fetch it regardless of which filter we add it to?

glozow commented at 12:25 pm on April 16, 2024:

Hm I think you’re right, it wouldn’t make a difference with downloads. Crossing that part out. Were we thinking of this within validation maybe? Linearize + chunk the package, see that a chunk has already been rejected as too low feerate, drop it?

in src/net_processing.cpp:3288 in f5989537e7 outdated

3276+
3277+    CTransactionRef tx_orphan{nullptr};
3278+    NodeId orphan_sender{-1};
3279+    Assume(m_recent_rejects_reconsiderable.contains(parent_wtxid.ToUint256()));
3280+
3281+    // Create a 1p1c package using the first child that hasn't already been rejected. Sort

mzumsande commented at 8:37 pm on April 15, 2024:

Why do we only try once in the case there are multiple children in the orphanage, instead of trying multiple times until one package succeeds? To avoid some kind of spamming attacks that could exhaust our computing power?

glozow commented at 7:51 am on April 16, 2024:

Yes exactly, this is to bound computation (imagine if somebody sent us 100 fake orphans descended from 1 transaction and we processed them all here). My idea was to do something similar to regular orphan processing, where we have a work queue and limit to 1 item per ProcessMessages.

There is no work queue here, though, and we drop the parent as soon as we try 1 (pass or fail). At coredev, we discussed adding a work queue for 1p1c as well. However, since it involves finding a way to store the low feerate parent, and we have plenty of low hanging fruit for improving orphan handling, we’ll save that for a later PR.

mzumsande commented at 9:02 pm on April 15, 2024: contributor

Started reviewing - haven’t looked at the tests yet.

guard against MempoolAcceptResult::m_replaced_transactions

It should never be a nullopt when the transaction result is valid -
Assume() this is the case. However, as a belt-and-suspenders just in
case it is nullopt, use an empty list.

6f4da19cc3

[doc] restore comment about why we check if ptx HasWitness before caching rejected txid c3c1e15831

glozow force-pushed on Apr 16, 2024

glozow commented at 11:09 am on April 16, 2024: member

thanks for the review @mzumsande!

in test/functional/p2p_opportunistic_1p1c.py:201 in 142d584dab outdated

196+
197+        # 2. Node requests the missing parent by txid.
198+        parent_txid_int = int(low_fee_parent["txid"], 16)
199+        peer_sender.wait_for_getdata([parent_txid_int])
200+
201+        # 3. Sender relays the parent. Parent+Child are evaluated as a package and accepted.

instagibbs commented at 5:25 pm on April 16, 2024:

0        # 3. Sender relays the parent. Parent+Child are evaluated as a package and rejected.

in test/functional/p2p_opportunistic_1p1c.py:189 in 142d584dab outdated

184+        hex_orphan_no_sig = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["value"] - Decimal("0.0001")})
185+        tx_orphan_bad_wit = tx_from_hex(hex_orphan_no_sig)
186+        tx_orphan_bad_wit.wit.vtxinwit.append(CTxInWitness())
187+        tx_orphan_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
188+
189+        peer_sender = node.add_p2p_connection(P2PInterface())

instagibbs commented at 5:31 pm on April 16, 2024:

would be good to explicitly test that the parent-giver isn’t punished in this scenario, and test if parent is consensus-bad it results in something expected.

  0diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
  1index 603cbf08a9..1c887289dc 100755
  2--- a/test/functional/p2p_opportunistic_1p1c.py
  3+++ b/test/functional/p2p_opportunistic_1p1c.py
  4@@ -164,78 +164,129 @@ class PackageRelayTest(BitcoinTestFramework):
  5         # itself and with a child. This is necessary, otherwise high_fee_child can be censored.
  6         parent_txid_int = int(low_fee_parent["txid"], 16)
  7         peer_sender.wait_for_getdata([parent_txid_int])
  8 
  9         # 7. The low feerate parent + high feerate child are submitted as a package.
 10         peer_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))
 11 
 12         # 8. Both transactions should now be in mempool
 13         node_mempool = node.getrawmempool()
 14         assert low_fee_parent["txid"] in node_mempool
 15         assert high_fee_child["txid"] in node_mempool
 16 
 17         node.disconnect_p2ps()
 18 
 19     def test_orphan_consensus_failure(self):
 20         node = self.nodes[0]
 21         low_fee_parent = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
 22         coin = low_fee_parent["new_utxo"]
 23         address = node.get_deterministic_priv_key().address
 24         # Create raw transaction spending the parent, but with no signature (a consensus error).
 25         hex_orphan_no_sig = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["value"] - Decimal("0.0001")})
 26         tx_orphan_bad_wit = tx_from_hex(hex_orphan_no_sig)
 27         tx_orphan_bad_wit.wit.vtxinwit.append(CTxInWitness())
 28         tx_orphan_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
 29 
 30-        peer_sender = node.add_p2p_connection(P2PInterface())
 31+        child_peer_sender = node.add_p2p_connection(P2PInterface())
 32+        parent_peer_sender = node.add_p2p_connection(P2PInterface())
 33 
 34         # 1. Child is received first. It is missing an input.
 35         child_wtxid_int = int(tx_orphan_bad_wit.getwtxid(), 16)
 36-        peer_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=child_wtxid_int)]))
 37-        peer_sender.wait_for_getdata([child_wtxid_int])
 38-        peer_sender.send_and_ping(msg_tx(tx_orphan_bad_wit))
 39+        child_peer_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=child_wtxid_int)]))
 40+        child_peer_sender.wait_for_getdata([child_wtxid_int])
 41+        child_peer_sender.send_and_ping(msg_tx(tx_orphan_bad_wit))
 42 
 43         # 2. Node requests the missing parent by txid.
 44         parent_txid_int = int(low_fee_parent["txid"], 16)
 45-        peer_sender.wait_for_getdata([parent_txid_int])
 46+        child_peer_sender.wait_for_getdata([parent_txid_int])
 47 
 48-        # 3. Sender relays the parent. Parent+Child are evaluated as a package and accepted.
 49-        peer_sender.send_message(msg_tx(low_fee_parent["tx"]))
 50+        # 3. "Honest" sender relays the parent. Parent+Child are evaluated as a package and accepted.
 51+        parent_peer_sender.send_message(msg_tx(low_fee_parent["tx"]))
 52 
 53         # 4. Transactions should not be in mempool.
 54         node_mempool = node.getrawmempool()
 55         assert low_fee_parent["txid"] not in node_mempool
 56         assert tx_orphan_bad_wit.rehash() not in node_mempool
 57 
 58         # 5. Peer sent a consensus-invalid transaction.
 59-        peer_sender.wait_for_disconnect()
 60+        child_peer_sender.wait_for_disconnect()
 61+
 62+        # 6. "Honest" peer unpunished
 63+        parent_peer_sender.sync_with_ping()
 64+
 65+    def test_parent_consensus_failure(self):
 66+        node = self.nodes[0]
 67+        low_fee_parent = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
 68+
 69+        # Add bad sigature to parent
 70+        tx_parent_bad_wit = tx_from_hex(low_fee_parent["hex"])
 71+        tx_parent_bad_wit.wit.vtxinwit.append(CTxInWitness())
 72+        tx_parent_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
 73+
 74+        coin = low_fee_parent["new_utxo"]
 75+        address = node.get_deterministic_priv_key().address
 76+        # Create raw transaction spending the parent, but with no signature (a consensus error).
 77+        hex_orphan_no_sig = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["value"] - Decimal("0.0001")})
 78+        tx_orphan_bad_wit = tx_from_hex(hex_orphan_no_sig)
 79+        tx_orphan_bad_wit.wit.vtxinwit.append(CTxInWitness())
 80+        tx_orphan_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
 81+
 82+        child_peer_sender = node.add_p2p_connection(P2PInterface())
 83+        parent_peer_sender = node.add_p2p_connection(P2PInterface())
 84+
 85+        # 1. Child is received first. It is missing an input.
 86+        child_wtxid_int = int(tx_orphan_bad_wit.getwtxid(), 16)
 87+        child_peer_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=child_wtxid_int)]))
 88+        child_peer_sender.wait_for_getdata([child_wtxid_int])
 89+        child_peer_sender.send_and_ping(msg_tx(tx_orphan_bad_wit))
 90+
 91+        # 2. Node requests the missing parent by txid.
 92+        parent_txid_int = int(tx_parent_bad_wit.rehash(), 16)
 93+        child_peer_sender.wait_for_getdata([parent_txid_int])
 94+
 95+        # 3. Parent sender relays the parent. Only parent should be evaluated.
 96+        parent_peer_sender.send_message(msg_tx(tx_parent_bad_wit))
 97+
 98+        # 4. Transactions should not be in mempool.
 99+        node_mempool = node.getrawmempool()
100+        assert tx_parent_bad_wit.rehash() not in node_mempool
101+        assert tx_orphan_bad_wit.rehash() not in node_mempool
102+
103+        # 5. Peer sent a consensus-invalid transaction.
104+        parent_peer_sender.wait_for_disconnect()
105+
106+        # 6. Child-sending peer unpunished for now!
107+        child_peer_sender.sync_with_ping()
108 
109     def run_test(self):
110         node = self.nodes[0]
111         self.wallet = MiniWallet(node)
112         self.wallet_nonsegwit = MiniWallet(node, mode=MiniWalletMode.RAW_P2PK)
113         self.generate(self.wallet_nonsegwit, 10)
114         self.generate(self.wallet, 20)
115 
116         fill_mempool(self, node, self.wallet)
117 
118         self.log.info("Check opportunistic 1p1c logic when parent (txid != wtxid) is received before child")
119         self.test_basic_parent_then_child(self.wallet)
120 
121         self.log.info("Check opportunistic 1p1c logic when parent (txid == wtxid) is received before child")
122         self.test_basic_parent_then_child(self.wallet_nonsegwit)
123 
124         self.log.info("Check opportunistic 1p1c logic when child is received before parent")
125         self.test_basic_child_then_parent()
126 
127         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid != wtxid)")
128         self.test_low_and_high_child(self.wallet)
129 
130         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid == wtxid)")
131         self.test_low_and_high_child(self.wallet_nonsegwit)
132 
133         self.log.info("Check opportunistic 1p1c logic with consensus-invalid orphan causes disconnect")
134         self.test_orphan_consensus_failure()
135 
136+        self.log.info("Check opportunistic 1p1c logic doesn't evaluate package with consensus-invalid parent")
137+        self.test_parent_consensus_failure()
138+
139 
140 if __name__ == '__main__':
141     PackageRelayTest().main()

glozow commented at 12:24 pm on April 19, 2024:

Added this test and tweaked a bit to check that orphan stays + can still be resolved.

in test/functional/p2p_opportunistic_1p1c.py:115 in 142d584dab outdated

110+        node.disconnect_p2ps()
111+
112+    def test_low_and_high_child(self, wallet):
113+        node = self.nodes[0]
114+        low_fee_parent = wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
115+        low_fee_child = wallet.create_self_transfer(utxo_to_spend=low_fee_parent["new_utxo"], fee_rate=2*FEERATE_1SAT_VB)

instagibbs commented at 6:06 pm on April 16, 2024:

suggestion: call this med_fee_child and use the mempoolminfee directly since that’s a more meaningful value than 2*FEERATE_1SAT_VB which is below the floating minfee

glozow commented at 12:24 pm on April 19, 2024:

done

in src/test/txpackage_tests.cpp:66 in 54d0c78d10 outdated

61+    Wtxid wtxid_6{WtxidFromString("0xe065bac15f62bb4e761d761db928ddee65a47296b2b776785abb912cdec474e3")};
62+
63+    BOOST_CHECK(wtxid_0.GetHex() < wtxid_1.GetHex());
64+    BOOST_CHECK(wtxid_1.GetHex() < wtxid_2.GetHex());
65+    BOOST_CHECK(wtxid_2.GetHex() < wtxid_3.GetHex());
66+    BOOST_CHECK(wtxid_1.GetHex() < wtxid_3.GetHex());

sr-gi commented at 7:51 pm on April 16, 2024:

In: 54d0c78d104fb5412a194816590a06cad8cadf80

This seems redundant

glozow commented at 3:54 pm on April 18, 2024:

deleted

in src/net_processing.cpp:4357 in 6f4da19cc3 outdated

4353@@ -4354,7 +4354,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4354         const TxValidationState& state = result.m_state;
4355 
4356         if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) {
4357-            ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value());
4358+            Assume(result.m_replaced_transactions.has_value());

sdaftuar commented at 12:42 pm on April 17, 2024:

This is a code style nit, but I think I agree with https://github.com/bitcoin/bitcoin/pull/21062/files#r571975710 that there’s not much benefit from using a std::optional on m_replaced_transactions? It just seems to lead to extra code around understanding when the field is set or not, when I think it would be simpler to say that it’s just a list which is either empty or non-empty based on whether a replacement took place.

Anyway I have no objection to the changes in this commit; just wanted to mention it so that if others agree, then perhaps this is a change we could make in the future.

glozow commented at 10:04 am on April 23, 2024:

I definitely agree it’s awkward and doesn’t have much benefit. I’ll leave the assume/handling here, and maybe in a followup we can change it to non-optional.

glozow commented at 12:43 pm on May 1, 2024:

Done in #30012

in src/net_processing.cpp:3337 in 25c712711d outdated

3288+    // Create a 1p1c package using the first child that hasn't already been rejected. Sort
3289+    // the children in random order to not create a bias that attackers can use to delay
3290+    // package acceptance. Create a random permutation of the indices.
3291+    std::vector<size_t> tx_indices(cpfp_candidates.size());
3292+    std::iota(tx_indices.begin(), tx_indices.end(), 0);
3293+    Shuffle(tx_indices.begin(), tx_indices.end(), m_rng);

mzumsande commented at 1:20 pm on April 17, 2024:

would it make sense to match peers and (if possible) always pick a child from the orphanage that was sent to us by the same peer that sent us the parent (instead of a random one)? That way, it wouldn’t be possible that a third peer could send us multiple low-fee children that we’d store in the orphanage, in the hope that we pick one of those and reject the package.

instagibbs commented at 1:37 pm on April 17, 2024:

I think I made this point in person from the philosophical standpoint that orphanage-churning aside, we probably shouldn’t allow peers to “cross-talk” when it comes to package evaluation and possible punishment.

In practice I think an adversary can just churn the orphanage until Future Work happens, but still might be a good conceptual framework to adhere to?

glozow commented at 1:57 pm on April 17, 2024:

great idea, will change 👍

glozow commented at 3:56 pm on April 18, 2024:

Did this now. We first gather all children by the same peer and try by recency order. If we cannot find one that matches, we look for children not by this peer, and try in random order.

instagibbs commented at 4:32 pm on April 18, 2024:

For some code deduplication you could just have GetChildrenFromPeer which takes std::optional<NodeId>, returning full std::vector<std::pair<CTransactionRef, NodeId>> list if std::nullopt or filtered to the specific nodeid otherwise. You can call it filtered once, then if a suitable orphan isn’t found, call it again unfiltered.

glozow commented at 1:53 pm on April 24, 2024:

I don’t think that is much simpler. Also I think this is easier to delete if/when we don’t try orphans from different peers in the future.

in src/test/txpackage_tests.cpp:77 in 142d584dab outdated

72+    DataStream stream4{
73+        ParseHex("02000000000101964b8aa63509579ca6086e6012eeaa4c2f4dd1e283da29b67c8eea38b3c6fd220000000000fdffffff0294c618000000000017a9145afbbb42f4e83312666d0697f9e66259912ecde38768fa2c0000000000160014897388a0889390fd0e153a22bb2cf9d8f019faf50247304402200547406380719f84d68cf4e96cc3e4a1688309ef475b150be2b471c70ea562aa02206d255f5acc40fd95981874d77201d2eb07883657ce1c796513f32b6079545cdf0121023ae77335cefcb5ab4c1dc1fb0d2acfece184e593727d7d5906c78e564c7c11d125cf0c00"),
74+    };
75+    CTransaction tx4(deserialize, TX_WITH_WITNESS, stream4);
76+    CTransactionRef ptx4{MakeTransactionRef(tx4)};
77+    Txid txid_4{TxidFromString("0xbd0f71c1d5e50589063e134fad22053cdae5ab2320db5bf5e540198b0b5a4e69")};

sr-gi commented at 2:55 pm on April 17, 2024:

In: 54d0c78d104fb5412a194816590a06cad8cadf80

I don’t see what the point of building the txid from a literal in this way and comparing it to the one obtained via GetHash() is. I’m guessing you’re trying to make the point that the txids are actually what you are claiming them to be (as opposed to just writing them in a comment), so the reader can manually check the difference between internal and human-readable lexicographic ordering. Is that really necessary?

glozow commented at 3:54 pm on April 18, 2024:

I figure it’s easier to read the test this way; you can easily tell that the lexicographical ordering is what I claim it to be

in src/test/txpackage_tests.cpp:123 in 54d0c78d10 outdated

118+    std::vector<CTransactionRef> package_564{ptx5, ptx6, ptx4};
119+    std::vector<CTransactionRef> package_546{ptx5, ptx4, ptx6};
120+    std::vector<CTransactionRef> package_645{ptx6, ptx4, ptx5};
121+    std::vector<CTransactionRef> package_654{ptx6, ptx5, ptx4};
122+    // All of them must have the same package hash 2dc93431aa6eb415a4e0995f77c78098d7ed97ca6f123a0d77a7747339525acd
123+    std::vector<uint256> expected_order_txns{wtxid_4.ToUint256(), wtxid_5.ToUint256(), wtxid_6.ToUint256()};

sr-gi commented at 3:55 pm on April 17, 2024:

In: 54d0c78d104fb5412a194816590a06cad8cadf80

Calling ToUint256() shouldn’t be needed

glozow commented at 3:48 pm on April 18, 2024:

done, thanks

in src/test/txpackage_tests.cpp:137 in 54d0c78d10 outdated

132+    std::vector<Wtxid> wtxids_546{wtxid_5, wtxid_4, wtxid_6};
133+    BOOST_CHECK_EQUAL(calculated_hash_456, GetCombinedHash(wtxids_546));
134+    std::vector<Wtxid> wtxids_654{wtxid_6, wtxid_5, wtxid_4};
135+    BOOST_CHECK_EQUAL(calculated_hash_456, GetCombinedHash(wtxids_654));
136+
137+    std::vector<uint256> expected_order_wtxids{wtxid_1.ToUint256(), wtxid_2.ToUint256(), wtxid_3.ToUint256(),

sr-gi commented at 3:56 pm on April 17, 2024:

In: 54d0c78d104fb5412a194816590a06cad8cadf80

Same as before, no need to cast to ToUint256

glozow commented at 3:48 pm on April 18, 2024:

done, thanks

in src/test/txpackage_tests.cpp:113 in 54d0c78d10 outdated

108+    BOOST_CHECK(txid_4.ToUint256() != wtxid_4.ToUint256());
109+    BOOST_CHECK(txid_6.ToUint256() != wtxid_6.ToUint256());
110+
111+    // Testing that package hash is sorting by wtxids, not txids.
112+    // For tx4 and tx5, txid order != wtxid order
113+    BOOST_CHECK(txid_5.GetHex() < txid_4.GetHex());

sr-gi commented at 4:06 pm on April 17, 2024:

In: 54d0c78d104fb5412a194816590a06cad8cadf80

I’m struggling to see the usefulness of this. You are showing that, for the provided transactions, the ordering may be different based on the representation used (wtxid/txid/ToUint256/GetHex), but I don’t think this clearly shows that the package hash is using one or the other.

You are already proving that the order is the one you are expecting by manually computing calculated_hash_456. You could also create different orderings based on ToUint256 (instead of GetHex) and txid (instead of wtxid) and check how those are not equal to calculated_hash_456 (after having checked that all of the permutations of GetPackageHash are equal to calculated_hash_456)

glozow commented at 3:53 pm on April 18, 2024:

Ok added tests calculating what the hashes would be if we used another order + checking they’re not the same

in src/test/txpackage_tests.cpp:299 in 54d0c78d10 outdated

295@@ -190,17 +296,28 @@ BOOST_FIXTURE_TEST_CASE(noncontextual_package_tests, TestChain100Setup)
296         BOOST_CHECK_EQUAL(state.GetRejectReason(), "package-not-sorted");
297         BOOST_CHECK(IsChildWithParents({tx_parent, tx_child}));
298         BOOST_CHECK(IsChildWithParentsTree({tx_parent, tx_child}));
299+        BOOST_CHECK_EQUAL(GetPackageHash({tx_child}), GetCombinedHash({tx_child->GetWitnessHash()}));

sr-gi commented at 5:23 pm on April 17, 2024:

In: 54d0c78d104fb5412a194816590a06cad8cadf80

nit: I think it wouldn’t hurt to have a comment here along the lines of:

0/// Check that `GetPackageHash`/ `GetCombinedHash` are consistent with each other, and that the input order does not affect the resulting hash

glozow commented at 3:48 pm on April 18, 2024:

(deleted GetCombinedHash)

in src/test/orphanage_tests.cpp:195 in 91f4efa420 outdated

190+    BOOST_CHECK(orphanage.AddTx(child_p1n0_p1n1, node));
191+    BOOST_CHECK(orphanage.AddTx(child_p1n0_p2n0, node));
192+
193+    // Check that GetChildren returns what is expected.
194+    std::set<Wtxid> expected_parent1_children{child_p1n0->GetWitnessHash(), child_p1n0_p2n0->GetWitnessHash(), child_p1n0_p1n1->GetWitnessHash()};
195+    std::set<Wtxid> expected_parent2_children{child_p2n1->GetWitnessHash(), child_p1n0_p2n0->GetWitnessHash()};

sr-gi commented at 8:08 pm on April 17, 2024:

in 91f4efa420958a93f4620379f8830231f276b23b

This should also be comparable by CTransactionRef, shouldn’t it? So GetWitnessHash doesn’t need to be called here and in following loops

glozow commented at 3:54 pm on April 18, 2024:

done

in src/test/orphanage_tests.cpp:205 in 91f4efa420 outdated

200+        BOOST_CHECK(expected_parent1_children.contains(child->GetWitnessHash()));
201+    }
202+    auto parent2_children{orphanage.GetChildren(parent2)};
203+    BOOST_CHECK_EQUAL(parent2_children.size(), expected_parent2_children.size());
204+    for (const auto& [child, peer] : parent2_children) {
205+        BOOST_CHECK(expected_parent2_children.count(child->GetWitnessHash()) > 0);

sr-gi commented at 8:14 pm on April 17, 2024:

in: 91f4efa420958a93f4620379f8830231f276b23b

nit: Use contains instead of count(...) > 0 for consistency with the previous check (previous loop)

glozow commented at 3:54 pm on April 18, 2024:

done

sr-gi commented at 8:17 pm on April 17, 2024: member

First pass. Reviewed up to 91f4efa420958a93f4620379f8830231f276b23b

in test/functional/p2p_1p1c_network.py:111 in 142d584dab outdated

106+        for i in range(self.num_nodes):
107+            peer = self.peers[i]
108+            for tx in transactions_to_presend[i]:
109+                inv = CInv(t=MSG_WTX, h=int(tx.getwtxid(), 16))
110+                peer.send_and_ping(msg_inv([inv]))
111+                peer.wait_for_getdata([int(tx.getwtxid(), 16)])

theStack commented at 7:25 am on April 18, 2024:

nit, feel free to ignore: strictly speaking those 3 lines are not needed, as sending in the tx unsolicitedly (without prior inv/getdata) works as well, though not adhering to the typical protocol flow.

glozow commented at 3:48 pm on April 18, 2024:

removed

glozow force-pushed on Apr 18, 2024

glozow commented at 4:02 pm on April 18, 2024: member

Main changes:

Try same-peer orphanage children (most recent first) before different-peer ones (randomized). Replaced GetChildren with GetChildrenFromSamePeer and GetChildrenFromDifferentPeer.
Deleted GetCombinedHash because it’s unused. Also deleted the tests for it
Changed GetPackageHash a bit. Using single-sha256 instead of double-sha256 and using just wtxids instead of serialized vector

glozow force-pushed on Apr 18, 2024

DrahtBot commented at 4:29 pm on April 18, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/23986234746

DrahtBot added the label CI failed on Apr 18, 2024

glozow commented at 9:40 am on April 19, 2024: member

debugging the p2p_opportunistic_1p1c.py failure. I think the wallet one is unrelated.

glozow force-pushed on Apr 19, 2024

DrahtBot removed the label CI failed on Apr 19, 2024

glozow commented at 12:32 pm on April 19, 2024: member

Ok ready for review again.

in test/functional/p2p_opportunistic_1p1c.py:56 in a053911cb9 outdated

51+            "-datacarriersize=100000",
52+            "-maxmempool=5",
53+        ]]
54+        self.supports_cli = False
55+
56+    def subest_check(self):

instagibbs commented at 2:08 pm on April 19, 2024:

duplicated cleanup, or something?

glozow commented at 2:18 pm on April 19, 2024:

Ah forgot to delete, yes

glozow commented at 11:57 am on April 22, 2024:

deleted

in test/functional/p2p_opportunistic_1p1c.py:278 in a053911cb9 outdated

273+        assert high_fee_child["txid"] not in node_mempool
274+
275+        # 5. Peer sent a consensus-invalid transaction.
276+        fake_parent_sender.wait_for_disconnect()
277+
278+        self.log.info("Check opportunistic 1p1c logic with consensus-invalid parent causes disconnect of the correct peer")

instagibbs commented at 2:56 pm on April 19, 2024:

duplicate log?

glozow commented at 11:58 am on April 22, 2024:

Ah that was supposed to be another log, should be fixed now

Fabcien referenced this in commit 67c0d93982 on Apr 19, 2024

in src/test/txpackage_tests.cpp:85 in 5c8aa65764 outdated

80+    BOOST_CHECK_EQUAL(tx_2.GetWitnessHash(), wtxid_2);
81+    BOOST_CHECK_EQUAL(tx_3.GetWitnessHash(), wtxid_3);
82+
83+    BOOST_CHECK(wtxid_1.GetHex() < wtxid_2.GetHex());
84+    BOOST_CHECK(wtxid_2.GetHex() < wtxid_3.GetHex());
85+    BOOST_CHECK(wtxid_1.GetHex() < wtxid_3.GetHex());

sr-gi commented at 4:18 pm on April 20, 2024:

In 5c8aa657642ef24e711a73c28278644f14117d73

This last check is still redundant

glozow commented at 11:44 am on April 22, 2024:

deleted

in src/txorphanage.cpp:286 in ca9f03f220 outdated

281+std::vector<std::pair<CTransactionRef, NodeId>> TxOrphanage::GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const
282+{
283+    LOCK(m_mutex);
284+
285+    // First construct vector of iterators to ensure we do not return duplicates of the same tx.
286+    std::vector<OrphanMap::iterator> unique_iters;

sr-gi commented at 4:56 pm on April 20, 2024:

In ca9f03f2207854c849a14d7af2fcd91a5f675e14

Wouldn’t it be easier (and simpler) to use a set here instead of a vector (as used to be the case in 9dc967195c4965973be0174ae6041be70a886c7a). It makes sense to use a vec in GetChildrenFromSamePeer given you are sorting based on a custom order, but here a set should be equivalent and involve less boilerplate (?)

glozow commented at 11:39 am on April 22, 2024:

I copied this from the way we do unique_parents in orphan parent requests (see the discussion on #19596 about the dynamic memory usage and speed). However I don’t mind either way and agree a std::set would be simpler, so happy to change if people prefer using a set.

in src/txorphanage.cpp:251 in ca9f03f220 outdated

246+{
247+    LOCK(m_mutex);
248+
249+    // First construct a vector of iterators to ensure we do not return duplicates of the same tx
250+    // and so we can sort by nTimeExpire.
251+    std::vector<OrphanMap::iterator> unique_iters;

sr-gi commented at 5:10 pm on April 20, 2024:

In ca9f03f2207854c849a14d7af2fcd91a5f675e14

I think this is misleading (both the variable name and the comment).

The reason a vector is chosen is so you can sort based on nTimeExpire. Removing the duplicates comes later (for most of the function, the iters may not be unique).

For the sake of future readers it may be worth changing it

glozow commented at 11:56 am on April 22, 2024:

I think it’s pretty common to name a temporary data structure based on its intended result. But ok, I’ve changed it to iters now.

in src/test/orphanage_tests.cpp:200 in 42859548ab outdated

195+        BOOST_CHECK(orphanage.AddTx(child_p1n0_p2n0, node1));
196+
197+        std::set<CTransactionRef> expected_parent1_children{child_p1n0, child_p1n0_p2n0, child_p1n0_p1n1};
198+        std::set<CTransactionRef> expected_parent2_children{child_p2n1, child_p1n0_p2n0};
199+
200+        // Check contents and ensure transactions are returned in order of recency.

sr-gi commented at 5:43 pm on April 20, 2024:

In 42859548ab5aebf40da6089b85065f7c204b992a

I don’t think the order of recency is being tested

glozow commented at 11:40 am on April 22, 2024:

Ah, forgot to remove the comment. We need to make time parameterizable across TxOrphanage members to test order, so not testing / saving for followup.

glozow commented at 11:44 am on April 22, 2024:

deleted

in src/test/orphanage_tests.cpp:203 in 42859548ab outdated

198+        std::set<CTransactionRef> expected_parent2_children{child_p2n1, child_p1n0_p2n0};
199+
200+        // Check contents and ensure transactions are returned in order of recency.
201+        const auto parent1_children_from_sender{orphanage.GetChildrenFromSamePeer(parent1, node1)};
202+        for (const auto& tx : parent1_children_from_sender) {
203+            BOOST_CHECK(expected_parent1_children.contains(tx));

sr-gi commented at 5:50 pm on April 20, 2024:

In 42859548ab5aebf40da6089b85065f7c204b992a

To make sure the two collections are equal you also need to check the their sizes match. This applies to the four cases.

glozow commented at 11:44 am on April 22, 2024:

done

sr-gi commented at 6:01 pm on April 20, 2024: member

Second pass (up to the same height: 42859548ab5aebf40da6089b85065f7c204b992a)

Left some additional comments on the updated approach.

glozow force-pushed on Apr 22, 2024

in src/test/orphanage_tests.cpp:225 in 507f5ea12a outdated

206+        BOOST_CHECK_EQUAL(parent2_children_from_sender.size(), expected_parent2_children.size());
207+        for (const auto& tx : parent2_children_from_sender) {
208+            BOOST_CHECK(expected_parent2_children.contains(tx));
209+        }
210+        // The peer must match
211+        BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent1, node2).empty());

instagibbs commented at 4:39 pm on April 22, 2024:

nit

0        BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent1, node2).empty());
1        BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent2, node2).empty());

glozow commented at 9:57 am on April 23, 2024:

Done. Also refactored orphanage_tests to be more readable.

in src/test/orphanage_tests.cpp:226 in 507f5ea12a outdated

221+        for (const auto& [tx, peer] : parent2_children_not_other) {
222+            BOOST_CHECK(expected_parent2_children.contains(tx));
223+            BOOST_CHECK_EQUAL(peer, node1);
224+        }
225+
226+        // There shouldn't be any children of this tx

instagibbs commented at 4:41 pm on April 22, 2024:

nit

0        // There shouldn't be any children of this tx in orphanage

instagibbs commented at 5:01 pm on April 22, 2024: member

reviewed through 7d220c6a5c0e0c5e8cfe79ebd2eae6e845d1d983

tested and confirmed fuzz coverage is hitting meaningful GetChildrenFrom* results

continuing longer range testing

in src/net_processing.cpp:4617 in 668313e00d outdated

4585@@ -4432,7 +4586,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4586                     // protocol for getting all unconfirmed parents.
4587                     const auto gtxid{GenTxid::Txid(parent_txid)};
4588                     AddKnownTx(*peer, parent_txid);
4589-                    if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/true)) AddTxAnnouncement(pfrom, gtxid, current_time);
4590+                    // Exclude m_recent_rejects_reconsiderable: the missing parent may have been
4591+                    // previously rejected for being too low feerate. This orphan might CPFP it.
4592+                    if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) AddTxAnnouncement(pfrom, gtxid, current_time);

sdaftuar commented at 5:23 pm on April 22, 2024:

Let’s say we’ve got a transaction T that is missing inputs. We loop through the missing parents and see that none are in m_recent_rejects, so we drop into this block of code.

If there’s more than 1 missing parent in m_recent_rejects_reconsiderable, is there any benefit to fetching them? It seems like we could tell in advance that validation would fail in that circumstance, because we only try 1P1C packages.

I don’t know if it’s worth additional complexity to deal with this case, but just wanted to flag the potential bandwidth waste if this were a common pattern.

glozow commented at 12:11 pm on April 23, 2024:

Good point, I don’t think this is too complex so I’ve added it.

While working on this, I remembered/realized that if there are any other unconfirmed parents at all, we will reject it because the package must be child-with-unconfirmed-parents, enforced here: https://github.com/bitcoin/bitcoin/blob/256e1703197fdddd78bc6d659431cd0fc3b63cde/src/validation.cpp#L1558-L1564

IIRC this was the way to check that a package was “2 generations only”. But I don’t know how useful this is, and it’s quite annoying here, so perhaps we should consider getting rid of this restriction in a followup… Then we can accept packages where the child has parents already in mempool.

in src/test/fuzz/txorphan.cpp:164 in b9caa4cfcb outdated

155@@ -136,6 +156,12 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
156                     orphanage.LimitOrphans(limit, limit_orphans_rng);
157                     Assert(orphanage.Size() <= limit);
158                 });
159+
160+        // Set tx as potential parent to be used for future GetChildren() calls.
161+        if (!ptx_potential_parent || fuzzed_data_provider.ConsumeBool()) {
162+            ptx_potential_parent = tx;
163+        }

sr-gi commented at 7:23 pm on April 22, 2024:

In b9caa4cfcbd1e176ab3ecd61973ab6721570aecb

I’m not too familiar with fuzzing, but wouldn’t it be better if this be set unconditionally? This will trigger on the first iteration of the loop, and then based on a coin flip AFAICT, which means that if the variable is not overwritten, we will call orphanage.AddChildrenToWorkSet(*ptx_potential_parent); with the same transaction multiple times. This is harmless, given the internals just add data to a set, so multiple calls won’t change it, but it is redundant (and so are the fors that will trigger latter).

sr-gi commented at 3:24 pm on April 23, 2024:

At the very least, if this is kept as is, the assignment should be moved to the outer context. Right now it is part of LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10 * DEFAULT_MAX_ORPHAN_TRANSACTIONS), which means that it would potentially be set to the same value over and over again

glozow commented at 9:23 am on April 26, 2024:

Thanks, I’ve moved it to the outer loop. Kept bool, seems harmless and I imagine we can find some more interesting code paths by sometimes not setting it immediately.

in src/net_processing.cpp:3315 in 668313e00d outdated

3283+    const auto cpfp_candidates_same_peer{m_orphanage.GetChildrenFromSamePeer(ptx, nodeid)};
3284+
3285+    // These children should be sorted from most newest to oldest. In the case of children that
3286+    // replace each other, this helps us accept the highest feerate (most recent) one most
3287+    // efficiently.
3288+    for (const auto& child : cpfp_candidates_same_peer) {

sdaftuar commented at 10:27 pm on April 22, 2024:

Just an observation: I guess the more common case will likely be multiple children of the same parent transaction (rather than conflicting children). Still, we have no idea which child will have the best chance of successfully bumping it, so this ordering seems as good as any.

glozow commented at 9:59 am on April 23, 2024:

Edited the comment to say this is a probably uncommon case

sdaftuar approved

sdaftuar commented at 10:38 pm on April 22, 2024: member

Code review ACK (apart from the tests, which I only skimmed). Will test…

glozow force-pushed on Apr 23, 2024

in src/net_processing.cpp:3335 in bb3bf0a39c outdated

3330+
3331+    if (tx_orphan) {
3332+        const Package package_1p1c{ptx, tx_orphan};
3333+        const std::vector<NodeId> senders{nodeid, orphan_sender};
3334+        const auto package_result{ProcessNewPackage(m_chainman.ActiveChainstate(), m_mempool, package_1p1c, /*test_accept=*/false, /*client_maxfeerate=*/std::nullopt)};
3335+        LogDebug(BCLog::TXPACKAGES, "package evaluation for parent %s (wtxid=%s) + child %s (wtxid=%s) in orphanage: %s\n",

instagibbs commented at 12:26 pm on April 23, 2024:

FWIW: I added logging here to see what number of parents and given by different peers than the orphan. After a day of running it’s 0 out of 162.

Is there a reason to think this should be a common pattern?

sdaftuar commented at 12:31 pm on April 23, 2024:

If a parent+child are relayed roughly simultaneously across the network, then I would expect there to be times when we download the transactions out of order (eg because we send a request for the parent to a different peer than the request for the child).

instagibbs commented at 12:40 pm on April 23, 2024:

I guess it’ll likely be something like:

peer A sends child, child is put in orphanage and parent tx request queued(but not yet sent)
peer B has slightly out of date feefilter for your node, sends INV for parent

though I’m not convinced this really will make a difference

glozow commented at 12:55 pm on April 23, 2024:

fwiw it’s definitely not common, but I do see them occasionally and have a handful of acceptances of packages from 2 different peers

glozow commented at 1:00 pm on April 23, 2024:

though I’m not convinced this really will make a difference

Is the suggestion to drop this and only try same-peer packages?

sdaftuar commented at 1:04 pm on April 23, 2024:

I guess it’ll likely be something like:

peer A sends child, child is put in orphanage and parent tx request queued(but not yet sent)

peer B has slightly out of date feefilter for your node, sends INV for parent

What I’d expect to happen is that we get inv’s for parent+child from both Peer A and Peer B, and we happen to request the parent from A and the child from B, but the child arrives first – it’s put in the orphanage, and since a request for the parent is already in flight to A, we don’t send another request for the parent to B.

(In this scenario I guess I’m assuming no feefilter, or that the feefilter value is slightly stale.)

instagibbs commented at 1:25 pm on April 23, 2024:

Is the suggestion to drop this and only try same-peer packages?

If this is hitting actual usage, no, because it’s coded and I actually reviewed it, and tests seem to cover it. I’m unsure if in BIP331-like world it would be used since it would be receiver-driven, but that might be thinking too far ahead.

(In this scenario I guess I’m assuming no feefilter, or that the feefilter value is slightly stale.)

Latter scenario seems likely in practice as I see things right on the bubble all the time.

glozow commented at 2:00 pm on April 24, 2024:

Ok, marking this as resolved

in test/functional/p2p_opportunistic_1p1c.py:312 in 55b1280c52 outdated

307+        # 2. Send child.
308+        peer_sender.send_and_ping(msg_tx(child_bumping["tx"]))
309+
310+        # 3. Node should not request any parents, as it should recognize that it will not accept
311+        # multi-parent-1-child packages.
312+        node.bumpmocktime(60)

instagibbs commented at 2:29 pm on April 23, 2024:

is there a constant we can put here?

glozow commented at 2:02 pm on April 24, 2024:

Added

in src/test/fuzz/txorphan.cpp:1 in 55b1280c52

sr-gi commented at 2:50 pm on April 23, 2024:

Reviewing the changes in this file I realized something seems to be odd with the way the test was initially designed:

duplicate_input is defined as a flag that, if set, will allow duplicate inputs to be used when building the transaction. However, the way this is approached is weird: if duplicate_input is not set, upon picking prevout as input, we will kick it off outpoints to make sure we don’t pick it up again as another input. Later on, all selected inputs are added back to outpoints, so they can be used again in the next iteration. However, this is done unconditionally, therefore, if duplicate_input is set, we will end up adding the same element to the collection twice (given we would not have kicked it off).

Hence, the more an input is picked, the more copies of that element will end up added to the collection, and the more likely that input will be picked. PickValue is already picking things randomly (AFAICT), so there is no need to have duplicates in the collection.

sr-gi commented at 2:52 pm on April 23, 2024:

I also found the way num_out is picked to be a bit strange. num_in is picked based on the outpoints size, given that is our input candidate set. However, num_out is picked in the same way, and it doesn’t really need to be bound by that, just by making sure the transaction stays within its size limits

sr-gi commented at 2:53 pm on April 23, 2024:

I don’t think this needs to be fixed in this PR btw, a followup would do

glozow commented at 9:20 am on April 26, 2024:

As discussed offline, I agree the fuzz test has a few quirks that could be fixed up, but I think they are out of scope for this PR so I’m going to mark this resolved.

in test/functional/p2p_opportunistic_1p1c.py:377 in 55b1280c52 outdated

312+        node.bumpmocktime(60)
313+        peer_sender.sync_with_ping()
314+        assert "getdata" not in peer_sender.last_message
315+
316+
317+    def run_test(self):

instagibbs commented at 3:38 pm on April 23, 2024:

regression test for the Assume() that was hit? And unsetting the mocktime to not have interference in subtests…

  0diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
  1index 2eaa2a0a79..f3a741498b 100755
  2--- a/test/functional/p2p_opportunistic_1p1c.py
  3+++ b/test/functional/p2p_opportunistic_1p1c.py
  4@@ -291,58 +291,97 @@ class PackageRelayTest(BitcoinTestFramework):
  5         child_bumping = self.wallet_nonsegwit.create_self_transfer_multi(
  6             utxos_to_spend=[parent_low_1["new_utxo"], parent_low_2["new_utxo"]],
  7             fee_per_output=999*parent_low_1["tx"].get_vsize(),
  8         )
  9 
 10         peer_sender = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=4, connection_type="outbound-full-relay")
 11 
 12         # 1. Send both parents. Each should be rejected for being too low feerate.
 13         # Send unsolicited so that we can later check that no "getdata" was ever received.
 14         peer_sender.send_and_ping(msg_tx(parent_low_1["tx"]))
 15         peer_sender.send_and_ping(msg_tx(parent_low_2["tx"]))
 16 
 17         # parent_low_1 and parent_low_2 are rejected for being low feerate.
 18         assert parent_low_1["txid"] not in node.getrawmempool()
 19         assert parent_low_2["txid"] not in node.getrawmempool()
 20 
 21         # 2. Send child.
 22         peer_sender.send_and_ping(msg_tx(child_bumping["tx"]))
 23 
 24         # 3. Node should not request any parents, as it should recognize that it will not accept
 25         # multi-parent-1-child packages.
 26         node.bumpmocktime(60)
 27         peer_sender.sync_with_ping()
 28         assert "getdata" not in peer_sender.last_message
 29 
 30+        node.setmocktime(0)
 31+
 32+
 33+ [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
 34+    def test_in_mempool_parent(self):
 35+        self.log.info("Check that node rejects a 1p1c package if another parent is already in mempool")
 36+
 37+        node = self.nodes[0]
 38+        #node.setmocktime(int(time.time()))
 39+
 40+        # 2-parent-1-child package where first parent is above mempool minfeerate but second parents is below
 41+        parent_high = self.wallet_nonsegwit.create_self_transfer(fee_rate=999*FEERATE_1SAT_VB, confirmed_only=True)
 42+        parent_low = self.wallet_nonsegwit.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
 43+        child_bumping = self.wallet_nonsegwit.create_self_transfer_multi(
 44+            utxos_to_spend=[parent_high["new_utxo"], parent_low["new_utxo"]],
 45+            fee_per_output=999*parent_low["tx"].get_vsize(),
 46+        )
 47+
 48+        peer_sender = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=4, connection_type="outbound-full-relay")
 49+
 50+        # 1. Send first parent which will be accepted.
 51+        peer_sender.send_and_ping(msg_tx(parent_high["tx"]))
 52+        assert parent_high["txid"] in node.getrawmempool()
 53+
 54+        # 2. Send child.
 55+        peer_sender.send_and_ping(msg_tx(child_bumping["tx"]))
 56+
 57+        # 3. Node should request parent_low via txid, but node will reject child_bumping + parent_low
 58+        #    due to `package-not-child-with-unconfirmed-parents`
 59+        #node.bumpmocktime(60)
 60+        parent_low_txid_int = int(parent_low["txid"], 16)
 61+        peer_sender.wait_for_getdata([parent_low_txid_int])
 62+        peer_sender.send_and_ping(msg_tx(parent_low["tx"]))
 63+
 64+        node_mempool = node.getrawmempool()
 65+        assert parent_low["txid"] not in node_mempool
 66+        assert child_bumping["txid"] not in node_mempool
 67+        assert parent_high["txid"] in node_mempool
 68 
 69     def run_test(self):
 70         node = self.nodes[0]
 71         self.wallet = MiniWallet(node)
 72         self.wallet_nonsegwit = MiniWallet(node, mode=MiniWalletMode.RAW_P2PK)
 73         self.generate(self.wallet_nonsegwit, 10)
 74         self.generate(self.wallet, 20)
 75 
 76         filler_wallet = MiniWallet(node)
 77         fill_mempool(self, node, filler_wallet)
 78 
 79         self.log.info("Check opportunistic 1p1c logic when parent (txid != wtxid) is received before child")
 80         self.test_basic_parent_then_child(self.wallet)
 81 
 82         self.log.info("Check opportunistic 1p1c logic when parent (txid == wtxid) is received before child")
 83         self.test_basic_parent_then_child(self.wallet_nonsegwit)
 84 
 85         self.log.info("Check opportunistic 1p1c logic when child is received before parent")
 86         self.test_basic_child_then_parent()
 87 
 88         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid != wtxid)")
 89         self.test_low_and_high_child(self.wallet)
 90 
 91         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid == wtxid)")
 92         self.test_low_and_high_child(self.wallet_nonsegwit)
 93 
 94         self.test_orphan_consensus_failure()
 95         self.test_parent_consensus_failure()
 96         self.test_multiple_parents()
 97+        self.test_in_mempool_parent()
 98 
 99 
100 if __name__ == '__main__':
101     PackageRelayTest().main()

glozow commented at 2:01 pm on April 24, 2024:

Added a similar test

instagibbs commented at 4:33 pm on April 23, 2024: member

reviewed through 55b1280c52af81aa6ea0860799fa16da49f51447

Looks good, have a suggestion for a test catching the Assume() issue(which does actually get hit in mainnet pretty fast with debug on)

in src/net_processing.cpp:608 in bb3bf0a39c outdated

603+     *   */
604+    void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector<NodeId>& senders)
605+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
606+
607+    /** Look for a child of this transaction in the orphanage to form a 1-parent-1-child package,
608+     * skipping any combinations have already been tried, and submit them together to the mempool.

sr-gi commented at 5:18 pm on April 23, 2024:

In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

nit: skipping any combinations that have already been tried (?)

glozow commented at 9:24 am on April 26, 2024:

added

in src/net_processing.cpp:3289 in bb3bf0a39c outdated

3284+    // Prefer children from this peer. This helps prevent censorship attempts in which an attacker
3285+    // sends lots of fake children for the parent, and we (unluckily) keep selecting the fake
3286+    // children instead of the real one provided by the honest peer.
3287+    const auto cpfp_candidates_same_peer{m_orphanage.GetChildrenFromSamePeer(ptx, nodeid)};
3288+
3289+    // These children should be sorted from most newest to oldest. In the (probably uncommon) case

sr-gi commented at 6:00 pm on April 23, 2024:

In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

nit: ~~most~~ newest

glozow commented at 9:35 am on April 26, 2024:

haha oops, deleted

in src/net_processing.cpp:3280 in bb3bf0a39c outdated

3275+    AssertLockHeld(cs_main);
3276+
3277+    const auto& parent_txid{ptx->GetHash()};
3278+    const auto& parent_wtxid{ptx->GetWitnessHash()};
3279+
3280+    CTransactionRef tx_orphan{nullptr};

sr-gi commented at 6:47 pm on April 23, 2024:

In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

I think you can potentially get rid of this variable by having a reference to a Package instead, which is set to the maybe_cpfp_package when the package cannot be found in m_recent_rejects_reconsiderable.

That way you won’t need to construct the package again in the current if (tx_orphan) scope.

glozow commented at 9:34 am on April 26, 2024:

Ok I’ve done a bit of refactoring to split this into 2 functions for finding the package and validating/processing the package, and made a PackageToValidate struct that’s difficult to misuse. Got rid of these “set me if you find something” variables.

sr-gi commented at 2:50 pm on April 26, 2024:

Much cleaner now

in src/net_processing.cpp:3248 in bb3bf0a39c outdated

3216+    AssertLockHeld(g_msgproc_mutex);
3217+    AssertLockHeld(cs_main);
3218+
3219+    if (package_result.m_state.IsInvalid()) {
3220+        m_recent_rejects_reconsiderable.insert(GetPackageHash(package));
3221+    }

sr-gi commented at 6:56 pm on April 23, 2024:

In bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6

Shouldn’t this be moved after the two next checks, to avoid bloating the rolling bloom filter with data that is unconditionally invalid? The only way for a package of this type to be actually resonsiderable is if it fails due to a transaction being rejected, but not if PCKG_POLICY or PCKG_MEMPOOL_ERROR

glozow commented at 9:30 am on April 26, 2024:

Packages aren’t ever reconsiderable since we never submit anything beyond 1p1c. I’ve clarified this in the docs now. I definitely think we should cache a rejection when the error is PCKG_POLICY or PCKG_MEMPOOL_ERROR.

sr-gi commented at 2:58 pm on April 26, 2024:

Why caching this in m_recent_rejects_reconsiderable instead of m_recent_rejects if we are not going to reconsider them?

glozow commented at 8:25 am on April 27, 2024:

Same question as #28970 (review)

I like that this doesn’t add extra burden to m_recent_rejects (which is probably the busier of the 2 filters even though they have the same size). It makes more sense if you think of each tx within the package as reconsiderable, even though the package isn’t?

sr-gi commented at 1:46 pm on April 29, 2024:

I think, design-wise, it’s a bit counter-intuitive, but I do agree that not overloading m_recent_rejects is potentially better

in src/test/txpackage_tests.cpp:121 in 6edf8ec0c5 outdated

116+    BOOST_CHECK(hash_if_by_txid != calculated_hash_123);
117+
118+    uint256 hash_if_use_txid = (HashWriter() << txid_2 << txid_1 << txid_3).GetSHA256();
119+    BOOST_CHECK(hash_if_use_txid != calculated_hash_123);
120+
121+    uint256 hash_if_use_int_order = (HashWriter() << wtxid_2 << wtxid_1 << txid_3).GetSHA256();

theStack commented at 1:33 pm on April 24, 2024:

Was it intended to mix both wtxids and txids for hashing in this test-case? (IIUC the meaning of “int order” here, this would be wtxid2, wtxid1, wtxid3, i.e. identical with hash_if_by_txid, so I guess one of the two test-cases can be removed).

0    uint256 hash_if_use_int_order = (HashWriter() << wtxid_2 << wtxid_1 << wtxid_3).GetSHA256();

glozow commented at 4:35 pm on April 24, 2024:

Yeah that’s a typo :+1:

glozow commented at 9:18 am on April 26, 2024:

fixed

in src/test/txpackage_tests.cpp:98 in 6edf8ec0c5 outdated

92+    BOOST_CHECK_EQUAL(tx_3.GetHash(), txid_3);
93+
94+    BOOST_CHECK(txid_2.GetHex() < txid_1.GetHex());
95+
96+    BOOST_CHECK(txid_1.ToUint256() != wtxid_1.ToUint256());
97+    BOOST_CHECK(txid_3.ToUint256() != wtxid_3.ToUint256());

theStack commented at 1:34 pm on April 24, 2024:

nit: for the sake of completeness, could also check that txid and wtxid are equal for non-segwit tx 2:

0    BOOST_CHECK(txid_1.ToUint256() != wtxid_1.ToUint256());
1    BOOST_CHECK_EQUAL(txid_2.ToUint256(), wtxid_2.ToUint256());
2    BOOST_CHECK(txid_3.ToUint256() != wtxid_3.ToUint256());

glozow commented at 9:18 am on April 26, 2024:

added

glozow force-pushed on Apr 24, 2024

instagibbs approved

instagibbs commented at 2:13 pm on April 24, 2024: member

ACK https://github.com/bitcoin/bitcoin/pull/28970/commits/30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

Changes are test only, and new subtest causes a debug-build crash when the PCKG_POLICY change is reverted.

DrahtBot requested review from theStack on Apr 24, 2024

DrahtBot requested review from murchandamus on Apr 24, 2024

DrahtBot requested review from sdaftuar on Apr 24, 2024

in src/net_processing.cpp:3267 in bb3bf0a39c outdated

3262+                }
3263+            }
3264+        }
3265+        package_iter++;
3266+        // There should be a sender for each tx, but fall back to the last one otherwise.
3267+        if (Assume(senders_iter != senders.rend())) senders_iter++;

theStack commented at 3:42 pm on April 24, 2024:

nit: as alternative, could check at the beginning of the function if the sizes of package and senders match and return early if they don’t (not sure how this function is used in the future if the 1p1c limit is lifted, but i guess a size mismatch should never happen, unless there is a bug at the call-site?). that would be slightly simpler to reason imho.

glozow commented at 4:35 pm on April 24, 2024:

I think we’d want to make sure that even if we had a bad senders vector we would still call ProcessValidTx / ProcessInvalidTx (seems kind of bad not to). Maybe a better approach is to use -1 when we don’t know what it is - I do realize we’re not handling the empty vector case.

(Yeah maybe this is moot as the 1 caller should always make senders properly…)

glozow commented at 9:31 am on April 26, 2024:

Ok I have deleted this now and created a PackageToValidate struct where the only constructor available guarantees there is a sender for each tx. So I’ve removed this Assume stuff as it’s probably overly paranoid.

in src/net_processing.cpp:3327 in bb3bf0a39c outdated

3322+            Package maybe_cpfp_package{ptx, cpfp_candidates_different_peer.at(index).first};
3323+            if (!m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
3324+                tx_orphan = cpfp_candidates_different_peer.at(index).first;
3325+                orphan_sender = cpfp_candidates_different_peer.at(index).second;
3326+                break;
3327+            }

theStack commented at 3:45 pm on April 24, 2024:

refactoring nit to avoid multiple accesses in cpfp_candidates_different_peer (feel free to ignore):

0            const auto [candidate_child, candidate_peer] = cpfp_candidates_different_peer.at(index);
1            const Package maybe_cpfp_package{ptx, candidate_child};
2             if (!m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
3                tx_orphan = candidate_child;
4                orphan_sender = candidate_peer;
5                break;
6            }

glozow commented at 10:09 am on April 26, 2024:

done

in test/functional/p2p_1p1c_network.py:129 in 30c9e6bc4e outdated

124+        packages_to_submit.append(package_hex_4)
125+
126+        # node0: sender
127+        # node1: pre-received the children (orphan)
128+        # node3: pre-received the parents (too low fee)
129+        # All nodes receive high_parent_51 ahead of time.

theStack commented at 4:17 pm on April 24, 2024:

0        # All nodes receive parent_31 ahead of time.

glozow commented at 8:48 am on April 26, 2024:

done

theStack commented at 4:19 pm on April 24, 2024: contributor

Another round through (modulo the fuzz commit), the code looks correct to me, just left a few nits below. Planning to do another review tomorrow with focus on the main commit bb3bf0a39c036fbd94b99ed7002db7d9bdf0a1d6, especially going through the AlreadyHaveTx call-sites and their include_reconsiderable values to check once more.

DrahtBot requested review from theStack on Apr 24, 2024

in test/functional/p2p_1p1c_network.py:67 in 30c9e6bc4e outdated

62+        package_hex_basic = [low_fee_parent["hex"], high_fee_child["hex"]]
63+        return package_hex_basic, low_fee_parent["tx"], high_fee_child["tx"]
64+
65+    def create_package_2outs(self, wallet):
66+        # First create a tester tx to see the vsize, and then adjust the fees
67+        placeholder_fee = 100000

sr-gi commented at 7:25 pm on April 24, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

Is the placeholder fee needed? create_self_transfer_multi already has a default fee, and the value shouldn’t matter, should it?

glozow commented at 8:45 am on April 26, 2024:

removed

in test/functional/p2p_1p1c_network.py:74 in 30c9e6bc4e outdated

69+
70+        low_fee_parent_2outs_tester = wallet.create_self_transfer_multi(
71+            utxos_to_spend=[utxo_for_2outs],
72+            num_outputs=2,
73+            fee_per_output=placeholder_fee,
74+            confirmed_only=True

sr-gi commented at 7:26 pm on April 24, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

confirmed_only is only used if no utxos are provided, but this is not the case, so it shouldn’t be needed.

glozow commented at 8:45 am on April 26, 2024:

removed

in test/functional/p2p_1p1c_network.py:119 in 30c9e6bc4e outdated

114+        # 4: parent + child package where the child spends 2 different outputs from the parent.
115+        package_hex_4, parent_4, child_4 = self.create_package_2outs(self.wallet)
116+
117+        # Assemble return results
118+        packages_to_submit = []
119+        txns_to_send = [[]] * self.num_nodes

sr-gi commented at 7:40 pm on April 24, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

nit: There’s no need to reserve here, this could be defined as txns_to_send = [[]] and subsequent calls could call append instead

glozow commented at 8:48 am on April 26, 2024:

I’ve changed them to just construct everything in 1 line, should be clearer

in test/functional/p2p_1p1c_network.py:151 in 30c9e6bc4e outdated

146+        self.wallet.rescan_utxos(include_mempool=True)
147+        packages_to_submit, transactions_to_presend = self.create_packages()
148+
149+        self.peers = []
150+        for i in range(self.num_nodes):
151+            self.peers.append(self.nodes[i].add_p2p_connection(P2PInterface()))

sr-gi commented at 7:48 pm on April 24, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

nit: This could be done more idiomatically, but feel free to disregard

0        self.peers = [self.nodes[i].add_p2p_connection(P2PInterface()) for i in range(self.num_nodes)]

glozow commented at 8:49 am on April 26, 2024:

done

in test/functional/p2p_1p1c_network.py:155 in 30c9e6bc4e outdated

150+        for i in range(self.num_nodes):
151+            self.peers.append(self.nodes[i].add_p2p_connection(P2PInterface()))
152+
153+        self.log.info("Pre-send some transactions to nodes")
154+        for i in range(self.num_nodes):
155+            peer = self.peers[i]

sr-gi commented at 8:18 pm on April 24, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

0        for (i, peer) in enumerate(self.peers):

glozow commented at 8:50 am on April 26, 2024:

done

in test/functional/p2p_opportunistic_1p1c.py:158 in 30c9e6bc4e outdated

153+        # Send again from peer_ignored, check that it is ignored
154+        peer_ignored.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=parent_wtxid_int)]))
155+        assert "getdata" not in peer_ignored.last_message
156+
157+        # 2. Send an (orphan) child that has a higher feerate, but not enough to bump the parent.
158+        low_child_wtxid_int = int(med_fee_child["tx"].getwtxid(), 16)

sr-gi commented at 9:28 pm on April 24, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

nit: shouldn’t this be called med_child_wtxid_int, for consistency?

glozow commented at 8:52 am on April 26, 2024:

done

in test/functional/p2p_opportunistic_1p1c.py:190 in 30c9e6bc4e outdated

174+        peer_ignored.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=low_child_wtxid_int)]))
175+        assert "getdata" not in peer_ignored.last_message
176+        # If either peer sends the parent again, package evaluation should not be attempted
177+        peer_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))
178+        peer_ignored.send_and_ping(msg_tx(low_fee_parent["tx"]))
179+

sr-gi commented at 9:31 pm on April 24, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

Given the comment above, isn’t this missing the check for both transactions not being in the mempool?

glozow commented at 8:54 am on April 26, 2024:

added

in test/functional/p2p_opportunistic_1p1c.py:211 in 30c9e6bc4e outdated

192+        peer_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))
193+
194+        # 8. Both transactions should now be in mempool
195+        node_mempool = node.getrawmempool()
196+        assert low_fee_parent["txid"] in node_mempool
197+        assert high_fee_child["txid"] in node_mempool

sr-gi commented at 9:34 pm on April 24, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

I think it wouldn’t hurt to check that med_fee_child is not in the mempool. It cannot be, given it is a double spend of high_fee_child, but it made me check how the transactions were created, given it could have been the case that low_fee_parent had more than one output

glozow commented at 8:54 am on April 26, 2024:

sure, added

in test/functional/p2p_opportunistic_1p1c.py:298 in 30c9e6bc4e outdated

276+
277+        self.log.info("Check that fake parent does not cause orphan to be deleted and real package can still be submitted")
278+        # 6. Child-sending should not have been punished and the orphan should remain in orphanage.
279+        # It can send the "real" parent transaction, and the package is accepted.
280+        package_sender.sync_with_ping()
281+        package_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))

sr-gi commented at 9:41 pm on April 24, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

Is the parent kept as recently rejected? I tried to modify the test to make the peer offer the transaction over the regular inv->getdata->tx flow and looks like the node won’t request it.

0parent_wtxid_int = int(low_fee_parent["tx"].getwtxid(), 16)
1package_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=parent_wtxid_int)]))
2package_sender.wait_for_getdata([parent_wtxid_int])
3package_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))

0  File "/xxx//test/functional/p2p_opportunistic_1p1c.py", line 283, in test_parent_consensus_failure
1    package_sender.wait_for_getdata([parent_wtxid_int])

If so, it feels that could be exploited to prevent a package from being accepted

instagibbs commented at 1:58 pm on April 25, 2024:

this is from test_orphan_consensus_failure causing the exact same parent to be added to m_recent_rejects_reconsiderable

Can we somehow cause chaintip updates during cleanup, without dropping minfee too much? What if cleanup detects that it went down too much, then calls fill_mempool again?

sr-gi commented at 2:10 pm on April 25, 2024:

I’ve done some more digging on this and looks like this is an interference between test_orphan_consensus_failure and test_parent_consensus_failure. The low_fee_parent created in the former is the exact same as the one created in the latter.

sr-gi commented at 2:13 pm on April 25, 2024:

this is from test_orphan_consensus_failure causing the exact same parent to be added to m_recent_rejects_reconsiderable

Can we somehow cause chaintip updates during cleanup, without dropping minfee too much? What if cleanup detects that it went down too much, then calls fill_mempool again?

lol, I missed this 🤣

instagibbs commented at 2:16 pm on April 25, 2024:

Adding this block to cleanup resets the cache, though I haven’t looked at if this messes with minfee too much:

0            # Cause small re-org and tickle AlreadyHaveTx to flush reject filters between tests
1            chaintip = self.nodes[0].getbestblockhash()
2            self.nodes[0].invalidateblock(chaintip)
3            peer_sender = self.nodes[0].add_p2p_connection(P2PInterface())
4            peer_sender.send_message(msg_tx(CTransaction()))
5            peer_sender.wait_for_disconnect()
6            self.nodes[0].reconsiderblock(chaintip)

glozow commented at 2:20 pm on April 25, 2024:

Can we somehow cause chaintip updates during cleanup, without dropping minfee too much? What if cleanup detects that it went down too much, then calls fill_mempool again?

Hm, fill_mempool isn’t really built for this (though we can change that). I’ll try to see if we can make sure we don’t reuse utxos.

glozow commented at 9:56 am on April 26, 2024:

I’ve fixed it by having each subtest set a unique nSequence on parent transactions :shrug: Also considered changing MiniWallet to have some randomness but this seemed simpler. I’ve added these lines to the test as well.

sr-gi commented at 2:30 pm on April 26, 2024:

Seems good enough

in test/functional/p2p_opportunistic_1p1c.py:142 in 30c9e6bc4e outdated

137+        feerate_just_above = node.getmempoolinfo()["mempoolminfee"]
138+        med_fee_child = wallet.create_self_transfer(utxo_to_spend=low_fee_parent["new_utxo"], fee_rate=feerate_just_above)
139+        high_fee_child = wallet.create_self_transfer(utxo_to_spend=low_fee_parent["new_utxo"], fee_rate=999*FEERATE_1SAT_VB)
140+
141+        peer_sender = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=3, connection_type="outbound-full-relay")
142+        peer_ignored = node.add_outbound_p2p_connection(P2PInterface(), p2p_idx=4, connection_type="outbound-full-relay")

sr-gi commented at 1:03 pm on April 25, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

nit: Given you are cleaning after each test, this could be 1, 2

sr-gi commented at 1:04 pm on April 25, 2024:

The same happens in the last two tests with p2p_idx=4

glozow commented at 8:52 am on April 26, 2024:

done

in test/functional/p2p_opportunistic_1p1c.py:341 in 30c9e6bc4e outdated

319+        peer_sender.sync_with_ping()
320+        assert "getdata" not in peer_sender.last_message
321+
322+    @cleanup
323+    def test_other_parent_in_mempool(self):
324+        self.log.info("Check opportunistic 1p1c fails if child already has another parent in mempool")

sr-gi commented at 1:14 pm on April 25, 2024:

In 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

How is this different from submitting the whole package? My understanding is that given a package with [p1_high, p2_low, c12], on processing the package, p1_high would make it to the mempool by itself: https://github.com/bitcoin/bitcoin/blob/2a07c4662d7266158d47f79fa2433ab22e22c907/src/validation.cpp#L1611

And the [p2_low, c12] will make it as a package: https://github.com/bitcoin/bitcoin/blob/2a07c4662d7266158d47f79fa2433ab22e22c907/src/validation.cpp#L1639-L1640

So why have the distinction?

glozow commented at 9:06 am on April 26, 2024:

How is this different from submitting the whole package?

The p2p logic is only going to match up 1p1cs, so it won’t ever submit the whole package. I suppose you’re asking why the validation logic is this way?

For the purpose of this PR, I think we should just try to make sure the p2p logic is able to handle the result returned from validation, whatever it may be. If you’re interested in why the validation stuff works the way it does, all the PRs are listed in #27463. The relevant one here is #22674 (also have review club notes). TLDR we do a topology check at the top of AcceptPackage and check that we have all unconfirmed ancestors. Idea was to drop it if we’re still missing something, though I don’t really like this approach anymore (also see #28970 (review)).

sr-gi commented at 1:25 pm on April 25, 2024: member

Third pass, reviewing the changes of the first two, plus going all the way to 30c9e6bc4e9f8b8b606e55435dc3f743cb2dd670

I think the code looks good overall, but I have some questions regarding some corner cases that I’m not completely sure about (see inline).

Finally, something I’ve noticed: Given a 1p1c whose parent we’ve seen already and we just received the child, the parent will be tried on its own twice before being considered as a package. This is because we will try it first when receiving it, another time when submitting it as a package, by splitting the package, and a third time alongside the child. My understanding of this is that we try to submit the smallest package that cannot make it on its own (I’m assuming to prevent some pathological cases). This makes sense in the general case, but not so much in the 1p1c. I’m not sure having additional complexity to treat this specific case would be worth it though.

[txpackages] add canonical way to get hash of package 092c978a42

[txorphanage] add method to get all orphans spending a tx 2f51cd680f

[unit test] TxOrphanage::GetChildrenFrom* d095316c1c

[fuzz] break out parent functions and add GetChildrenFrom* coverage

It's very hard to randomly construct a transaction that would be the
parent of an existing orphanage tx. For functions like
AddChildrenToWorkSet and GetChildren that take orphan parents, use a tx
that was previously constructed.

410ebd6efa

[p2p] add separate rejections cache for reconsiderable txns 6c51e1d7d0

glozow force-pushed on Apr 26, 2024

DrahtBot added the label CI failed on Apr 26, 2024

DrahtBot commented at 10:15 am on April 26, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/24292958143

[p2p] opportunistically accept 1-parent-1-child packages 87c5c524d6

[functional test] opportunistic 1p1c package submission e518a8bf8a

glozow force-pushed on Apr 26, 2024

glozow commented at 1:02 pm on April 26, 2024: member

Addressed @theStack and @sr-gi comments. Thanks!

in src/net_processing.cpp:607 in 87c5c524d6 outdated

602+     *                          in package, in the same order.
603+     *   */
604+    void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector<NodeId>& senders)
605+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
606+
607+    /** A package to validate  */

instagibbs commented at 1:12 pm on April 26, 2024:

redundant comment

in src/net_processing.cpp:3240 in 87c5c524d6 outdated

3236@@ -3198,6 +3237,117 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c
3237     }
3238 }
3239 
3240+void PeerManagerImpl::ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector<NodeId>& senders)

instagibbs commented at 1:25 pm on April 26, 2024:

now that we have PackageToValidate, just directly pass here?

glozow commented at 12:43 pm on May 1, 2024:

Done in #30012

in src/net_processing.cpp:620 in 87c5c524d6 outdated

615+                                   NodeId child_sender) :
616+            m_txns{parent, child},
617+            m_senders {parent_sender, child_sender}
618+        {}
619+
620+        std::string ToString() const {

instagibbs commented at 1:51 pm on April 26, 2024:

useful log message for stats :+1:

instagibbs approved

instagibbs commented at 2:00 pm on April 26, 2024: member

reACK e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f

non-blocking comments

reviewed via git range-diff master 30c9e6b e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f

DrahtBot removed the label CI failed on Apr 26, 2024

sr-gi commented at 1:54 pm on April 29, 2024: member

tACK e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f

All comments have been addressed/replied to. Tests are now passing even in this case: #28970 (review)

theStack approved

theStack commented at 8:36 am on April 30, 2024: contributor

Code-review ACK e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f :package:

glozow commented at 8:59 am on April 30, 2024: member

Since there are a few ACKs now, listing followups. I plan to open a PR for the first two immediately:

redundant comment + pass PackageToValidate directly #28970#pullrequestreview-2025023787
make MempoolAcceptResult::m_replaced_transactions non-optional #28970 (review)
(already in #29974) fix quirks in fuzz/txorphan.cpp #28970 (review)
consider dropping requirement that all unconfirmed parents be present, allow package with other parent in mempool #28970 (review). This may conflict with #28984, so will hold off for now.
delete orphan if parents are rejected #28970 (review)

in src/net_processing.cpp:4651 in 87c5c524d6 outdated

4645@@ -4467,6 +4646,19 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4646         if (state.IsInvalid()) {
4647             ProcessInvalidTx(pfrom.GetId(), ptx, state, /*maybe_add_extra_compact_tx=*/true);
4648         }
4649+        // When a transaction fails for TX_RECONSIDERABLE, look for a matching child in the
4650+        // orphanage, as it is possible that they succeed as a package.
4651+        if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) {

dergoegge commented at 1:52 pm on April 30, 2024:

I think attempting to accept a 1p1c package could also be done when reconsidering transactions from the orphanage in ProcessOrphanTx. E.g. you might have a 1p1c package in your orphanage (P -> C) and if P is reconsidered but rejected as TX_RECONSIDERABLE you might want to look for C in the orphanage and if found try 1p1c acceptance.

Maybe its not worth adding right now (any scenarios I can think of seem somewhat rare) but perhaps worthwhile later if the data suggests so.

in src/net_processing.cpp:4597 in e518a8bf8a

4592                     fRejectedParents = true;
4593                     break;
4594+                } else if (m_recent_rejects_reconsiderable.contains(parent_txid) && !m_mempool.exists(GenTxid::Txid(parent_txid))) {
4595+                    // More than 1 parent in m_recent_rejects_reconsiderable: 1p1c will not be
4596+                    // sufficient to accept this package, so just give up here.
4597+                    if (rejected_parent_reconsiderable.has_value()) {

glozow commented at 1:59 pm on April 30, 2024:

Ah, just found a mistake. I think this can go in a followup though.

0                    if (rejected_parent_reconsiderable.has_value() && *rejected_parent_reconsiderable != parent_txid.ToUint256()) {

sdaftuar commented at 2:04 pm on April 30, 2024:

Hmm, we’re looping over unique parents already, so I think this code should be fine? In which case is there a bug?

instagibbs commented at 2:05 pm on April 30, 2024:

they’re unique txids, which scenarios will this fix?

glozow commented at 2:11 pm on April 30, 2024:

Ah you’re right, nevermind! I was thinking this would be a bug if it spends twice from the same low-feerate parent. Yes, unique_parents already takes care of this case.

in src/txorphanage.cpp:308 in 2f51cd680f outdated

303+        }
304+    }
305+
306+    // Erase duplicates
307+    std::sort(iters.begin(), iters.end(), IteratorComparator());
308+    iters.erase(std::unique(iters.begin(), iters.end()), iters.end());

dergoegge commented at 2:04 pm on April 30, 2024:

Why is this sort comparison different from the one in GetChildrenFromSamePeer?

These functions are almost identical as well. Perhaps a more generic std::vector<std::pair<CTransactionRef, NodeId>> GetChildren(const CTransactionRef&) that returns all children and their senders would be less code?

mzumsande commented at 10:32 pm on April 30, 2024:

I agree, would be nice to avoid this code duplication.

instagibbs commented at 10:36 pm on April 30, 2024:

made a similar point at #28970 (review) fwiw (with a response attached)

glozow commented at 7:56 am on May 1, 2024:

Why is this sort comparison different from the one in GetChildrenFromSamePeer?

The idea is to return GetChildrenFromSamePeer in recency order. This one could be returned that way as well, as the goal is to have a random order (and we randomize the indices), but all I wanted to do here was deduplicate.

These functions are almost identical as well.

As for the 2 functions having duplicate code, for a longer explanation: I’m hoping we can remove 1p1cs with different providers by tracking all orphan announcers, which would mean deleting GetChildrenFromDifferentPeer. I think it’ll be easier to delete the function + unit tests wholesale if it’s a different function. But it seems I’m in the minority here, and apologies for resolving the first comment about this. Will avoid this in the future.

dergoegge commented at 2:08 pm on April 30, 2024: member

light Code review ACK e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f

in src/policy/packages.cpp:159 in 092c978a42 outdated

154+    std::vector<Wtxid> wtxids_copy;
155+    std::transform(transactions.cbegin(), transactions.cend(), std::back_inserter(wtxids_copy),
156+        [](const auto& tx){ return tx->GetWitnessHash(); });
157+
158+    // Sort in ascending order
159+    std::sort(wtxids_copy.begin(), wtxids_copy.end(), [](const auto& lhs, const auto& rhs) { return lhs.GetHex() < rhs.GetHex(); });

achow101 commented at 9:41 pm on April 30, 2024:

In 092c978a42e8f4a02291b994713505ba8aac8b28 “[txpackages] add canonical way to get hash of package”

Is there a reason to convert the wtxids into hex strings for this comparison? That seems kind of expensive, especially when Wtxid already has an operator<.

I surmise that this has to do with the “concatenated in lexicographical order (treating the wtxids as little endian encoded uint256, smallest to largest)” and BIP 331 defines the same thing, but it’s not clear to me why it must be like that (and couldn’t also be changed in the BIP).

If this must be done in reverse byte order, then I think it would at be better to use std::lexicographical_compare with reverse iterators, rather than turning the wtxids into strings:

0    std::sort(wtxids_copy.begin(), wtxids_copy.end(), [](const auto& lhs, const auto& rhs) { return std::lexicographical_compare(std::make_reverse_iterator(lhs.end()), std::make_reverse_iterator(lhs.begin()), std::make_reverse_iterator(rhs.end()), std::make_reverse_iterator(rhs.begin())); });

glozow commented at 8:06 am on May 1, 2024:

Is there a reason to convert the wtxids into hex strings for this comparison? That seems kind of expensive, especially when Wtxid already has an operator<.

I surmise that this has to do with the “concatenated in lexicographical order (treating the wtxids as little endian encoded uint256, smallest to largest)” and BIP 331 defines the same thing, but it’s not clear to me why it must be like that (and couldn’t also be changed in the BIP).

The code was originally using the operator< defined on uint256, and the opposite switch happened :sweat_smile: A few people commented on lexicographical being underspecified or different from how it’s usually defined (e.g. #28970 (review)). Mostly I felt like reverse byte order was much more intuitive, so I changed it here and in the BIP. Yes this doesn’t have to match the BIP since it’s only used internally, but I figured I might as well write something that’s reusable.

Thanks for the std::lexicographical_compare suggestion, I’ll add that in a followup.

glozow commented at 12:43 pm on May 1, 2024:

Done in #30012

in src/net_processing.cpp:3140 in 6c51e1d7d0 outdated

3132@@ -3097,7 +3133,14 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx
3133         // See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
3134         // for concerns around weakening security of unupgraded nodes
3135         // if we start doing this too early.
3136-        m_recent_rejects.insert(ptx->GetWitnessHash().ToUint256());
3137+        if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) {
3138+            // If the result is TX_RECONSIDERABLE, add it to m_recent_rejects_reconsiderable
3139+            // because we should not download or submit this transaction by itself again, but may
3140+            // submit it as part of a package later.
3141+            m_recent_rejects_reconsiderable.insert(ptx->GetWitnessHash().ToUint256());

mzumsande commented at 10:30 pm on April 30, 2024:

nit: I think that 6c51e1d7d021ed6523107a6db87a865aaa8fc4c9 changes behavior such that txns which are now in m_recent_rejects_reconsiderable instead of m_recent_rejects will be ignored when calling m_recent_rejects.contains(...). That is probably an undesired change of behavior - however it’s just for 1 commit :man_shrugging:

achow101 commented at 10:31 pm on April 30, 2024: member

ACK e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f

in src/net_processing.cpp:3254 in 87c5c524d6 outdated

3249+    // We currently only expect to process 1-parent-1-child packages. Remove if this changes.
3250+    if (!Assume(package.size() == 2)) return;
3251+
3252+    // No package results to look through for PCKG_POLICY or PCKG_MEMPOOL_ERROR
3253+    if (package_result.m_state.GetResult() == PackageValidationResult::PCKG_POLICY ||
3254+        package_result.m_state.GetResult() == PackageValidationResult::PCKG_MEMPOOL_ERROR) return;

mzumsande commented at 10:37 pm on April 30, 2024:

I wonder if there are situations in which we fail on a package level and therefore return here and never call ProcessInvalidTx() to remove the child from the orphanage. Would that be possible, for example, with v3 transactions that violate PackageV3Checks(), and is there something we could do against it? (or should, maybe it’s not that terrible?).

glozow commented at 8:48 am on May 1, 2024:

PCKG_POLICY doesn’t populate m_tx_results so we wouldn’t have results for those transactions - it’s meant for situations in which the package is invalid, but the individual transactions could be if submitted in a different context. However, going over the failures that are marked as PCKG_POLICY, I agree there’s a few failures that are classified as PCKG_POLICY but are actually more definitively invalid, and we should bubble that up to p2p.

Having both mempool + package v3 parents, v2-spending-v3, and v3-spending-v2 are failures that can’t change in another context, so we can delete the tx from orphanage.

We shouldn’t do this for package-mempool-limits and package-not-child-with-unconfirmed-parents, since there could be a parent with a smaller witness that makes this child acceptable.

The 2 changes we’d make are:

update validation-side things to return a MempoolAcceptResult for the results where we should mark transactions as invalid. Perhaps we can reclassify these failures as PCKG_TX failures, or allow PCKG_POLICY failures to populate m_tx_results. cc @instagibbs ?
update the p2p logic here to continue when it’s PCKG_POLICY and call ProcessInvalidTx for the MempoolAcceptResults we find.

instagibbs commented at 12:40 pm on May 1, 2024:

PCKG_POLICY doesn’t populate m_tx_results so we wouldn’t have results for those transactions

From my reading of the code, package-mempool-limits populates m_tx_results? We also allow those cases in our fuzz target(which I’m guessing is there because I hit it!).

We could probably be more aggressive about filling out results when possible and passing those results along, sounds like future work for now?

glozow commented at 12:46 pm on May 1, 2024:

Then maybe we should just do this?

Update the p2p logic here to continue when it’s PCKG_POLICY and call ProcessInvalidTx for the MempoolAcceptResults we find.

(and remove the Assume that they always exist)

glozow commented at 2:24 pm on May 1, 2024:

^I added this to #30012

achow101 merged this on Apr 30, 2024

achow101 closed this on Apr 30, 2024

mzumsande commented at 10:43 pm on April 30, 2024: contributor

Light ACK e518a8bf8abf3d7b83c9013f56d0dca18ae04d6f I didn’t review the functional tests in much detail, but the p2p code looks good to me.

glozow deleted the branch on May 1, 2024

p2p: opportunistically accept 1-parent-1-child packages #28970

Code Coverage

Reviews

Conflicts