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 :) |
glozow
changed the title
|
Code review ACK 94f4946b9b42b3d7c7c87ffc98a2b5248e91cc46. 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 <[email protected]>
|
utACK ee862d6 Checked that final commit is move-only |
harding
left a comment
harding
left a comment
There was a problem hiding this comment.
A couple language nits in case this gets updated, otherwise LGTM. I didn't review the move-only commit.
| 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 :)
6 participants
various followups from #20833