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

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

this doesn't return a boolean

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

fixed

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?

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.

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?

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?

Old behavior should be preserved now

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

fixed

this is never set

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

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

E.g.:

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

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.

Got it, thank you.

added a comment on top of where the package is created

@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?

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.

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

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...

there any science to these choices?

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.

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

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.

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

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.

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

Good time to add documentation to this function

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

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

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

apparently not?

marking as resolved since #29827 does this

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

done

please give some documentation

added

please give some documentation

added

        if (m_recent_rejects_reconsiderable.contains(wtxid)) {

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

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)

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

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

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

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?

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

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

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

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

as well as wtxid just below

done

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

added

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

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

Why? There wouldn't be anything interesting in state

slight readability preference, feel free to ignore

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

we do add orphans to vExtraTxnForCompact though?

unused

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

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?

double-sync for what?

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

had this timeout locally once

can't seem to recreate anymore :shrug:

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?

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.

could this subtest be in its own subroutine

done

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

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

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

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

could stand a unit test and some basic fuzz coverage

added unit tests and fuzz coverage

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

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.

tiny preference for putting this case first

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

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

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

removed

In 5405171740aa77e1eb1110fa8be97318edba380a: nit: could set noban_tx_relay

done

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

removed, thanks

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

        peer_sender = node.add_p2p_connection(P2PInterface())

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

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

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

fixed

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

done

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

done

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

done

quick test case that returns empty would be good too

added

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?

@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):

The only way to create an ATMP result of type `MempoolAcceptResult::ResultType::VALID` is using the
static method `MempoolAccepptResult::Success`, which in turn calls the private successful case constructor of
`MempoolAcceptResult`. This one always sets `m_replaced_transactions`, therefore
`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.

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.

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

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).

Elaborated in commit message

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

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

done, yay for 20

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.

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

think you forgot to push?

think you forgot to push?

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

add a case where:

1. CONSENSUS-invalid child is propagated to peer
2. low_fee parent is given
3. package evaluation is attempted
4. peer is disconnected

to ensure we're handling invalid tx properly

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.

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

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.

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

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

added something similar

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

this is also causing the sanitizer run failure

Woops yes will fix

newline?

(deleted GetCombinedHash)

newline?

(deleted GetCombinedHash)

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.

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.

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

updated

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).

Done, and edited the logging a bit

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

added

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.

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).~

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?

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?

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?

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.

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

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.

diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
index 603cbf08a9..1c887289dc 100755
--- a/test/functional/p2p_opportunistic_1p1c.py
+++ b/test/functional/p2p_opportunistic_1p1c.py
@@ -164,78 +164,129 @@ class PackageRelayTest(BitcoinTestFramework):
         # itself and with a child. This is necessary, otherwise high_fee_child can be censored.
         parent_txid_int = int(low_fee_parent["txid"], 16)
         peer_sender.wait_for_getdata([parent_txid_int])
 
         # 7. The low feerate parent + high feerate child are submitted as a package.
         peer_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))
 
         # 8. Both transactions should now be in mempool
         node_mempool = node.getrawmempool()
         assert low_fee_parent["txid"] in node_mempool
         assert high_fee_child["txid"] in node_mempool
 
         node.disconnect_p2ps()
 
     def test_orphan_consensus_failure(self):
         node = self.nodes[0]
         low_fee_parent = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
         coin = low_fee_parent["new_utxo"]
         address = node.get_deterministic_priv_key().address
         # Create raw transaction spending the parent, but with no signature (a consensus error).
         hex_orphan_no_sig = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["value"] - Decimal("0.0001")})
         tx_orphan_bad_wit = tx_from_hex(hex_orphan_no_sig)
         tx_orphan_bad_wit.wit.vtxinwit.append(CTxInWitness())
         tx_orphan_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
 
-        peer_sender = node.add_p2p_connection(P2PInterface())
+        child_peer_sender = node.add_p2p_connection(P2PInterface())
+        parent_peer_sender = node.add_p2p_connection(P2PInterface())
 
         # 1. Child is received first. It is missing an input.
         child_wtxid_int = int(tx_orphan_bad_wit.getwtxid(), 16)
-        peer_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=child_wtxid_int)]))
-        peer_sender.wait_for_getdata([child_wtxid_int])
-        peer_sender.send_and_ping(msg_tx(tx_orphan_bad_wit))
+        child_peer_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=child_wtxid_int)]))
+        child_peer_sender.wait_for_getdata([child_wtxid_int])
+        child_peer_sender.send_and_ping(msg_tx(tx_orphan_bad_wit))
 
         # 2. Node requests the missing parent by txid.
         parent_txid_int = int(low_fee_parent["txid"], 16)
-        peer_sender.wait_for_getdata([parent_txid_int])
+        child_peer_sender.wait_for_getdata([parent_txid_int])
 
-        # 3. Sender relays the parent. Parent+Child are evaluated as a package and accepted.
-        peer_sender.send_message(msg_tx(low_fee_parent["tx"]))
+        # 3. "Honest" sender relays the parent. Parent+Child are evaluated as a package and accepted.
+        parent_peer_sender.send_message(msg_tx(low_fee_parent["tx"]))
 
         # 4. Transactions should not be in mempool.
         node_mempool = node.getrawmempool()
         assert low_fee_parent["txid"] not in node_mempool
         assert tx_orphan_bad_wit.rehash() not in node_mempool
 
         # 5. Peer sent a consensus-invalid transaction.
-        peer_sender.wait_for_disconnect()
+        child_peer_sender.wait_for_disconnect()
+
+        # 6. "Honest" peer unpunished
+        parent_peer_sender.sync_with_ping()
+
+    def test_parent_consensus_failure(self):
+        node = self.nodes[0]
+        low_fee_parent = self.wallet.create_self_transfer(fee_rate=FEERATE_1SAT_VB, confirmed_only=True)
+
+        # Add bad sigature to parent
+        tx_parent_bad_wit = tx_from_hex(low_fee_parent["hex"])
+        tx_parent_bad_wit.wit.vtxinwit.append(CTxInWitness())
+        tx_parent_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
+
+        coin = low_fee_parent["new_utxo"]
+        address = node.get_deterministic_priv_key().address
+        # Create raw transaction spending the parent, but with no signature (a consensus error).
+        hex_orphan_no_sig = node.createrawtransaction([{"txid": coin["txid"], "vout": coin["vout"]}], {address : coin["value"] - Decimal("0.0001")})
+        tx_orphan_bad_wit = tx_from_hex(hex_orphan_no_sig)
+        tx_orphan_bad_wit.wit.vtxinwit.append(CTxInWitness())
+        tx_orphan_bad_wit.wit.vtxinwit[0].scriptWitness.stack = [b'garbage']
+
+        child_peer_sender = node.add_p2p_connection(P2PInterface())
+        parent_peer_sender = node.add_p2p_connection(P2PInterface())
+
+        # 1. Child is received first. It is missing an input.
+        child_wtxid_int = int(tx_orphan_bad_wit.getwtxid(), 16)
+        child_peer_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=child_wtxid_int)]))
+        child_peer_sender.wait_for_getdata([child_wtxid_int])
+        child_peer_sender.send_and_ping(msg_tx(tx_orphan_bad_wit))
+
+        # 2. Node requests the missing parent by txid.
+        parent_txid_int = int(tx_parent_bad_wit.rehash(), 16)
+        child_peer_sender.wait_for_getdata([parent_txid_int])
+
+        # 3. Parent sender relays the parent. Only parent should be evaluated.
+        parent_peer_sender.send_message(msg_tx(tx_parent_bad_wit))
+
+        # 4. Transactions should not be in mempool.
+        node_mempool = node.getrawmempool()
+        assert tx_parent_bad_wit.rehash() not in node_mempool
+        assert tx_orphan_bad_wit.rehash() not in node_mempool
+
+        # 5. Peer sent a consensus-invalid transaction.
+        parent_peer_sender.wait_for_disconnect()
+
+        # 6. Child-sending peer unpunished for now!
+        child_peer_sender.sync_with_ping()
 
     def run_test(self):
         node = self.nodes[0]
         self.wallet = MiniWallet(node)
         self.wallet_nonsegwit = MiniWallet(node, mode=MiniWalletMode.RAW_P2PK)
         self.generate(self.wallet_nonsegwit, 10)
         self.generate(self.wallet, 20)
 
         fill_mempool(self, node, self.wallet)
 
         self.log.info("Check opportunistic 1p1c logic when parent (txid != wtxid) is received before child")
         self.test_basic_parent_then_child(self.wallet)
 
         self.log.info("Check opportunistic 1p1c logic when parent (txid == wtxid) is received before child")
         self.test_basic_parent_then_child(self.wallet_nonsegwit)
 
         self.log.info("Check opportunistic 1p1c logic when child is received before parent")
         self.test_basic_child_then_parent()
 
         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid != wtxid)")
         self.test_low_and_high_child(self.wallet)
 
         self.log.info("Check opportunistic 1p1c logic when 2 candidate children exist (parent txid == wtxid)")
         self.test_low_and_high_child(self.wallet_nonsegwit)
 
         self.log.info("Check opportunistic 1p1c logic with consensus-invalid orphan causes disconnect")
         self.test_orphan_consensus_failure()
 
+        self.log.info("Check opportunistic 1p1c logic doesn't evaluate package with consensus-invalid parent")
+        self.test_parent_consensus_failure()
+
 
 if __name__ == '__main__':
     PackageRelayTest().main()

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

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

done

In: 54d0c78d104fb5412a194816590a06cad8cadf80

This seems redundant

deleted

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.

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.

Done in #30012

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.

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?

great idea, will change 👍

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.

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.

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: 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?

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: 54d0c78d104fb5412a194816590a06cad8cadf80

Calling ToUint256() shouldn't be needed

done, thanks

In: 54d0c78d104fb5412a194816590a06cad8cadf80

Same as before, no need to cast to ToUint256

done, thanks

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)

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

In: 54d0c78d104fb5412a194816590a06cad8cadf80

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

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

(deleted GetCombinedHash)

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

done

in: 91f4efa420958a93f4620379f8830231f276b23b

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

done

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.

removed

duplicated cleanup, or something?

Ah forgot to delete, yes

deleted

duplicate log?

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

In 5c8aa657642ef24e711a73c28278644f14117d73

This last check is still redundant

deleted

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 (?)

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 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

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 42859548ab5aebf40da6089b85065f7c204b992a

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

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

deleted

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.

done

nit

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

Done. Also refactored orphanage_tests to be more readable.

nit

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

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.

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 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).

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

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.

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.

Edited the comment to say this is a probably uncommon case

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?

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).

I guess it'll likely be something like:

1. peer A sends child, child is put in orphanage and parent tx request queued(but not yet sent)
2. 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

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

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

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

I guess it'll likely be something like:

1. peer A sends child, child is put in orphanage and parent tx request queued(but not yet sent)
2. 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.)

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.

Ok, marking this as resolved

is there a constant we can put here?

Added

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.

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

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