package testmempoolaccept followups

glozow commented at 8:40 am on May 27, 2021: member

various followups from #20833

DrahtBot added the label Build system on May 27, 2021

DrahtBot added the label Mempool on May 27, 2021

DrahtBot added the label RPC/REST/ZMQ on May 27, 2021

DrahtBot added the label TX fees and policy on May 27, 2021

DrahtBot added the label Validation on May 27, 2021

DrahtBot commented at 3:11 pm on May 27, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22097 (validation: Move package acceptance size limit from KvB to WU by ariard)
#21866 ([Bundle 7/7] validation: Farewell, global Chainstate! by dongcarl)
#21800 (mempool/validation: mempool ancestor/descendant limits for packages by glozow)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

ariard commented at 5:20 am on May 28, 2021: member

I think this is ready for rebase and review now :)

glozow force-pushed on May 28, 2021

glozow renamed this:
~~[WIP] package testmempoolaccept followups~~
package testmempoolaccept followups
on May 28, 2021

glozow marked this as ready for review on May 28, 2021

glozow commented at 8:51 am on May 28, 2021: member

yes! CC @ariard and @jnewbery :)

fanquake removed the label Build system on Jun 1, 2021

fanquake removed the label Mempool on Jun 1, 2021

fanquake removed the label TX fees and policy on Jun 1, 2021

fanquake added the label Refactoring on Jun 1, 2021

jnewbery commented at 12:59 pm on June 1, 2021: member

Code review ACK 94f4946b9b42b3d7c7c87ffc98a2b5248e91cc46.

Thanks for addressing the outstanding review comments!

in src/policy/packages.h:18 in 94f4946b9b outdated

14@@ -14,6 +15,7 @@
15 static constexpr uint32_t MAX_PACKAGE_COUNT{25};
16 /** Default maximum total virtual size of transactions in a package in KvB. */
17 static constexpr uint32_t MAX_PACKAGE_SIZE{101};
18+static_assert(MAX_PACKAGE_SIZE * 4 * 1000 >= MAX_STANDARD_TX_WEIGHT);

ariard commented at 5:30 pm on June 1, 2021:

Can you switch the magic constant number of 4 by WITNESS_SCALE_FACTOR ?

It won’t be required if #22097 gets first, but I don’t care about the order, I can take the change there.

glozow commented at 9:03 am on June 2, 2021:

yeah that makes sense, done

in src/validation.cpp:626 in 94f4946b9b outdated

620@@ -619,6 +621,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
621     {
622         const CTransaction* ptxConflicting = m_pool.GetConflictTx(txin.prevout);
623         if (ptxConflicting) {
624+            if (!args.m_allow_bip125_replacement) {
625+                // Transaction conflicts with a mempool tx, but we're not allowing replacements.
626+                return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict");

ariard commented at 5:32 pm on June 1, 2021:

What do you think about updating the reject reason to “bip125-replacement-disallowed” ?

glozow commented at 9:03 am on June 2, 2021:

sure, updated

ariard commented at 5:33 pm on June 1, 2021: member

Otherwise, looks good to me

[package] static_assert max package size >= max tx size 6c5f19d9c4

[rpc] reserve space in txns 7d91442461

[refactor] comment/naming improvements e8ecc621be

disallow_mempool_conflicts -> allow_bip125_replacement and check earlier 5cac95cd15

glozow force-pushed on Jun 2, 2021

glozow commented at 9:42 am on June 2, 2021: member

Addressed @ariard’s comments

jnewbery commented at 10:16 am on June 2, 2021: member

reACK 5cac95cd15da04b83afa1d31a43be9f5b30a1827

glozow commented at 3:50 pm on June 2, 2021: member

@ariard I looked back at the PR and just remembered your #20833 (review) about encapsulating the package policies, so I’ve added that commit and co-authored you.

in src/validation.cpp:1087 in 0d7bbcd9f0 outdated

1084@@ -1085,63 +1085,14 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
1085 
1086     PackageValidationState package_state;
1087     const unsigned int package_count = txns.size();

jnewbery commented at 4:19 pm on June 2, 2021:

This local variable is no longer needed:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index fef880152a..a5d7966b09 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -1083,13 +1083,12 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 5 {
 6     AssertLockHeld(cs_main);
 7 
 8-    PackageValidationState package_state;
 9-    const unsigned int package_count = txns.size();
10     // These context-free checks can be done before grabbing the mempool lock.
11+    PackageValidationState package_state;
12     if (!CheckPackage(txns, package_state)) return PackageMempoolAcceptResult(package_state, {});
13 
14     std::vector<Workspace> workspaces{};
15-    workspaces.reserve(package_count);
16+    workspaces.reserve(txns.size());
17     std::transform(txns.cbegin(), txns.cend(), std::back_inserter(workspaces),
18                    [](const auto& tx) { return Workspace(tx); });
19     std::map<const uint256, const MempoolAcceptResult> results;

glozow commented at 4:29 pm on June 2, 2021:

done

MOVEONLY: context-free package policies

Co-authored-by: ariard <antoine.riard@gmail.com>

ee862d6efb

glozow force-pushed on Jun 2, 2021

jnewbery commented at 3:28 pm on June 3, 2021: member

utACK ee862d6efb4c3c01e55f0d5d7a82cce75323cf40

Checked that final commit is move-only

in src/rpc/rawtransaction.cpp:908 in e8ecc621be outdated

904@@ -905,7 +905,7 @@ static RPCHelpMan testmempoolaccept()
905                 RPCResult{
906                     RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n"
907                         "Returns results for each transaction in the same order they were passed in.\n"
908-                        "It is possible for transactions to not be fully validated ('allowed' unset) if an earlier transaction failed.\n",
909+                        "It is possible for transactions to not be fully validated ('allowed' unset) if another transaction failed.\n",

harding commented at 7:20 pm on June 7, 2021:

In [refactor] comment/naming improvements

I think this is a bit confusing: the reader hasn’t be introduced to the allowed field yet, so they don’t know it’s part of the results. When I read this, I thought it was an input parameter that I had missed. I don’t think this is important, but if you edit, maybe this would be clearer:

"Transactions that cannot be fully validated due to failures in other transactions will not contain an 'allowed' result.\n"

glozow commented at 9:28 am on June 9, 2021:

Good point, I’ll do this if I need to retouch or in a followup

glozow commented at 10:47 am on June 10, 2021:

Added to #21800

in src/validation.h:243 in e8ecc621be outdated

242-*                                   parent-child dependencies. The transactions must not conflict, i.e.
243-*                                   must not spend the same inputs, even if it would be a valid BIP125
244-*                                   replace-by-fee. Parents must appear before children.
245+*                                   parent-child dependencies. The transactions must not conflict
246+*                                   with each other, i.e., must not spend the same inputs. If any
247+*                                   dependencies exist, parents must appear before children.

harding commented at 7:24 pm on June 7, 2021:

In [refactor] comment/naming improvements

µ-nit: maybe “parents must appear anywhere in the list before their children”. Otherwise someone could mistakenly infer that a parent needed to appear immediately before its children.

glozow commented at 9:29 am on June 9, 2021:

Agree, will do this if I need to retouch or in a followup

glozow commented at 10:47 am on June 10, 2021:

Added to #21800

harding commented at 7:39 pm on June 7, 2021: contributor

A couple language nits in case this gets updated, otherwise LGTM. I didn’t review the move-only commit.

in src/validation.cpp:1109 in ee862d6efb

1108-        // impossible, we don't need to track the coins spent. Note that this logic will need to be
1109-        // updated if RBFs in packages are allowed in the future.
1110-        assert(args.disallow_mempool_conflicts);
1111+        // package to spend. Since we already checked conflicts in the package and we don't allow
1112+        // replacements, we don't need to track the coins spent. Note that this logic will need to be
1113+        // updated if package replace-by-fee is allowed in the future.

ariard commented at 9:24 pm on June 8, 2021:

plot twist: in fact we may need package replace-by-feerate :)

ariard commented at 9:26 pm on June 8, 2021: member

Code Review ACK ee862d6

Thanks for taking suggestions.

fanquake merged this on Jun 10, 2021

fanquake closed this on Jun 10, 2021

glozow deleted the branch on Jun 10, 2021

sidhujag referenced this in commit e21cd3b9fe on Jun 10, 2021

gwillen referenced this in commit b413362d11 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

package testmempoolaccept followups #22084

Conflicts