#29941 Recreated a pull request because there was a conflict. Trying to resolve the conflict but the old one automatically closed.
Add missing comparison for TODO comments in mempool_packages.py
Also, notice that the ancestor size limits and descendant size limits actually implemented in #21800 , so I removed the todo for those two size limits.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | alfonsoromanz, kevkevinpal, rkrux, achow101 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
ACK e912717
looks good to me
@Umiiii I am not sure why you are asking me because I am not familiar with this change yet. I’m sure a maintainer will take a look at this change soon. You need to have a bit of patience though, there are a lot of open PRs and Issues and this being open for 3 days is not a very long time. And in general, more review is always better, 3 ACKs is the rule of thumb for simple but non-trivial changes.
And please remove the @ mentions in your commit message. The commit message is merged with the change and that leads to a lot of unnecessary notifications for those that are tagged.
205- for tx in mempool:
206- assert_equal(mempool[tx]['unbroadcast'], False)
207-
208- # TODO: test ancestor size limits
209+ entry0 = self.nodes[0].getmempoolentry(tx)
210+ entry1 = self.nodes[1].getmempoolentry(tx)
mempool1 only stores string(txid), while getmempoolentry contains more information that we need to check, so no, it is not redundant. Also see: https://developer.bitcoin.org/reference/rpc/getmempoolentry.html
getrawmempool is also needed, got it.
250@@ -251,10 +251,14 @@ def run_test(self):
251 assert tx in mempool1
252 for tx in chain[CUSTOM_DESCENDANT_LIMIT:]:
253 assert tx not in mempool1
254- # TODO: more detailed check of node1's mempool (fees etc.)
255-
256- # TODO: test descendant size limits
260+ entry1 = self.nodes[1].getmempoolentry(tx)
261+ assert not entry0['unbroadcast']
262+ assert not entry1['unbroadcast']
263+ assert_equal(entry1['fees']['base'], entry0['fees']['base'])
264+ assert_equal(entry1['vsize'], entry0['vsize'])
265+ assert_equal(entry1['depends'], entry0['depends'])
compareMempoolsEntry(entry0, entry1)
tACK e912717
Make successful, all functional tests pass. Left couple suggestions.
Also for some reason checking out this PR using this method doesn’t work, I had to checkout this particular commit instead. Is it because the forked repo’s branch is also called master?
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally
