package testmempoolaccept followups #22084
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
I think this is ready for rebase and review now :) |
|
Code review ACK 94f4946. Thanks for addressing the outstanding review comments! |
|
Addressed @ariard's comments |
|
reACK 5cac95c |
|
@ariard I looked back at the PR and just remembered your #20833 (comment) about encapsulating the package policies, so I've added that commit and co-authored you. |
Co-authored-by: ariard <antoine.riard@gmail.com>
|
utACK ee862d6 Checked that final commit is move-only |
| @@ -905,7 +905,7 @@ static RPCHelpMan testmempoolaccept() | |||
| RPCResult{ | |||
| RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n" | |||
| "Returns results for each transaction in the same order they were passed in.\n" | |||
| "It is possible for transactions to not be fully validated ('allowed' unset) if an earlier transaction failed.\n", | |||
| "It is possible for transactions to not be fully validated ('allowed' unset) if another transaction failed.\n", | |||
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Good point, I'll do this if I need to retouch or in a followup
| * replace-by-fee. Parents must appear before children. | ||
| * parent-child dependencies. The transactions must not conflict | ||
| * with each other, i.e., must not spend the same inputs. If any | ||
| * dependencies exist, parents must appear before children. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Agree, will do this if I need to retouch or in a followup
| assert(args.disallow_mempool_conflicts); | ||
| // package to spend. Since we already checked conflicts in the package and we don't allow | ||
| // replacements, we don't need to track the coins spent. Note that this logic will need to be | ||
| // updated if package replace-by-fee is allowed in the future. |
There was a problem hiding this comment.
plot twist: in fact we may need package replace-by-feerate :)
various followups from #20833