validate package transactions with their in-package ancestor sets #26711

cb065422e58be7558e6c2ad9eadfa885bb48c708 Let's add an Assume checking tx == package.end()?

cb065422e58be7558e6c2ad9eadfa885bb48c708

After this change it's impossible to hit quit_early=false and txns_package_eval.empty(), which was possible before. It means that even if everything was either (valid, already in the mempool, or invalid due to a subset of failures) — you will force the checks again? (before this change, it could have terminated early)

Maybe related to the other comment i left.

After this change it's impossible to hit quit_early=false and txns_package_eval.empty(), which was possible before.

Right, quit_early=false && txns_package_eval.empty() means everything so far has been valid or txid already in mempool.

It means that even if everything was either (valid, already in the mempool, or invalid due to a subset of failures) — you will force the checks again? (before this change, it could have terminated early)

Ok I will try to break down all the possibilities in this scenario. If we get to the child and quit_early=false && txns_package_eval.empty(), everything else has been valid, and we should validate the child. It doesn't matter if we do it inside or outside this loop, but it's better to only do it 1 time. The child's result can be: valid, policy/missing inputs, or other failure. If txid already in mempool we would have already exited the loop.

Before the commit:

• If valid: the tx was validated 1 time, inside the loop.
• If policy/missing inputs: the tx was validated 2 times, both inside the loop and with AcceptMultipleTransactions(txns_package_eval) outside the loop.
• If other failure: the tx was validated 1 time, inside the loop.

Right after the loop, quit_early || txns_package_eval.empty() hits, but none of them terminated without validating the child.

After the commit:

• If valid: the tx is validated 1 time, outside the loop.
• If policy/missing inputs: the tx is validated 1 time, outside the loop.
• If other failure: the tx is validated 1 time, outside the loop.

Right after the loop, quit_early || txns_package_eval.empty() does not hit. The tx is validated, and then the results returned. The function returns at a later line of code, but that doesn't mean more work was done.

would help for reading clarity if nothing else, agreed

Thank you for this elaborate analysis!

So in if valid and if other failure cases, it's not just inside vs outside the loop, right? Because "outside the loop" also means re-validating other transactions, not just the child (AcceptMultipleTransactions call). (Note that I'm look only at the first commit so far)

What I'm talking about seems to be fixed in the next commit 879b55fa9da3688fb6b85e7f40f9778753a9102f. If my understanding is correct, I'd suggest mentioning this in the commit message.

Added

Thank you yes, it makes more sense to switch the commits around. Switched their order and made the commit messages more descriptive

cb065422e58be7558e6c2ad9eadfa885bb48c708

What you're saying in the commit message is true only for TxValidationResult::TX_MEMPOOL_POLICY or TxValidationResult::TX_MISSING_INPUTS? In other failures, we won't validate the child for the second time (because it would trigger quit_early)

What's even worse than a mistake in the commit message, you won't be able to catch other kinds of failures here (because you don't set quit_early here), so you might do some extra work which should have been avoided otherwise, no?

Added in the commit message that, specifically if the failure is policy or missing inputs, it was validated twice. Thanks

Rest of this comment seems appropriate elsewhere now that its put in its own subroutine.

Moved back to IsPackageWellFormed()

While we're here: CheckPackageWellFormed

Going with IsPackageWellFormed to follow the Is* pattern, if that's ok

s/so/to/

Done

when calling Txns()

Only when calling GetAncestorSet()*, added comment that Txns() is unchanged.

879b55fa9da3688fb6b85e7f40f9778753a9102f

According to this change, it should now be possible to hit mempoolaccept_res.m_result_type == INVALID, but only when [...].

However, I don't see how the AcceptPackageWrappingSingle code change touches mempoolaccept_res at all. So it's unclear why the Assume is changed here.

Oops yes, I've removed this

     * Expects that:

Done

Maybe we can get rid of quit_early and have Packagifier contain a HasBan function?

(was able to get rid of quit_early without HasBan())

If we change GetAncestorSet to an optional and store banned-ness, then we can use it to check if there is nothing left to be submitted directly by calling txns_package_eval.empty().

    if (packageified.HasBan() || txns_package_eval.empty()) {

Or, if we keep GetAncestorSet as is, just do txns_package_eval.empty() only? It will be empty in both the cases where it has a banned input, or where all transactions have been excluded?

Then we can just get rid of quit_early and don't need to track if we've banned anything?

Good idea, got rid of quit_early and exists(). Using txns_package_eval std::nullopt or empty

036d2b07fb3ce404dd6787d8d0fe4e80c168fdf4 This Assume is orthogonal to the rest of the code changes (these changes don't touch mempoolaccept_res), so it's unclear why you remove it here.

Oops, removed

4c4cea3856b1fa2a50de25847b19ae90c4263916 nit: maybe it's better to use HasConflicts. Double-negation is harder to understand :)

I've changed it to IsConsistent(). I didn't want to change to HasConflicts() because that would require changing the function implementation to be opposite, and the Is* pattern seems to be good for readability

8764ecc566413b4974e030c596193e75ff39f746 Can this result in an endless recursion, if txA spends txB and txB spends txA at the same time? Or possibly through txC. (similar in IsAncestorPackage)

If we have a cycle of transactions, SHA256 is broken, no? 😅

You are right. Not sure if it's worth commenting. It's unlikely someone will violate this assumption (it would require big changes to Bitcoin), but it could save time to someone like me who forget about this property.

I've added a comment to visit()

8764ecc566413b4974e030c596193e75ff39f746 The comment is incorrect: nullopt is returned, not empty vector.

Thanks, fixed

cea7dcf4388ad65436fcb98c7025b607708e09ab nit: worth changing input_height to uint too, maybe even with const?

• It will need to be cast to int anyway when we call AddCoins(). Maybe one day if we change AddCoins() ?
• I don't think const is very meaningful since it's passed by value

db6eed0cdfb8a90b54fee4955fd9b07d5fc1817b Consider everything, including the child, to be valid. Previously, the execution would terminate at txns_package_eval.empty() gate after the loop. After this commit, this won't pass — and everything (other transactions) would be validated once again.

I think it's the opposite - we avoid re-validating things now.

Consider everything, including the child, to be valid.

Ok, we can break this down. Let's split it into 2 types of scenarios: non-CPFP case and CPFP case. In the CPFP case, the parent needs the child in order to pass.

(1) Non-CPFP case (1a) Before this commit Inside this loop, validate parent. The parent passes and is submitted to the mempool. Then, validate child. The child passes and is submitted. After the loop, we quit because txns_package_eval.empty(). How many times we validated something: 2

(1b) After this commit Inside this loop, validate parent. The parent passes and is submitted to the mempool. Inside this loop, at this condition, txns_package_eval.push_back(child) and break. After the loop, we validate using AcceptPackageWrappingSingle({child}). How many times we validated something: 2

(2) CPFP case (2a) Before this commit Inside loop, validate parent. The parent fails due to too low fee. txns_package_eval.push_back(parent). Inside this loop, validate child. The child fails due to missing inputs. txns_package_eval.push_back(child). After the loop, we validate using AcceptPackageWrappingSingle({parent, child}). How many times we validated something: 3

(2b) After this commit Inside loop, validate parent. The parent fails due to too low fee. txns_package_eval.push_back(parent). Inside this loop, at this condition, txns_package_eval.push_back(child) and break. After the loop, we validate using AcceptPackageWrappingSingle({parent, child}). How many times we validated something: 2

Thank you for helping me understand this. I understand you are right now.

https://github.com/bitcoin/bitcoin/commit/db6eed0cdfb8a90b54fee4955fd9b07d5fc1817b Should say "non-child transaction'

Added

936c7f170bc68ec583ae026ddbaf197ca6cd0792 Could you help me understand how the latter condition can be possible for this transaction?

I assume you're asking "how can txns_package_eval be empty?" If all of the transactions were already in the mempool, they have each been Exclude'd. So GetAncestorSet(child) returns a vector of all the non-excluded ancestors, which is an empty vector.

132159cc6ae36da7697fcbb90bd5914e02a41386 This limit is never checked?

meant to be a precondition, so the caller should check it beforehand

93e976d98726040edf4070175c46fdc504aabff5 why not ban here?

Here, the tx failed for missing inputs or policy (i.e. possibly too low fee), so there is a chance of it becoming valid if later validated as a package.

93e976d98726040edf4070175c46fdc504aabff5 Does "passed" mean "valid" in this context?

Yes

Can't it be empty if the child is invalid? (if (!subpackage) { etc)

subpackage has type std::optional<std::vector<CTransactionRef>>, so !subpackage means it is std::nullopt. empty vector means all transactions have been excluded.

I'm thinking about the following:

• two transactions
• Ban(parent)
• if (!subpackage) { on the child iteration
• txns_package_eval.empty() but nothing is "valid" (if that's what you mean by passed)

Maybe you meant "when quit_early is false, then [...]" but I'm not sure.

In that case, if (!subpackage) { hits on the child iteration, and its result is prefilled with the "missing inputs" failure. And then we have !txns_package_eval, not txns_package_eval.empty().

And then we have !txns_package_eval,

See the commit I'm referring to, !txns_package_eval is impossible there. I know that this inaccuracy goes away in the next commit, but w.r.t. 93e976d98726040edf4070175c46fdc504aabff5 this comment seems incorrect.

Ah I see. I can move this comment to the last commit.

Noting that we can remove the missing inputs condition from here, since we are validating subpackages now and nobody should have missing inputs unless there is a transaction missing

I don't think this is true?

We're only returning TxValidationResult::TX_MEMPOOL_POLICY for package size of 1(which becomes a Single Accept), so and subsequent subpackage relying on this prior tx will infer a result of TX_MISSING_INPUTS, invalid-tx-dependency in AcceptPackageWrappingSingle.

Wondering if it might be worth it to have a specific TxValidationResult that is precisely for the reasons that we may allow re-evaluation(early-ish abort due to low (package) feerate), and no others. Would this make reasoning about DoS easier?

https://github.com/instagibbs/bitcoin/commit/30eaa61160cf63cc79d1bc3232d33de091f94391

This is what I was thinking. As a reviewer I can pretty quickly tell what errors will be allowed retries later(and be included in following sub-packages), and which will not. e.g., bip125-replacement-disallowed will never work later, nor will txn-mempool-conflict, min relay fee not met, too-long-mempool-chain(?), too-long-mempool-chain(?) and nothing currently in ReplacementChecks.

Could also just bubble up the individual result to avoid the TX_MISSING_INPUTS case, but I really like being able to very quickly find all instances... thoughts?

This is just being moved here and I don't think this can overflow in practice, but the init value should be int64_t{0}, so maybe it would make sense to fix this somewhere within this PR. (searched the rest of the codebase after #27021 (review)).

Ah thanks!

this should assert that package is non-empty

Aha, thanks!

        for (int parent_idx{0}; parent_idx < 99; ++parent_idx) {

ordering between package[i] and packageified.Txns()[i] doesn't seem to be stable? The above shuffle is only "undone" by sorting by number of in-package ancestors when constructing the AncestorPackage, but of course all parents have the same value.

Changed to always use package[i], and added a comment about this unstable sort

under what situations does the "representative tx" not get filled, and why is TX_MISSING_INPUTS the right value to set?

Elaborated

perfect

hmm I think the fuzzer was smart enough to give a coin with the same hash as the txid of a constructed transaction below, built on a different set of ancestors...

I think hashing these bytes would defeat this since it can't generate a valid tx with that txid?

or to be lazier, just set the initial utxos' prevout.n index to some impossibly high numbers to ever be generated

I think hashing these bytes would defeat this since it can't generate a valid tx with that txid?

Did this one, thanks 👍

this comment becomes a bit disconnected from the logic itself.

Elaborated on the comment, hopefully it's better now

I know what this means, but kinda... odd that AncestorPackage can be !IsAncestorPackage

Yeah. Added some docs to the class (also mentioned BIP331) so maybe it's more clear? I used to call it Packageifier because it can potentially build a package out of any random list of transactions. But then it's weird because we packageify a Package. Open to naming improvements 😅

should just leave this example in in the positive sense of it being accepted?

Re-added

CI Failure here:

C:/Users/ContainerAdministrator/AppData/Local/Temp/cirrus-ci-build/src/test/txpackage_tests.cpp(622): error: in "txpackage_tests/package_submission_tests": check GetVirtualTransactionSize(*tx_parent2) == 191 has failed [190 != 191]

I'm going to guess that this is because real signatures are being created, which can occasionally be smaller than 71 bytes, thus resulting in a tx that is one byte smaller. If the script type doesn't matter, I would suggest using taproot keypath spends since that has fixed size signatures. Otherwise these checks will need to account for possibly varying transaction sizes.

I'm fine if the magic number checks have a bit of wiggle room tbh, just leave a comment?

Changed to use T-T-T-Taproot for fixed size (thanks @achow101)

grandparent still not fixed size :facepalm: edited the assertion to be on the multiple of 10. I think it worked this time.

taproot coinbase outputs wen

LGTM

even more sensible than my suggestion :+1: no issues running so far

ASCII :gem: needs connection between grandparent and child

Fixed

s/121/112/ ?

const CAmount child_fee{minfeerate.GetFee(5676 + 121 + 121 + 227) - grandparent_fee - parent_fee - parent_fee};

What does 5676 + 121 + 121 + 227 numbers represent, 5676 corresponds to the grandparent size does 121 121 mean the sizes of parent1 and parent2 (which should be 112) ? Also is 227 the child size? maybe use a variable with a descriptive name for the magic numbers before usage since I see they are used in multiple places.

diamond keeps going "out of sync", maybe just have variable names that people can find?

How about actually checking the result here?

Test the result and ensure that all the package transactions are accepted?

        submitpackage_result = self.nodes[0].submitpackage(chain_hex)
        assert 'tx-results' in submitpackage_result
        result = submitpackage_result['tx-results']
        assert_equal(len(result), len(chain_hex))
        assert all([(tx_from_hex(tx_hex).getwtxid() in result) for tx_hex in chain_hex])

I understand the rationale, but it seems dangerous to skip validation steps in special cases because of expected checks happening later in another scope (that's how I understand it). Such things can lead to nasty errors down the line.

Kind of confusing and grep unfriendly that this test helper has the exact same name as that Package method. Maybe call it TestAncestorPackage or so?

nit: Developer notes say "Class member variables have a m_ prefix.".

Done

Just checking the last tx doesn't guarantee this alone. Maybe mention here that this works because subpackages are being evaluated by themselves as well?

This check doesn't feel like it makes sense to me -- it should either be unnecessary (because dealing with subpackages first takes care of it), or else it seems insufficient (because a grandparent might be paying for parent and child, and child paying for parent; but grandparents alone are good enough, but after they're accepted parent and child combined aren't good enough).

IIRC circles back to the diamond problem: #27609 (comment)

Both the parents should contribute to the grandparent being included, but in ancestor packages we can't take that into account(without a common descendant). Instead the whole ancestor package is included, even though the child is likely to be immediately evicted.

I don't think this needs a diamond, even.

For example: mempool is evicting below 10sat/vb, grandparent is A at 3300sat, 100vb (33sat/vb); parent is B spending A, at 700sat, 200vb (3.5sat/vb), child is C, spending B at 2000 sat, 100vb (20sat/vb). If you accept A first, then B alone is still below the eviction threshold, but so is B+C (2700sat, 300vb 9sat/vb), even though C is doing cpfp here.

@ajtowns Right, this is the example I thought of as well. But I don't understand why you think the check is unnecessary. From my understanding, this is the point where the subpackage A+B would fail then so I think it is necessary. It just depends on the fact that the function is called with each subpackage.

I don't understand what this scenario has to do with this case. A will be accepted, then B rejected, then B+C rejected, all for having package feerates lower than 10 sat/vbyte.

I don't think it's unnecessary - the diamond package would be accepted if we remove this check, the subpackage algorithm doesn't help with this. However, I'm also not sure if it's sufficient - e.g. what if we'd add another "grandchild" to the child of the diamond package, with a feerate slightly above the previous package feerate (e.g. 5.1 sat/vB). Then this check wouldn't fail anymore, but the descendant feerate of the child (i.e. the combined fees of the child and the grandchild) could be below the minfeerate, so the child/grandchild would get evicted right after submission?

The A <- B <- C example is handled without the diamond check, yes. Accept A, reject B, reject B+C. I'm pretty sure the code on master would do this if you removed the child-with-parents requirement.

The case in the unit test isn't handled by validating subpackages, no. I agree the "no parent pay for child" rule is not enough in some cases, and is too restrictive in others.

For example, let's say the minfeerate is 2sat/vB:

A1      A2                 (both 1sat/vB)
  \    /  \
    B1     B2              (both 3.5sat/vB)
      \   /
        C                  (1sat/vB)

Ideally we take all except C. We reject A1, reject A2, reject A1+A2+B1, accept A2+B2, reject A1+B1+C. It would be nice if, while considering A1+B1+C, we are again willing to take a subset. Or if we were to try another order or operations, we could reject A1, reject A2, accept A2+B2, accept A1+B1, reject C. But the logic for these two strategies is essentially "try every possible subset." For this example, C actually can be any tx we shouldn't accept, e.g. one with an invalid signature.

Perhaps one way to generalize this is that, within an ancestor package, we should be able to accept a subset. But unless we try every subset, it seems there's always a way to rearrange a package or add a tx to make us reject part of it. V3 world has fewer possibilities but still feels a bit intractable.

Maybe we can brainstorm a way to rearrange which checks we do when. Like #26711 (review). Perhaps try everything without feerate checks, eliminate invalid ones, then do feerate analysis. But one transaction could replace transactions in the mempool, then affecting the ancestor set of another transaction in the package...

We can do our best to have the sender-side logic give a good sort. And if a sender is deliberately crafting packages that we'll reject, we address that by taking care not to cache rejections too aggressively. e.g. here we'd want to cache the A1+A2+B1 rejection, but be willing to retry A1+B1.

So my idea for that would be to have net_processing do retries:

[EDIT: fix logic for skipping B1 on first pass. Probably doesn't quite make sense for B1 to be sorted before B2 in a feerate topo sort, if B1's ancestor fee rate is lower than B2's though...]

• precheck each
• topo sort by feerate: A1, A2, B1, B2, C
• skip A1, A2 as too low fee rate
• skip [A1, A2, B1] as too low fee rate
• accept B2 with ancestor A2; recalculate fee rates
• skip C as too low fee
• return [A1,B1,C] as skipped
• on the next ProcessMessages call for that peer, retry [A1,B1,C]
• topo sort again; skip A1, accept B1, recalculate C, skip C
• return [C] as skipped
• on the next ProcessMessages call for that peer, retry [C]
• no successes, so return [C] as soft-rejected

If these retries aren't sufficient to capture CPFPs, they are also unnecessary from the standpoint of getting MVP deployed perhaps. If there is no way to avoid missing things without going full cluster mempool, I'd suggest keeping things very simple to start. I'll let Gloria whiteboard some alternatives before exploring further.

So my #26711 (review) for that would be to have net_processing do retries:

This seems like a good option to consider for computational complexity, but I think could be even slower if we are blocked by UTXO fetching - they are cached when fetched in PreChecks, uncached at the end of ProcessNewPackage, and then reloaded again when we retry. So just thinking about UTXO fetching, it seems better to do retries while we have them loaded and cached.

I feel like this also requires us making a tradeoff between "good enough result" and potentially a very long queue of things to retry. We would need to put the txns in an orphanage/memory-bounded data structure between retries. We'd want to slow down package relay requests with this peer to avoid getting backed up, be prepared to drop things, etc. In situations where somebody is specially crafting packages, imo we aren't necessarily solving the problem and we'd end up doing more work.

Should probably mark it as EXPERIMENTAL in the rpc docs -- see "sendall" eg in wallet/rpc/spend.cpp

Not really sure this should be advertised in the release notes at this stage at all, though.

I worry that this approach can cause O(n^2) validation costs in some way, if you've got n txs and end up doing work that results in multiple soft-rejects for each tx.

I wonder if it wouldn't be better to do something more like:

• topologically sort the package
• accept each tx in order into a mem-pond -- a special temporary mini pool just for this package, that enforces consensus rules, but doesn't care about minimum fees
• any txs that weren't valid obviously fail at this point and are dropped, along with any descendants
• for everything that made it into the pond, consider moving it into the mempool proper, applying the cpfp logic and possibly rbf logic that the mempond didn't enforce

Ideally, I think we'd want to:

1. lookup utxos (slow, but unavoidable)
2. drop anything that can't possibly pay sufficient fees
3. run script validation in topological order (slow, don't do it if parents failed, only do it once)
4. do final fee rate analysis (can't do this until we know what txs are actually valid)

One way to preserve the current approach while reducing the risk of O(n^2) behaviour might be to move the "retry" behaviour out of validation -- ie:

• do PreChecks on all the package txs (then you can calculate fees)
• topologically sort the txs
• for each tx:
  • high enough fee rate? try accepting it.
    • failure: immediately return.
    • success: remove that tx and its ancestors from the package, marking them as succeeded; recalculate fee rates for descendants
• release mempool.cs

Then net_processing or rpc can still retry the remaining txs if there was a failure, but can decide to only do that after processing a tx from every other peer, eg.

After whiteboarding this idea + chunking, ended up with #26711 (comment)

unused

IsChildWithParents() is unused after this outside of tests - can we remove it?

Removed

after c44df9b0bfa66c77e5cbbff6688ea73cf39ec1d3, the packages.md explanation "Packages must be child-with-unconfirmed-parents packages." is out of date. It would be good to document the definition of an "ancestor package" there instead.

Fixed

9962efedb813e02034f2d1634efe79562c5adc19

Still no definition of ancestor package it seems?

In 7e25e002100f57d69a967f475eb484a8637d9dcb "[unit test] parent pay for child is not allowed"

The magic numbers in this test were bothering me, so here's a diff that drops most of them. It refactors CreateValidMempoolTransaction again to have a function CreateValidTransaction that just creates a transaction. This can take a feerate and it will deduct the fee from one of the outputs. This test is then changed to have target feerates for each of the transactions that are all relative to minfeerate.

diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
index 1e00a75a79..1d480eedad 100644
--- a/src/test/txpackage_tests.cpp
+++ b/src/test/txpackage_tests.cpp
@@ -378,16 +378,13 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
         // Diamond shape:
         //
         //     grandparent
-        //       5679sat
-        //        5676vB
+        //       minfr/5
         //     ^    ^    ^
         //  parent1 |   parent2
-        // 12000sat |  12000sat
-        //  112vB   |   112vB
+        // minfr*25 |  minfr*25
         //       ^  |  ^
         //        child
-        //       2424sat
-        //        959vB
+        //       minfr - 0.05 s/vB
         //
         // grandparent is below minfeerate
         // {grandparent + parent1} and {grandparent + parent2} are both below minfeerate
@@ -395,71 +392,72 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
         // child has a feerate just below minfeerate
         // {grandparent + parent1 + parent2 + child} is above minfeerate
         // All transactions should be rejected.
-        const CAmount grandparent_fee{5679};
+        const CFeeRate grandparent_feerate{minfeerate.GetFeePerK() / 5};
         std::vector<CTransactionRef> grandparent_input_txns;
         std::vector<COutPoint> grandparent_inputs;
         for (auto i{1}; i < 50; ++i) {
             grandparent_input_txns.push_back(m_coinbase_txns[i]);
             grandparent_inputs.push_back(COutPoint{m_coinbase_txns[i]->GetHash(), 0});
         }
-        CAmount last_value = grandparent_inputs.size()*50*COIN - 10*COIN - 10*COIN - grandparent_fee;
-        auto mtx_grandparent{CreateValidMempoolTransaction(/*input_transactions=*/grandparent_input_txns,
-                                                           /*inputs=*/grandparent_inputs,
-                                                           /*input_height=*/102,
-                                                           /*input_signing_keys=*/{coinbaseKey},
-                                                           /*outputs=*/{CTxOut{10*COIN, parent_locking_script}, CTxOut{10*COIN, parent_locking_script},
-                                                                        CTxOut{last_value, parent_locking_script}},
-                                                           /*submit=*/false)};
+        CAmount last_value = grandparent_inputs.size()*50*COIN - 10*COIN - 10*COIN;
+        auto [mtx_grandparent, grandparent_fee] = CreateValidTransaction(/*input_transactions=*/grandparent_input_txns,
+                                                    /*inputs=*/grandparent_inputs,
+                                                    /*input_height=*/102,
+                                                    /*input_signing_keys=*/{coinbaseKey},
+                                                    /*outputs=*/{CTxOut{10*COIN, parent_locking_script}, CTxOut{10*COIN, parent_locking_script},
+                                                                 CTxOut{last_value, parent_locking_script}},
+                                                    /*feerate=*/grandparent_feerate,
+                                                    /*fee_output=*/2);
         CTransactionRef tx_grandparent = MakeTransactionRef(mtx_grandparent);
         package_ppfc.push_back(tx_grandparent);
 
-        const CAmount parent_fee{12000};
-        const CAmount parent_value{10*COIN - parent_fee};
-        auto mtx_parent1{CreateValidMempoolTransaction(/*input_transaction=*/tx_grandparent, /*input_vout=*/0,
-                                                       /*input_height=*/102,
-                                                       /*input_signing_key=*/parent_key,
-                                                       /*output_destination=*/child_locking_script,
-                                                       /*output_amount=*/parent_value, /*submit=*/false)};
+        const CFeeRate parent_feerate{minfeerate.GetFeePerK() * 25};
+        const CAmount parent_value{10*COIN};
+        auto [mtx_parent1, parent_fee1] = CreateValidTransaction(/*input_transactions=*/{tx_grandparent},
+                                                /*inputs=*/{COutPoint{tx_grandparent->GetHash(), 0}},
+                                                /*input_height=*/102,
+                                                /*input_signing_keys=*/{parent_key},
+                                                /*outputs=*/{CTxOut{parent_value, child_locking_script}},
+                                                /*feerate=*/parent_feerate,
+                                                /*fee_output=*/0);
         CTransactionRef tx_parent1 = MakeTransactionRef(mtx_parent1);
         package_ppfc.push_back(tx_parent1);
-        auto mtx_parent2{CreateValidMempoolTransaction(/*input_transaction=*/tx_grandparent, /*input_vout=*/1,
-                                                       /*input_height=*/102,
-                                                       /*input_signing_key=*/parent_key,
-                                                       /*output_destination=*/child_locking_script,
-                                                       /*output_amount=*/parent_value, /*submit=*/false)};
+        auto [mtx_parent2, parent_fee2] = CreateValidTransaction(/*input_transactions=*/{tx_grandparent},
+                                                /*inputs=*/{COutPoint{tx_grandparent->GetHash(), 1}},
+                                                /*input_height=*/102,
+                                                /*input_signing_keys=*/{parent_key},
+                                                /*outputs=*/{CTxOut{parent_value, child_locking_script}},
+                                                /*feerate=*/parent_feerate,
+                                                /*fee_output=*/0);
         CTransactionRef tx_parent2 = MakeTransactionRef(mtx_parent2);
         package_ppfc.push_back(tx_parent2);
 
 
-        const CAmount child_fee{minfeerate.GetFee(5676 + 121 + 121 + 227) - grandparent_fee - parent_fee - parent_fee};
-        const CAmount child_value{last_value + 2*parent_value - child_fee};
-        auto mtx_child{CreateValidMempoolTransaction(/*input_transactions=*/{tx_grandparent, tx_parent1, tx_parent2},
-                                                     /*inputs=*/{COutPoint{tx_grandparent->GetHash(), 2}, COutPoint{tx_parent1->GetHash(), 0}, COutPoint{tx_parent2->GetHash(), 0}},
-                                                     /*input_height=*/102,
-                                                     /*input_signing_keys=*/{parent_key, child_key, grandchild_key},
-                                                     /*outputs=*/{CTxOut{child_value, grandchild_locking_script}},
-                                                     /*submit=*/false)};
+        const CFeeRate child_feerate{minfeerate.GetFeePerK() - 50};
+        const CAmount child_value{last_value + 2*parent_value};
+        auto [mtx_child, child_fee] = CreateValidTransaction(/*input_transactions=*/{tx_grandparent, tx_parent1, tx_parent2},
+                                              /*inputs=*/{COutPoint{tx_grandparent->GetHash(), 2}, COutPoint{tx_parent1->GetHash(), 0}, COutPoint{tx_parent2->GetHash(), 0}},
+                                              /*input_height=*/102,
+                                              /*input_signing_keys=*/{parent_key, child_key, grandchild_key},
+                                              /*outputs=*/{CTxOut{child_value, grandchild_locking_script}},
+                                              /*feerate=*/child_feerate,
+                                              /*fee_output=*/0);
         CTransactionRef tx_child = MakeTransactionRef(mtx_child);
         package_ppfc.push_back(tx_child);
 
-        // Magic Number Sanity Checks
-        // A little bit of wiggle room because the signature spending the coinbase is not fixed size.
-        BOOST_CHECK_EQUAL(GetVirtualTransactionSize(*tx_grandparent) / 10, 567);
-        BOOST_CHECK_EQUAL(GetVirtualTransactionSize(*tx_parent1), 112);
-        BOOST_CHECK_EQUAL(GetVirtualTransactionSize(*tx_parent2), 112);
-        BOOST_CHECK_EQUAL(GetVirtualTransactionSize(*tx_child), 227);
         // Neither parent can pay for the grandparent by itself
-        BOOST_CHECK(minfeerate.GetFee(GetVirtualTransactionSize(*tx_grandparent) + GetVirtualTransactionSize(*tx_parent1)) > grandparent_fee + parent_fee);
-        BOOST_CHECK(minfeerate.GetFee(GetVirtualTransactionSize(*tx_grandparent) + GetVirtualTransactionSize(*tx_parent2)) > grandparent_fee + parent_fee);
+        BOOST_CHECK_EQUAL(parent_fee1, parent_fee2);
+        BOOST_CHECK(minfeerate.GetFee(GetVirtualTransactionSize(*tx_grandparent) + GetVirtualTransactionSize(*tx_parent1)) > grandparent_fee + parent_fee1);
+        BOOST_CHECK(minfeerate.GetFee(GetVirtualTransactionSize(*tx_grandparent) + GetVirtualTransactionSize(*tx_parent2)) > grandparent_fee + parent_fee1);
         const auto parents_vsize = GetVirtualTransactionSize(*tx_grandparent) + GetVirtualTransactionSize(*tx_parent1) + GetVirtualTransactionSize(*tx_parent2);
         // Combined, they can pay for the grandparent
-        BOOST_CHECK(minfeerate.GetFee(parents_vsize) <= grandparent_fee + 2 * parent_fee);
+        BOOST_CHECK(minfeerate.GetFee(parents_vsize) <= grandparent_fee + 2 * parent_fee1);
         const auto total_vsize = parents_vsize + GetVirtualTransactionSize(*tx_child);
         BOOST_CHECK(minfeerate.GetFee(GetVirtualTransactionSize(*tx_child)) > child_fee);
         // The total package is above feerate, but mostly because of the 2 parents
-        BOOST_CHECK(minfeerate.GetFee(total_vsize) <= grandparent_fee + 2 * parent_fee + child_fee);
+        BOOST_CHECK(minfeerate.GetFee(total_vsize) <= grandparent_fee + 2 * parent_fee1 + child_fee);
         // Child feerate is less than the package feerate
-        BOOST_CHECK(CFeeRate(child_fee, GetVirtualTransactionSize(*tx_child)) < CFeeRate(grandparent_fee + 2 * parent_fee + child_fee, total_vsize));
+        BOOST_CHECK(CFeeRate(child_fee, GetVirtualTransactionSize(*tx_child)) < CFeeRate(grandparent_fee + 2 * parent_fee1 + child_fee, total_vsize));
 
         const auto result_ppfc = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool, package_ppfc, /*test_accept=*/false);
         BOOST_CHECK(result_ppfc.m_state.IsInvalid());
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 1ca69371b3..11b98cda49 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -343,12 +343,13 @@ CBlock TestChain100Setup::CreateAndProcessBlock(
 }
 
 
-CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(const std::vector<CTransactionRef>& input_transactions,
-                                                                     const std::vector<COutPoint>& inputs,
-                                                                     int input_height,
-                                                                     const std::vector<CKey>& input_signing_keys,
-                                                                     const std::vector<CTxOut>& outputs,
-                                                                     bool submit)
+std::pair<CMutableTransaction, CAmount> TestChain100Setup::CreateValidTransaction(const std::vector<CTransactionRef>& input_transactions,
+                                                                                  const std::vector<COutPoint>& inputs,
+                                                                                  int input_height,
+                                                                                  const std::vector<CKey>& input_signing_keys,
+                                                                                  const std::vector<CTxOut>& outputs,
+                                                                                  const std::optional<CFeeRate>& feerate,
+                                                                                  const std::optional<uint32_t>& fee_output)
 {
     CMutableTransaction mempool_txn;
     mempool_txn.vin.reserve(inputs.size());
@@ -372,17 +373,57 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(const std::
     }
     // Build Outpoint to Coin map for SignTransaction
     std::map<COutPoint, Coin> input_coins;
+    CAmount inputs_amount{0};
     for (const auto& outpoint_to_spend : inputs) {
         // - Use GetCoin to properly populate utxo_to_spend,
         Coin utxo_to_spend;
         assert(coins_cache.GetCoin(outpoint_to_spend, utxo_to_spend));
         input_coins.insert({outpoint_to_spend, utxo_to_spend});
+        inputs_amount += utxo_to_spend.out.nValue;
     }
     // - Default signature hashing type
     int nHashType = SIGHASH_ALL;
     std::map<int, bilingual_str> input_errors;
     assert(SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors));
 
+    // Calculate fees paid
+    CAmount current_fee = inputs_amount - std::accumulate(outputs.begin(), outputs.end(), CAmount(0),
+        [](const CAmount& acc, const CTxOut& out) {
+            return acc + out.nValue;
+        });
+
+    // Deduct fees from fee_output to meet feerate if set
+    if (feerate.has_value()) {
+        assert(fee_output.has_value());
+        assert(fee_output.value() < mempool_txn.vout.size());
+
+        CAmount target_fee = feerate.value().GetFee(GetVirtualTransactionSize(CTransaction{mempool_txn}));
+        CAmount deduction = target_fee - current_fee;
+
+        if (deduction > 0) {
+            // Only deduct fee if there's anything to deduct.
+            // If the caller has put more fees than the target feerate, don't change the fee.
+            mempool_txn.vout[fee_output.value()].nValue -= deduction;
+
+            // Re-sign since an output has changed
+            input_errors.clear();
+            assert(SignTransaction(mempool_txn, &keystore, input_coins, nHashType, input_errors));
+            current_fee = target_fee;
+        }
+    }
+
+    return {mempool_txn, current_fee};
+}
+
+CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(const std::vector<CTransactionRef>& input_transactions,
+                                                                     const std::vector<COutPoint>& inputs,
+                                                                     int input_height,
+                                                                     const std::vector<CKey>& input_signing_keys,
+                                                                     const std::vector<CTxOut>& outputs,
+                                                                     bool submit)
+{
+    CMutableTransaction mempool_txn = CreateValidTransaction(input_transactions, inputs, input_height, input_signing_keys, outputs).first;
+
     // If submit=true, add transaction to the mempool.
     if (submit) {
         LOCK(cs_main);
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 106bee6b4b..6c15140ffa 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -9,6 +9,7 @@
 #include <key.h>
 #include <node/caches.h>
 #include <node/context.h>
+#include <policy/feerate.h>
 #include <primitives/transaction.h>
 #include <pubkey.h>
 #include <random.h>
@@ -20,10 +21,10 @@
 #include <util/vector.h>
 
 #include <functional>
+#include <optional>
 #include <type_traits>
 #include <vector>
 
-class CFeeRate;
 class Chainstate;
 
 /** This is connected to the logger. Can be used to redirect logs to any other log */
@@ -154,6 +155,27 @@ struct TestChain100Setup : public TestingSetup {
     //! Mine a series of new blocks on the active chain.
     void mineBlocks(int num_blocks);
 
+    /**
+     * Create a transaction, optionally setting the fee based on the feerate.
+     * Note: The feerate may not be met exactly depending on whether the signatures can have different sizes.
+     *
+     * [@param](/bitcoin-bitcoin/contributor/param/) input_transactions   The transactions to spend
+     * [@param](/bitcoin-bitcoin/contributor/param/) input_height         The height of the block that included the input transactions.
+     * [@param](/bitcoin-bitcoin/contributor/param/) inputs               Outpoints with which to construct transaction vin.
+     * [@param](/bitcoin-bitcoin/contributor/param/) input_signing_keys   The keys to spend the input transactions.
+     * [@param](/bitcoin-bitcoin/contributor/param/) outputs              Transaction vout.
+     * [@param](/bitcoin-bitcoin/contributor/param/) feerate              The feerate the transaction should pay.
+     * [@param](/bitcoin-bitcoin/contributor/param/) fee_output           The index of the output to take the fee from.
+     * [@return](/bitcoin-bitcoin/contributor/return/) The transaction and the fee it pays
+     */
+    std::pair<CMutableTransaction, CAmount> CreateValidTransaction(const std::vector<CTransactionRef>& input_transactions,
+                                                                   const std::vector<COutPoint>& inputs,
+                                                                   int input_height,
+                                                                   const std::vector<CKey>& input_signing_keys,
+                                                                   const std::vector<CTxOut>& outputs,
+                                                                   const std::optional<CFeeRate>& feerate = std::nullopt,
+                                                                   const std::optional<uint32_t>& fee_output = std::nullopt);
+
     /**
      * Create a transaction and submit to the mempool.
      *

Taken, thanks

In 005581b5ea61edd444439900109c527c7112a650 "[packages] AncestorPackage sorts and builds ancestor subsets"

nit: These two loops could be combined

In 005581b5ea61edd444439900109c527c7112a650 "[packages] AncestorPackage sorts and builds ancestor subsets"

nit: The inputs add up to 4851, not 4900.

what is cto please Saw it in policy/packages.cpp AncestorPackage::visit description comment?

ctor is short for constructor.

yay! much easier to think about now

needs a reasonable comment to distinguish from TX_MEMPOOL_POLICY

my suggestion was adding something like "but result may differ when in different package"

Added that to the comment :+1:

very glad to see the special casing of this to MISSING_INPUTS gone

"[refactor] fill in results for every tx in AcceptPackage"

It could have just been submitted in the above loop, maybe if this goes away later it won't be worth being more verbose...

comment has been removed

We'll need some miniminer Experts(TM) to review this, but I'm just going to assume for now that this will be used as a topological "tie breaker" for linearization that is currently in the branch to this point

so if there's a bug in it, it's no worse than topo-sort, and can be improved later if dependencies need to be cut for whatever reason

Yeah agreed, makes it much more important that there isn't e.g. a crash bug in there somewhere, and if anything goes wrong we should quit gracefully and default to topo sort.

cc @murchandamus I think this is something you could review without too much additional background

(a.fee * a.vsize > b.fee * a.vsize)

fix comment

In e82f4e7d7b41592b8891a2c6f9b961b2fc2ec51e "[txpackages] add AncestorPackage for linearizing packages"

typo

                        // (a.fee * b.vsize > b.fee * a.vsize) is a shortcut for (a.fee / a.vsize > b.fee / b.vsize)

Fixed

would be nice to put "Filtered" in the function names of whatever is doing filtering to reduce mental load

Renamed to FilteredAncestorSet and FilteredAncestorFeeAndVsize

if this somehow happens in prod, do we want to keep stumbling along?

Imo it's fine, since we don't really do anything with it.

Seems like this will continue evaluating individual txns that have no in-package ancestors(that aren't accepted individually before!). Seems safe, but wasn't immediately obvious to me. It will abort any package evaluation later.

Refined the comment

I think we decided pre-cluster mempool we can't really nail all these cases due to inherent symmetry.

With this rule we can see cases where the penultimate child is paid for by parents, the penultimate child and final child being immediately trimmed next at below-minfee rates. It's not very pathological since the evicted package is below minfee and if the mempool isn't too large it is worthwhile to mine. I think we agreed these can still exist, but just noting.

might want to note that we do this final check after filling out results_final to give more meaningful tx results

Added comment

just noting even though I think it's reasonable: we may bail on other subpackages(that would fit in mempool) if we hit chain limits on any particular subpackage?

Note that UTXOs are cached in m_view so there is not repeated loading the second time we call PreChecks

im a utxo cache newb but it looks like the the utxo caches are being wiped before the actual submission which has its own PreChecks?

These are just uncaching the temporary coins (i.e. outputs of the package transactions), so they are not part of the UTXO set and definitely weren't pulled from disk.

Ah! Might be worth being overly explanatory here.

Added to the comment

So if my understanding of the API is correct, you receive a package from AcceptPackage(), and there is a call to the constructor AncestorPackage(), then we verify each transaction being component of the package is in the mempool by wtxid or txid or PreChecks() them.

If the transaction is PreChecks() valid, then we call AddFeeAndVsize(). If we have a TX_SINGLE_FAILURE, we add AddFeeAndVsize() or if we have a TX_MISSING_INPUTS, we SkipWithDescendants() the transaction. All other cases are treaded as impossible or failure.

So when we call LinearizeWithFees() in AcceptPackage and therefore start to rely on MiniMiner calculations. we have asserted than all packages component linearize are either spending a UTXO in the chainstate set, or spending an in-package component, so the worst-case scenario from a DoS viewpoint will be the limits as set in BIP331 / nversion=3 ?

So if my understanding of the API is correct, you receive a package from AcceptPackage() ...

Correct

we have asserted than all packages component linearize are either spending a UTXO in the chainstate set, or spending an in-package component, so the worst-case scenario from a DoS viewpoint will be the limits as set in BIP331 / nversion=3 ?

The first check in AcceptPackage is IsPackageWellFormed which requires the package to be 25 transactions at most. The linearization with MiniMiner also does not fetch in-mempool transactions. So you can assume we never construct the miniminer with more than 25 transactions.

This is independent of v3 so those limits aren't enforced.

BIP331 doesn't impose a maximum size other than 100 for pkgtxns, but I expect the p2p logic to observe that an ancpkginfo with more than 25 transactions wouldn't be submitted and just exit there without downloading the transactions. So we wouldn't send a getpkgtxns request for more than 25, and thus would immediately reject a (unsolicited) pkgtxns with more than 25. See logic for handling ancpkginfo, we will give up on downloading an ancestor package from a peer if it has too many transactions: https://github.com/bitcoin/bitcoin/pull/27742/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R4398.

So you can assume we never construct the miniminer with more than 25 transactions.

Thanks, yes I think MAX_PACKAGE_COUNT as enforced by IsPackageWellFormed is the upper bound that one can assume in the calculation of MiniMiner. This can ease the DoS reasoning to link the max ancpkginfo issued at the p2p level with MAX_PACKAGE_COUNT as requested by the mempool logic to save on bandwidth both by the local node (the list of wtxids announced) and its peer (pkgtnxs’s txn), I think.

In e82f4e7d7b41592b8891a2c6f9b961b2fc2ec51e "[txpackages] add AncestorPackage for linearizing packages"

nit: avoid looking up twice

    const auto& entry_it = m_txid_to_entry.find(tx->GetHash());
    if (entry_it == m_txid_to_entry.end()) return std::nullopt;
    const auto& entry = entry_it->second;

Done

In e82f4e7d7b41592b8891a2c6f9b961b2fc2ec51e "[txpackages] add AncestorPackage for linearizing packages"

Since all of the transactions in the package are already stored in PackageEntrys in m_txid_to_entry, could m_txns just be a vector of PackageEntry&? This would reduce the number of lookups to m_txid_entry that are needed just to sort the transactions when we are returning them.

Additionally, if functions like GetAncestorSet used a std::set<PackageEntry&>, then std::sort at the end would not be necessary.

std::vector<PackageEntry&> isn't possible, but will do reference wrappers

From my understanding we have submitpackage which is the current interface under which child-with-unconfirmed-parents can be submitted. The definition is “a topologically sorted package that consists of exactly one child and all of its unconfirmed parents (no other transactions may be present)”.

The definition given by AncestorPackage class is “A potential BIP331 Ancestor Package, i.e one transaction with its set of ancestors”, which sounds similar.

If child-with-unconfirmed-parents and AncestorPackage definitions are identical, I think the child-with-unconfirmed-parents definition in packages.md can be updated to fusion them, and maybe ProcessNewPackage can be updated.

Oh true, removed the 2+ thing

I think you thought about it in term of AncestorPackage API design, though what is the purpose of SkipWithDescendants as the “dangle” state of a transaction could be determined as soon as all the transactions are received and IsPackageWellFormed ?

Therefore checking than we have available UTXOs in mempool (or in-package) to spend could be done before to call to AncestorPackage ctor and visit to obtain the ancestor_subset, sounds it could be a small perf again assuming AcceptPackage is re-worked a bit.

Therefore checking than we have available UTXOs in mempool (or in-package) to spend could be done before to call to AncestorPackage ctor

I don't think this is true. We should sort the transactions before we look up UTXOs.

Fair, I still think it’s slightly less DoSy though depends benchmark and UTXO layout on disk of course.

The name could be more verbose to reflect the notion of “connection” it lays on, e.g m_is_ancestor_package.

Unclear if “(Modified) fees” means GetModifiedFee()

Removed mention of "modified," as it's not really relevant whether it's real fees or not

if an entry is skip'ed, but also included in another entry's descendant_subset, this can cause an Assume to fail here https://github.com/bitcoin/bitcoin/pull/26711/files#diff-a8d952408e5a47db5e09d0b13117088fef66805cf706f6df83dfa0dc5814874eR142

I think this is as simple as a descendant missing inputs, right? I'm uncertain to the implications of this.

I hit this in fuzz testing, and commented out the assertion to continue fuzzing

~Hm, it doesn't seem possible to skip a tx that is a descendant of a non-skipped tx (that means we submitted a tx to mempool without submitting its ancestor). I haven't seen the fuzzer but perhaps the issue is that the fuzzer is selecting transactions to skip a little too randomly?~ Edit: Ignore this, was only thinking about Skip() and not SkipWithDescendants

Jk I understand what you mean now by descendant missing inputs. I think the solution is to delete dangling transactions from descendant_subsets

I've fixed this by filtering the descendant_caches before passing it to MiniMiner (seemed cleaner than deleting from descendant_subset).

looks fixed to me :+1:

    const auto txns_copy = m_txns;
    std::sort(m_txns.begin(), m_txns.end(), CompareEntry());
        if (!IsSorted(Txns())) {
        m_txns = txns_copy;
    }  

Taken

    }));
    // If we somehow break topological ordering, revert to what we already had without
    // ancestor set scoring
    if (!IsSorted(Txns())) {
        m_txns = txns_copy;
        Assume(IsSorted(Txns())); // Not a lot we can do here
    }

Taken

    const auto txns_copy = m_txns;
    std::sort(m_txns.begin(), m_txns.end(), CompareEntry());

Taken

This should be noted in the declaration. I'm also not sure how this is a "more severe" case. Is it really worth it to punt this until later when we have mempool lock?

Also should have a unit/functional test to capture this allowed behavior.

Added note and a test

nit

        expected_pool_size += package_with_rbf.size() - 1;

taken

We're not trimming anymore during package evaluation; does this make sense to change?

Yes, as it's a type of fee failure that can be "overcome" by package submission. If there's a CPFP package and we received the parent first, and rejected it for this reason, we should put it in the m_recent_rejects_reconsiderable cache instead of the m_recent_rejects cache so we can still cpfp it in a package.

Ok so to be clear this is a future-facing p2p issue. Makes sense.

annotate args; maybe clang tidy would catch the wrong values in wrong slots?

Added

gripe: these !Assume() things are throwing me for a loop, especially !!empty

(that's why they have comments above them) if (!Assume(assumption)) exit_early_because_assumption_doesnt_hold();

I think this conditional is already handled by the previous conditional? Could make it an else block to make it clearer

removed conditional, made else block

anything we can Assume on success?

added a if (Assume(success)) if that's what you mean

this value can also be set by SkipWithDescendants() if its the descendant, so not just because it's already in mempool

Yes, clarified docs.

sigops-adjusted I presume, better to be explicit

added "depends on policy, calculated externally" which hopefully gets the point across.

in AncestorPackage ctor?

yes, added

isn't this a duplicate of the condition just above?

Oof yes removed

is this just an arbitrary tie-breaker?

Yeah. I've deleted it.

the latter condition is also IsSorted(), might want to make that extra clear

Clarified

    // Sort again, this time including mining scores and individual feerates.

Taken

Commit message for "[validation] fully abort package validation for non-TX_SINGLE_FAILURE"

"for all other failures, abort the whole thing." this should say something like "for all other failures, continue with single evaluation, but do not do package evaluation of remaining transactions in package."?

I know this commit is relatively temporary, but I'm wondering if we can have something like linearized_package.SkipPackage() to just allow linearized_package.FilteredTxns() to return empty, rather than quit_early and checking two ways later on if to bail.

Took the suggested changes to the commit message

Unsure if SkipPackage is cleaner than having an external bool tbh