refactors for subpackage evaluation

glozow commented at 9:14 am on October 31, 2023: member

This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in #26711 (comment).

Split package sanitization in policy/packages.h into helper functions
- Add some tests for its quirks (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1340521597)
Rename CheckPackage to IsPackageWellFormed
Improve the CreateValidTransaction unit test utility to:
- Configure the target feerate and return the fee paid
- Signal BIP125 on transactions to enable RBF tests
- Allow the specification of multiple inputs and outputs
Move CleanupTemporaryCoins into its own function to be reused later without duplication

DrahtBot commented at 9:14 am on October 31, 2023: contributor

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

Code Coverage

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, instagibbs

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26711 (validate package transactions with their in-package ancestor sets by glozow)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake requested review from instagibbs on Oct 31, 2023

in src/policy/packages.h:60 in f60560bf39 outdated

55+ * parent), checks that all parents appear somewhere in the list before their respective children.
56+ * No other ordering is enforced. This function cannot detect indirect dependencies (e.g. a
57+ * transaction's grandparent if its parent is not present).
58+ * @returns true if sorted. False if any tx spends the output of a tx that appears later in txns.
59+ */
60+bool IsSorted(const Package& txns);

dergoegge commented at 9:54 am on October 31, 2023:

nit: IsSorted does not convey what is being checked for sortedness.

0bool IsPackageSorted(const Package& txns);

glozow commented at 12:14 pm on October 31, 2023:

Ok, renamed this to IsTopoSortedPackage, i.e. “is a topologically sorted package” to be more descriptive.

dergoegge commented at 12:18 pm on October 31, 2023:

can you do the same for IsConsistent? e.g. IsPackageConsistent

glozow commented at 2:38 pm on October 31, 2023:

Renamed to IsConsistentPackage

ajtowns commented at 1:29 am on November 1, 2023:

Could alternatively make these member functions of Package ?

glozow force-pushed on Oct 31, 2023

laanwj added the label Mempool on Oct 31, 2023

glozow added the label Refactoring on Oct 31, 2023

glozow force-pushed on Oct 31, 2023

in src/policy/packages.h:64 in 80cd897945 outdated

59+ */
60+bool IsTopoSortedPackage(const Package& txns);
61+
62+/** IsTopoSortedPackage where a set of txids has been pre-populated. The set is assumed to be correct and
63+ * is mutated within this function (even if return value is false). */
64+bool IsTopoSortedPackage(const Package& txns, std::unordered_set<uint256, SaltedTxidHasher>& all_txids);

dergoegge commented at 10:35 am on November 1, 2023:

This can be dropped from the header, nothing is using it externally.

glozow commented at 5:23 pm on November 1, 2023:

ok

in src/policy/packages.h:83 in 80cd897945 outdated

79  * 2. The total weight cannot exceed MAX_PACKAGE_WEIGHT.
80  * 3. If any dependencies exist between transactions, parents must appear before children.
81  * 4. Transactions cannot conflict, i.e., spend the same inputs.
82  */
83-bool CheckPackage(const Package& txns, PackageValidationState& state);
84+bool IsPackageWellFormed(const Package& txns, PackageValidationState& state);

dergoegge commented at 10:38 am on November 1, 2023:

nit: the naming is slightly inconsistent now, it would be nicer if all the functions are either IsPackage* or Is*Package instead of a mix. Would also not be opposed to turning Package into a class and have these be member functions.

glozow commented at 5:23 pm on November 1, 2023:

I made it iswellformedpackage

in src/test/txpackage_tests.cpp:109 in 80cd897945 outdated

104+    dup_tx.vin.emplace_back(InsecureRand256(), 0);
105+    dup_tx.vin.emplace_back(InsecureRand256(), 0);
106+    Package package_with_dup_tx;
107+    BOOST_CHECK(IsConsistentPackage(package_with_dup_tx));
108+    package_with_dup_tx.emplace_back(create_placeholder_tx(1, 1));
109+    BOOST_CHECK(IsConsistentPackage(package_with_dup_tx));

dergoegge commented at 10:49 am on November 1, 2023:

0    CMutableTransaction dup_tx;
1    dup_tx.vin.emplace_back(uint256{}, 0);
2    dup_tx.vin.emplace_back(uint256{}, 0);
3    Package package_with_dup_tx{MakeTransactionRef(dup_tx)};
4    BOOST_CHECK(IsConsistentPackage(package_with_dup_tx));
5    package_with_dup_tx.emplace_back(create_placeholder_tx(1, 1));
6    BOOST_CHECK(IsConsistentPackage(package_with_dup_tx));

dup_tx is otherwise unused and does not conflict with itself.

glozow commented at 5:23 pm on November 1, 2023:

fixed

dergoegge changes_requested

[refactor] move package checks into helper functions

This allows IsSorted() and IsConsistent() to be used by themselves.
IsSorted() with a precomputed set is used so that we don't create this
set multiple times.

da9aceba21

scripted-diff: rename CheckPackage to IsWellFormedPackage

-BEGIN VERIFY SCRIPT-
sed -i 's/CheckPackage(/IsWellFormedPackage(/g' $(git grep -l CheckPackage)
-END VERIFY SCRIPT-

6ff647a7e0

[test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125

Support the creation of a transaction with multiple specified inputs or
outputs. Also accept a target feerate and return the fee paid.

Also, signal BIP125 by default - a subsequent commit needs to RBF
something.

Co-authored-by: Andrew Chow <achow101@gmail.com>

10c0a8678c

MOVEONLY: CleanupTemporaryCoins into its own function

Avoid duplicate code. This will be used at the end of every
AcceptSubPackage and after PreChecks loop in AcceptPackage.

b5a60abe87

glozow force-pushed on Nov 1, 2023

glozow commented at 5:26 pm on November 1, 2023: member

I started going down the path of making Package a class and these member functions, but it touched hundreds of loc and made it really awkward to try to use it within AncestorPackage when we’re checking if subsets are sorted

in src/policy/packages.cpp:61 in da9aceba21 outdated

59+    for (const auto& tx : txns) {
60+        if (tx->vin.empty()) {
61+            // This function checks consistency based on inputs, and we can't do that if there are
62+            // no inputs. Duplicate empty transactions are also not consistent with one another.
63+            // This doesn't create false negatives, as unconfirmed transactions are not allowed to
64+            // have no inputs.

ariard commented at 11:09 pm on November 2, 2023:

“as unconfirmed transactions are not allowed to have no inputs at consensus validation (see CheckTransaction)"

in src/policy/packages.h:66 in da9aceba21 outdated

61+
62+/** Checks that these transactions don't conflict, i.e., spend the same prevout. This includes
63+ * checking that there are no duplicate transactions. Since these checks require looking at the inputs
64+ * of a transaction, returns false immediately if any transactions have empty vin.
65+ *
66+ * Does not check consistency of a transaction with oneself; does not check if a transaction spends

ariard commented at 11:12 pm on November 2, 2023:

I don’t know what is mean by consistency of a transaction with oneself (e.g if txouts are under 0 or over MAX_MONEY) or you’re only implying “does not spend same prevout multiple times”.

glozow commented at 8:59 am on November 3, 2023:

I don’t think it’s that helpful to list more things this function doesn’t check.

dergoegge approved

dergoegge commented at 10:45 am on November 3, 2023: member

Code review ACK b5a60abe8783852f5b31bc1e63b5836530410e65

nit: the commit message in da9aceba217bbded6909f06144eaa1e1a4ebcb69 still has the old names

instagibbs approved

instagibbs commented at 1:52 pm on November 3, 2023: member

ACK b5a60abe8783852f5b31bc1e63b5836530410e65

fanquake merged this on Nov 3, 2023

fanquake closed this on Nov 3, 2023

glozow deleted the branch on Nov 3, 2023

refactors for subpackage evaluation #28758

Code Coverage

Reviews

Conflicts