validation: mempool validation and submission for packages of 1 child + parents #22674

I have no clue about details of packages and related PRs. Interested in this RPC which can be helpful in my tests. Is this like broadcasting transactions in batches?

0for (int i = 0; i < MAX; i++) {
1  if (consensus and mempool policies) {
2  // add to broadcast queue
3} else {
4  // Error
5}
6}
7
8//BroadcastTransaction(node, queue);

Is it possible to select peer for broadcasting this package? Not sure if enough people will ever agree to add such option in sendrawtransaction but we can experiment with test RPCs if its easy to implement. Context: #21876

So RPC can have one argument to mention peer address.

Is this like broadcasting transactions in batches?

No, but you can make multiple calls to sendrawtransaction

Is it possible to select peer for broadcasting this package?

No, I don’t think that would be very helpful here. If you’re interested in enabling specific functionality, perhaps open a PR?

At some point we will have a P2P protocol for packages which we want to punish peers for violating, so we definitely want to distinguish between a network-wide rule violation and mempool rejection based on our local policy rules. In other words, this distinction mainly informs the net processing layer about the misbehavior of peers.

Within this PR, we obviously haven’t defined P2P stuff yet, but I am attempting to formally specify what the rules are.

So these are all PCKG_BAD and, in the future, we’d want to punish a peer that gives us something like this:

• more than 25 transactions
• transactions aren’t sorted
• there are conflicts within the package

But these would be PCKG_POLICY:

• package mempool ancestor/descendants exceed our limits
• package feerate is below our mempool minimum

So these are all PCKG_BAD and, in the future, we’d want to punish a peer that gives us something like this:

I think we’re never discouraging based on tx-relay or mempool policy violations because our peers can run a policy far different than ours ? Like a future version of Core could have package with more than 25 component, a old peer connected would punish that peer for a behavior valid according to the relay rules of the former.

Discouraring/punishing on the rational of policy violations could trigger netsplits and I think that’s why we’re never marking a peer as Misbehaving in MaybePunishNodeForPeer for policy failure reasons.

However, I could see that a ill-crafted package could be punished in the sense that MAX_PEER_PACKAGE_ANNOUNCEMENTS is dynamically constrained. But that would mean we have a stable package format across p2p package versions, which sounds a bit early to commit on IMO.

Likely we need to think more about it, I would say for this PR it’s a bit premature to introduce PCKG_BAD though if you think otherwise maybe better to document it’s for future p2p purposes.

How are you aiming to use exact=true ?

I understand that if you’re aiming to submit the package to your local mempool, you know the state of it and you can adjust the package components to only what is needed (exact=false).

However, if you submit a package over the p2p network, one can’t assume a knowledge of the network mempools. A missing or replaced parent fails the propagation of the package. For this reason, a package broadcaster should always submit exact packages

I’m just thinking we should keep the acceptance paths unified between submitting a package over RPC and submitting a package over p2p.

Correct, I expect all unconfirmed parents to be present, and P2P should be talking about full packages.

At some point, I was hoping to be able to statically verify that a transaction is exactly child-with-parents by requiring transactions for all of the child’s inputs to be present, but I realized we have to allow the parents to confirm earlier than the child, and it’s not reasonable to expect those confirmed parents to continue being relayed.

Sooooo yeah I’ll remove it

Maybe doc/policy or doc/txrelay ? I guess we’ll have another doc for RBF revamp.

Related #22806

nit:

0A **child-with-unconfirmed-parents** package is a topologically sorted package that consists of

Does that mean that if I have:

• tx1 in the mempool
• tx2 and tx3 not yet published
• childTx spending tx1, tx2 and tx3

A package containing [tx2, tx3, childTx] is invalid? Do I need to put tx1 in that package? I plan on potentially funding CPFP children with unconfirmed inputs, that means I’d need an extra step to insert them in the package: but if they made it to the mempool on their own, it seems unnecessary, isn’t it?

If the package [tx2, tx3, childTx] is valid, we should probably update the phrasing here to:

0The last transaction in the package is the child; each of its inputs must refer to a UTXO in the
1current chain tip, the mempool or some preceding transaction in the package.

Yes, based on these rules, you need to put tx1 in the package. I believe this is better because we can relay packages across the network without worrying about differences in mempool contents.

I can see how it’s inconvenient to need to track the other parents when you’re submitting to your node. One thing I can offer is adding a bit of code to our client interface code to “fill in” the package using mempool contents before submitting via ProcessNewPackage - that should be easy enough to do.

I can see how it’s inconvenient to need to track the other parents when you’re submitting to your node. One thing I can offer is adding a bit of code to our client interface code to “fill in” the package using mempool contents before submitting via ProcessNewPackage - that should be easy enough to do.

That would be great!

Just to make sure I didn’t mess something up: I was able to do package RBF from eclair based on your package-mempool-accept branch, was that expected?

I had (localCommitTx, localAnchorTx) in my mempool, and I was able to replace it with (remoteCommitTx, remoteAnchorTx) that paid more fees (with both commit txs paying 0 fee).

I think we should keep the package API “hard-to-misuse” for the user.

IIUC, this PR introduce a second definition of a package, divergent of the testmempoolaccept/AcceptMultipleTransactions one. This second definition is more restrictive as the package must be a child with all of its unconfirmed parents and sorted.

If a user sanitizes a package through testmempoolaccept with a 2-parent, 2-child to verify policy correctness. Then calls submitpackage, this second RPC should fail while the package topology isn’t changed.

I think this can be concerning for L2 with pre-signed transactions, as this illegal package might be the only valid state owned by the L2 node to enforce her balance.

I wonder if this check doesn’t wrongly block some replacement.

Let’s say you have A + B already in the mempool, where B is a child of A. Then I submit A’ + B, where A’ is a same-txid-different-witness replacement candidate of A, with an improved feerate. A’+B should replace A + B, however it will be rejected as B is already in the mempool ?

Note that this is AcceptMultipleTransactions and not AcceptPackage.

I’m referring to the check inPreChecks(), where it returns failure if the transaction is already in the mempool. So AcceptMultipleTransactions expects the txns passed in to not already be in the mempool. I think it’s important to state this precondition in the function documentation.

On the other hand, AcceptPackage will allow transactions to already be in the mempool, and will de-duplicate them before passing them to AcceptMultipleTransactions (not included in this PR, but see #22290).

That’s right, it’s AcceptMultipleTransactions. I agree it’s important to state requirements in function documentation.

Note as a nit outside the scope of this PR, testmempoolaccept documentation doesn’t seem to reflect the not-already-in-mempool condition “If multiple transactions are passed in, parents must come before children and package policies apply: the transactions cannot conflict with any mempool transactions or each other”

“package policies apply: the transactions cannot conflict with any mempool transactions or each other”

https://github.com/bitcoin/bitcoin/blob/8ae4ba481ce8f7da173bef24432729c87a36cb70/src/rpc/rawtransaction.cpp#L875-L876

Well, the base feerate of a package component is still offered to the miner, even if this component should not have been accepted in the mempool as a single tx ?

I think a stronger rational is the lack of p2p packages for now, preventing a package to effectively propagate to miner mempools and as such be considered as a valid blockspace demand ?

Outdated name:

0 * See doc/policy/packages.md for documentation of package validation rules.

I’d suggest not using the word “static” here, which already has multiple meanings in c++ that don’t apply here. I think “context-free” would be better.

I’d also suggest changing the commit log from [packages] add static IsChildWithParents function to [packages] add context-free IsChildWithParents function

0        CMutableTransaction mtx_parent_also_child;

to match the naming below

Other possible unit tests (not necessarily in this PR):

• too large package (26 txs)
• too heavy package
• conflicting txs in package
• duplicate txs in package (with same and different witnesses)

Now that they’ve been moved to their own function in packages.h, we can make the tests in txvalidation_tests.cpp simpler unit tests and move them to txpackage_tests.cpp.

Added this in #23381

I think it’s better to continue using std::set::empty rather than implicitly casting the int to a bool:

0    ws.m_replacement_transaction = !ws.m_conflicts.empty();

nit: the lock annotation shouldn’t be aligned with the open parens (since it’s not contained in the parens):

0    bool FinalizePackage(const ATMPArgs& args, std::vector<Workspace>& workspaces, PackageValidationState& package_state,
1                         std::map<const uint256, const MempoolAcceptResult>& results)
2        EXCLUSIVE_LOCKS_REQUIRED(cs_main, m_pool.cs);

nit: I found the similar comment in packages.cpp clearer: “The package is expected to be sorted, so the last transaction is the child.”

I would clarify it to:

0    // The package is expected to be sorted, so all transactions except the last one are the unconfirmed parents

In 40d3f98e7b3d9630f99efe06fba1f6a6a21794b5 “[policy] require submitted packages to be child-with-unconfirmed-parents”

ISTM this isn’t that the transaction conflicts with the mempool but rather that it already exists in the mempool? A similar check is done in PreChecks where the wtxid is also checked. Should the wtxid be checked here as well?

In 40d3f98e7b3d9630f99efe06fba1f6a6a21794b5 “[policy] require submitted packages to be child-with-unconfirmed-parents”

Why must non-package inputs be confirmed? From my understanding of AcceptMultipleTransactions, it would be fine to have transactions which have unconfirmed inputs so long as they are already in the mempool.

In fd3dbcc7c74eb9da6631c217893c325266bbf2e1 “[validation] full package accept + mempool submission”

Perhaps mention that this value is unused, so it is ok for it to be false? At first, this seemed incorrect since testing package acceptance should have the same parameters as actual package acceptance (modulo adding to the mempool).

In fd3dbcc7c74eb9da6631c217893c325266bbf2e1 “[validation] full package accept + mempool submission”

If these are never supposed to fail, then shouldn’t these be an assertion rather than returning a failure?

nit: since we’ve already asserted that package.size() >= 2, I think this can be slightly simplified to

0    return std::all_of(package.cbegin(), package.cend() - 1,
1                       [&input_txids](const auto& ptx) { return input_txids.count(ptx->GetHash()) > 0; });

also in validation.cpp

nit: is there a reason for not just using the Package alias?

0        Package package;

nit: I think you meant to say “Unrelated transactions”?

0    // Unrelated transactions are not allowed in package submission.

I checked the rules present in ProcessPackage/AcceptPackage/AcceptMultipleTransactions. There are 2 package policies I can see missing.

The first one is the “package memory usage too high”. That being said this check is host-dependent so it would be too artificial to expect users to respect it. In practice, you will hit first MAX_PACKAGE_SIZE.

The second one is the package must be at least 2-txn sized, otherwise you will fail on IsChildWithParents first check. It might be obvious from the “child-with-unconfirmed-parents packages” definition, though good to explicit requirement.

Do you want to add to this specification that it’s “unstable” or “work-in-progress” and shouldn’t be strongly rely on by users for now ?

Also I recommend to add a safety warning for the “Packages must be child-with-unconfirmed-parents packages” rule, such as “Please note the usage of multiple-parents one-child might be unsafe for your use-case and should be weigh in with care” (see previous comment).

1. I’ve removed the “package memory usage too high” check
2. Added a comment that packages must contain at least 2 transactions
3. Added a disclaimer that the policy docs aren’t exhaustive
4. Added a warning that multi-parent-1-child may be unsafe for some use cases

IIUC the design rational behind this check is to prevent unbounded mempool growth in-between individual transaction acceptance and final call to LimitMempoolSize ?

Intuitively, I would say package memory usage is already bounded by MAX_PACKAGE_SIZE and MAX_STANDARD_TX_WEIGHT, even if we have to assume malloc overhead, total_memory_usage should stay correlated enough to total vsize, to not have to introduce a new check ?

What are the reasons for a package transaction to not exist in the mempool at that step ?

If it has failed the script checks, we should have marked all_submitted as false and returned early. Asking the question because FinalizePackage comment explicitly says that package acceptance should have been atomically or not (“either all of the transactions are added or none”) .

Maybe the sentence could be better phrased than “make sense”, like “As submitted package are ultimately upper bounded by in-mempool package limits, ensure that package acceptance limits are encompassed by the in-mempool ones” ?

Note, the mempool package limits can be adjusted by the node settings, e.g size=100. In that case, we refuse package, of which the size has been considered as acceptable by the node operator. I think it’s not a big deal, as those settings are rarely tweaked, but maybe batching applications are concerned ?

Changed to:

// If a package is submitted, it must be within the mempool’s ancestor/descendant limits. Since a // submitted package must be child-with-unconfirmed-parents (all of the transactions are an ancestor // of the child), package limits are ultimately bounded by mempool package limits. Ensure that the // defaults reflect this constraint.

Note, I think the error string is a little bit confusing here.

The error sounds to suggest the package is invalid due to not being a child with unconfirmed parents. However, a package can be valid if parents are in different state than unconfirmed transactions in the mempool, namely already included in the chain tip. Reading back packages.md, the fact that the parents inputs can be already available in the UTXO set is laid out in the “Definitions” section, but not in the “Rules” ones. A hurried user could think a package must only be made of unconfirmed parents, a restriction on the utility of the feature.

To reduce risks of confusion, I think the confirmed-parents allowance could be mentioned in the “Rules” section for redundancy and the message error here could be “missing-parents-from-package-or-utxo-set”.

However, a package can be valid if parents are in different state than unconfirmed transactions in the mempool, namely already included in the chain tip.

This is not the case. If the package contains a parent that is already confirmed, it is an invalid package. This is because we don’t want to rescan previous blocks for the parent transaction (we might not have them if we’re pruning); we only use the UTXO set.

A hurried user could think a package must only be made of unconfirmed parents, a restriction on the utility of the feature.

This would be correct. A package must only have unconfirmed parents.

This is not the case. If the package contains a parent that is already confirmed, it is an invalid package.

My mistake, I should have precised, “a package can be valid if the child parents are in different state than unconfirmed transactions in the mempool, namely already included in the chain tip” so we agree here.

This would be correct. A package must only have unconfirmed parents.

Yes, though the package child is allowed to have not-in-the-package-confirmed-parents. I was intending to hightlight that packages.md is not explicit on that. I think it can be precised later.

While I agree on the end goal to avoid introducing tx-relay censorship vector, I disagree on assigning mempool policies heterogeneity as the root cause. If a user decides for a highly-restrictive policy (e.g bumping mintxrelayfee), as protocol developers, there is not that much we can do to prevent such “self”-censorship.

What we would like to avoid is offering a naive pinning trick to a malicious counterparty in the context of time-sensitive multi-party protocols. E.g, in Lightning, a counterparty catches at early propagation a commitment transaction A from the holder, attaches a low-feerate child B’ on its own anchor output, then propagates widely this malicious package by mass-connecting. When the honest package A + B is reaching the majority of the network, it fails package acceptance if we don’t have a relaxation to re-evaluate package even if a subset of components are already in the mempool.

Would this be a more comprehensive comment?

“Node operators are free to set their mempool policies however they please, nodes may receive transactions in different orders, and malicious counterparties may try to take advantage of policy differences to pin transactions. As such, it’s possible for some package transaction(s) to already be in the mempool, and we don’t want to reject the entire package in that case (as that could be a censorship vector).”

may try to take advantage of policy differences to pin transactions

“or transactions propagation delays” (i.e bypass _INVENTORY_BROADCAST_INTERVAL timers)"

Otherwise, far better, good to me.

I think this is a behavior unsafe for the unwarranted user.

In the context of multi-party protocol, a malicious counterparty broadcast a transaction A’ with an inflated witness. When the package A+B issued by a honest participant is entering into the mempool, A’ is swapped in place of A. However, A has a far smallest witness, thus swapping for A’ effectively downgrades the package feerate. This breaks the fee-bumping expectation of the honest participant, as a malleated package is now propagating on the network, instead of the higher one.

As of today, I don’t know deployed protocols which would be affected by this package feerate-downgrade issue. That said, with the activation of Taproot it’s now far easier to introduce applications/protocols vulnerable, as shared-owners of a utxo might have witnesses unfairly asymmetric in weight due to the script path spend control block.

I assume this behavior should be corrected if witness replacement is implemented. However, I think we shouldn’t make assumptions it’s done before package relay. In the meantime, I think it’s better to not introduce unsafe or obscure behaviors, of which the curation might be missed by future reviewers. I would say it’s better to just fail package acceptance if a same txid, different wtxid already exists in the mempool (same behavior than single-transaction acceptance in PreChecks).

In the context of multi-party protocol, a malicious counterparty broadcast a transaction A’ with an inflated witness. When the package A+B issued by a honest participant is entering into the mempool, A’ is swapped in place of A. However, A has a far smallest witness, thus swapping for A’ effectively downgrades the package feerate.

As specified in the proposal: “We use the package feerate of the package after deduplication” which is intended to avoid an issue like this, and others. More rationale is included in the proposal.

This breaks the fee-bumping expectation of the honest participant, as a malleated package is now propagating on the network, instead of the higher one.

Given that the feerate of A’ was sufficient to enter the mempool, this is made no worse or better with the introduction of package mempool accept, as we’re treating A’ vs A identically to how we treat individual transactions. We will deduplicate A from the package and assess B individually; its package feerate only includes the transactions that are not already in the mempool. Of course this means the ancestor score of B once everything is submitted is lower, but the danger has not been made worse by package mempool acceptance, as we would have the same result if B was received as an individual transaction.

I assume this behavior should be corrected if witness replacement is implemented.

Yes exactly. I still stipulate that package RBF and witness replacements can be worked on in parallel; both are improvements but neither requires the other. I’ve tried to document the limitation (known to both single transactions and packages) in the code, as seen in this diff.

I would say it’s better to just fail package acceptance if a same txid, different wtxid already exists in the mempool (same behavior than single-transaction acceptance in PreChecks).

I think this is worse than the bloated-witness problem acknowledged above. If A’ is already in the mempool and:

• A and B are received separately as individual transactions -> A is rejected and B is accepted
• A+B is received as a package -> package fails, B is rejected.

This makes package acceptance more strict than individual transaction acceptance, which is something we want to avoid.

I agree with @glozow’s proposed behavior to use the existing mempool transaction (for now) in case it has a different wtxid. From what I see, the worse that can happen if that the sender proposed a package with feerate F1, and it ends up having feerate F2 < F1.

A sender that doesn’t see its transaction included should keep bumping the child, so it should eventually confirm, it will just cost more than expected, but I don’t think it introduces a fundamental attack vector (at least I currently don’t see one).

And we can later implement replacement to choose between A and A’ the one with the smallest weight (which is incentives compatible as it pays a higher feerate).

As specified in the proposal: “We use the package feerate of the package after deduplication” which is intended to avoid an issue like this, and others. More rationale is included in the proposal.

Yes, I’ve browsed against the gist and it doesn’t precise in details how the deduplication is operated, at least on the witness replacement case (I think ?).

Given that the feerate of A’ was sufficient to enter the mempool, this is made no worse or better with the introduction of package mempool accept, as we’re treating A’ vs A identically to how we treat individual transactions. We will deduplicate A from the package and assess B individually; its package feerate only includes the transactions that are not already in the mempool. Of course this means the ancestor score of B once everything is submitted is lower, but the danger has not been made worse by package mempool acceptance, as we would have the same result if B was received as an individual transaction.

Yes, I agree package mempool accept hasn’t worsen that case compared to the individual transaction processing.

Note, my comment was about the future when package relay is supported, if we forget to operate incentives compatibles witness replacement in the case of incoming A’ smaller than already in-mempool A. If we p2p relay the with-dedup-transactions package, that would constitute an attack vector as A + B have a smaller feerate than A’ + B.

Currently, this is the code behavior though I think we all agree to correct that in the future. I think it’s nice to either update the gist or leave a more detailed comment in the code, and thus ease future review/implementation work.

Yes exactly. I still stipulate that package RBF and witness replacements can be worked on in parallel

I don’t fundamentally disagree on working them in parallel, beyond the scarcity of review bandwidth.

This makes package acceptance more strict than individual transaction acceptance, which is something we want to avoid.

Actually, I think package acceptance is stricter than individual transaction acceptance, at least at package limits evaluation (CheckPackageLimits). User-wise, I still think it’s better to fail loudly than silently accept a mutant, though if we can’t agree here, maybe at least warn back the user about the wtxid swap ?

As future package components should be evaluated on the union of the fees instead of the transaction ones to decide acceptance, uncareful deduplication could provoke unexpected package submission failures.

Let’s say you have the package A+B+C, where A and B are parents of C, A and C are low-feerate, B is high-feerate. If B is already in the mempool, when the package A+B+C is received, B is dedup, then the union of fees of A+C is realized, it’s too low to satisfy the mempool min fee, the packages is rejected.

To avoid this behavior, we should compute the union of fees pre-dedup in AcceptPackage, then pass down the result to AcceptMultipleTransactions. It’s open if future testmempoolaccept of multiple transaction should be also based on the union of fees, or we think it’s an acceptable divergence w.r.t package submission. I think we should have a TODO pointing to that context.

Further, I think deduplication is also introducing issues w.r.t ancestors/descendants limits. If we decide to relay the package on the positive result from AcceptMultipleTransactions, a dedup package might have lower ancestors/descendants limits than the complete one we announce to our peers.

Let’s say you have the package A+B+C, where A and B are parents of C, A and C are low-feerate, B is high-feerate. If B is already in the mempool, when the package A+B+C is received, B is dedup, then the union of fees of A+C is realized, it’s too low to satisfy the mempool min fee, the packages is rejected. To avoid this behavior, we should compute the union of fees pre-dedup in AcceptPackage, then pass down the result to AcceptMultipleTransactions.

It is actually incentive-compatible to reject this package. B’s fees should not supplement A’s fees, because B does not depend on A; including B in the package feerate would be “sibling-pays-for-sibling” behavior. I’ve illustrated this in examples Q1 and Q2.

Further, I think deduplication is also introducing issues w.r.t ancestors/descendants limits. If we decide to relay the package on the positive result from AcceptMultipleTransactions, a dedup package might have lower ancestors/descendants limits than the complete one we announce to our peers.

No, we’ll still enforce ancestor/descendant limits on the package in the same way. The package transactions that are already in the mempool will be included in ancestor counts. The nice thing about a child-with-parents package is that all package transactions are ancestors of the child transaction so we won’t under/overestimate :).

Since we always have all of the unconfirmed parents in the package, mempool contents won’t affect whether our package is within the limits specified by the protocol.

For example, if we can’t have more than 25 in a package but somebody has a package where the child has 30 in-mempool ancestors, regardless of the size of the package after deduplication, the package is defined as child-with-all-unconfirmed-parents. Thus, since it has 30 unconfirmed ancestors, the defined package has a size of 31 and is too big.

It is actually incentive-compatible to reject this package. B’s fees should not supplement A’s fees, because B does not depend on A; including B in the package feerate would be “sibling-pays-for-sibling” behavior.

I agree with that. @ariard if you also agree with that, is the rest of your comment resolved? Or is there a part I’m missing?

It is actually incentive-compatible to reject this package. B’s fees should not supplement A’s fees, because B does not depend on A; including B in the package feerate would be “sibling-pays-for-sibling” behavior. I’ve illustrated this in examples Q1 and Q2.

You’re right, that wouldn’t be incentive-compatible. It also points that a naive implementation of fee union would be flawed, and that packages fees should be computed in topological descending order.

No, we’ll still enforce ancestor/descendant limits on the package in the same way. The package transactions that are already in the mempool will be included in ancestor counts. The nice thing about a child-with-parents package is that all package transactions are ancestors of the child transaction so we won’t under/overestimate :).

Right, at the individual transaction level. At the package-level, in CheckPackageLimits, which is processed after deduplication, a parent transaction is accounted both as the child ancestor and as a package member in the check against limitAncestorCount (L271, in src/txmempool.cpp). If we prune the package size at deduplication, we might pass that check, which would be fail by a peer with a mempool, lacking the dedup elements ?

At the package-level, in CheckPackageLimits, which is processed after deduplication, a parent transaction is accounted both as the child ancestor and as a package member in the check against limitAncestorCount (L271, in src/txmempool.cpp). If we prune the package size at deduplication, we might pass that check, which would be fail by a peer with a mempool, lacking the dedup elements ?

Nope, this is a fair concern, but I made sure to make it impossible for a package transaction to be double-counted after de-duplication. See this assertion that transactions passed in to CheckPackageLimits() must not be in the mempool:

https://github.com/bitcoin/bitcoin/blob/8c0bd871fcf6c5ff5851ccb18a7bc7554a0484b0/src/validation.cpp#L931-L936

Yes my concern is if we relay the de-dup transactions as part of the p2p package we announce to our peer. A package A is evaluated as valid by Alice because she is dedup’ing half-of-the-package due to already in-mempool transaction. The whole package is relayed to Bob, who has an empty mempool. The whole package is evaluated and fails against CheckPackageLimits.

That said, I’m making assumptions on the p2p package mechanism, which has not been yet formalized. I’m making a mental note and I would say let’s think about it more when p2p is introduced. We can still correct AcceptPackage back then.

Essentially yes, so that we can handle same-txid-different-witness. It’s “if you can’t find it by wtxid then try txid because it may have been swapped out.”

I also considered adding a special result type like ResultType::WITNESS_SWAPPED to communicate “try again using txid” to the caller (would that be better?).

Another approach is to include the entire transaction in the MempoolAcceptResult, but it seems weird if the result transaction’s wtxid doesn’t match the key wtxid.

It’s a little ugly, but I’m not sure if there’s a cleaner way to do this?

I also considered adding a special result type like ResultType::WITNESS_SWAPPED to communicate “try again using txid” to the caller (would that be better?).

It does feel better to me, but there’s a part of your answer I don’t understand. This case only happens when the mempool contain a “same txid, different wtxid” entry, right? Let’s call A our proposed tx and A’ the mempool “same txid, different wtxid” tx. Why can’t you simply use A wtxid as the key in the m_tx_results map (even if A’ is actually used)? Why do you want the caller to try again? If the caller sees that the result type is ResultType::WITNESS_SWAPPED this should give them all the information they need?

In e12fafda2dfbbdf63f125e5af797ecfaa6488f66 “[validation] de-duplicate package transactions already in mempool”

Shouldn’t this be txid since if wtxid were in the mempool, the previous if would be the path taken?

0            auto iter = m_pool.GetIter(txid);

There also does not appear to be any tests for this case.

Perhaps:

0    assert(package.size() > 1);
1    const auto& child = package.back();

The size of the package has been checked in IsChildWithParents(), but good to check here as a reminder to the developer and in case IsChildWithParents() is ever changed. Alternatively:

0    // package size must be > 1 from check in `IsChildWithParents()`
1    const auto& child = package.back();

I don’t like this call into Finalize() from FinalizePackage(). You’ve essentially added a boolean parameter to Finalize() (by adding m_package_submission to ATMPArgs), that controls which parts of Finalize() are run. That’s usually a bit of a red flag.

Currently, the only part of Finalize() that you need is the call to addUnchecked() (transaction replacement is currently disabled for packages, and validForFeeEstimation and LimitMempoolSize() are gated on m_package_submission). I think you should just bring the call to addUnchecked() directly into FinalizePackage().

It looks to me like calling addUnchecked() (via Finalize()) for a transaction that’s already in the mempool will cause problems (in addUnchecked(), mapTx.insert() will return the existing mempool transaction, and then various state will be incorrectly updated).

I think the later commit e12fafda2dfbbdf63f125e5af797ecfaa6488f66 [validation] de-duplicate package transactions already in mempool prevents this by deduplicating transactions in the package. However, that’s not at all obvious since the checking code is quite removed from this call. I’d suggest:

• Assume() that the transaction is not in the mempool here (and don’t call addUnchecked() if it is)
• inside addUnchecked(), assert that the transaction is not in the mempool.

Thanks for your feedback! I actually don’t think it’d be cleaner to move addUnchecked() to that function even though it’s the only package-related thing that’s done in Finalize() right now. It repeats code, and will be worse in the future: we’ll also need to remove replacements later with package RBF (and flag valid for fee estimation with package relay). The boolean has 2 uses now, and I intend to use it later for fee-related logic too.

That being said, I can see how the calling relationships are different from expectations, so I’ll renamed FinalizePackage to SubmitPackage in the followup. Hopefully that makes it more clear that it’s not supposed to be analogous to Finalize().

It’s actually prevented from the get-go, since AcceptMultipleTransactions() rejects the whole list of transactions if there are any that already exist in the mempool. The de-duplication logic allows us to only pass the not-already-in-mempool transactions.

I agree it’s helpful and more future-proof to add some sanity checks, since calling addUnchecked() on a transaction that’s already in the mempool would be awful.

Is there a good reason that ConsensusScriptChecks() is inside FinalizePackage() when validating a package, but called before Finalize() when validating a single transaction? I think it’d be clearer if there was symmetry between the two (i.e. ConsensusScriptChecks() is called directly from AcceptMultipleTransactions()).

There’s also the inconsistency that testmempoolaccept for a single transaction will call ConsensusScriptChecks(), but won’t call ConsensusScriptChecks() for packages.

We need to call ConsensusScriptChecks() + submit for each transaction in sequence (and cannot call all the ConsensusScriptChecks()s together, then submit them all together), because of the sanity checks inside ConsensusScriptChecks() that all coins are available. So unless we put all of the logic from FinalizePackage() into AcceptMultipleTransactions(), I don’t think it’s possible to make these patterns the same.

However, I’m happy to rename it to SubmitPackage() or something so it doesn’t seem like there’s supposed to be a pattern. AFAIK ConsensusScriptChecks() is really part of submission logic.

Maybe:

0        std::string unused_err_string;

CTxMemPool::GetIter() takes a txid, so this lookup will fail if wtxid != txid.

Does it? I think since mapTx indexes by txid and wtxid, it might allow both? The test passes when txid != wtxid. Either way, thanks for the good catch, should probably make that interface clearer…

In fact, I think for now you can just search for the txid using m_pool.GetIter(txid) and collapse the two cases (already in mempool, same-txid-different-wtxid in mempool) into one.

Even though the two cases are treated the same, they probably won’t be in the future, which is why I separated the branches in this PR. Now, given that they’re already separate, I don’t think it makes much sense to collapse them and then separate them again when we need to.