package testmempoolaccept followups #22084
pull glozow wants to merge 5 commits into bitcoin:master from glozow:20833-followups changing 10 files +120 −84-
glozow commented at 8:40 am on May 27, 2021: membervarious 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: memberI 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
-
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);
glozow commented at 9:03 am on June 2, 2021:yeah that makes sense, donein 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, updatedariard commented at 5:33 pm on June 1, 2021: memberOtherwise, looks good to me[package] static_assert max package size >= max tx size 6c5f19d9c4[rpc] reserve space in txns 7d91442461[refactor] comment/naming improvements e8ecc621bedisallow_mempool_conflicts -> allow_bip125_replacement and check earlier 5cac95cd15glozow force-pushed on Jun 2, 2021jnewbery commented at 10:16 am on June 2, 2021: memberreACK 5cac95cd15da04b83afa1d31a43be9f5b30a1827glozow 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:doneMOVEONLY: context-free package policies
Co-authored-by: ariard <antoine.riard@gmail.com>
glozow force-pushed on Jun 2, 2021jnewbery commented at 3:28 pm on June 3, 2021: memberutACK 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
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
harding commented at 7:39 pm on June 7, 2021: contributorA 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: memberCode Review ACK ee862d6
Thanks for taking suggestions.
fanquake merged this on Jun 10, 2021fanquake closed this on Jun 10, 2021
glozow deleted the branch on Jun 10, 2021sidhujag referenced this in commit e21cd3b9fe on Jun 10, 2021gwillen referenced this in commit b413362d11 on Jun 1, 2022DrahtBot locked this on Aug 16, 2022
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me