Hi,
I have been reading mempool_tests.cpp file and maybe there are two things that can be improved in MempoolIndexingTest:
1) Typo
Line https://github.com/bitcoin/bitcoin/blob/4ad83a9597e4f5340742d080e26437c5d8d203ba/src/test/mempool_tests.cpp#L223 should be:
// Now tx8 should be sorted low, but tx6/tx7 both high
2) Extra lines
It looks to me that these two lines are not needed:
The PR #12127 (here) is the PR to understand the current status quo.
PS: A digram for the MempoolIndexingTest would make it easier to follow.
3) Asserted order
I would add a comment like this
// Verify order: tx8 (0), tx3 (0), tx5 (10k), tx1 (10k), tx4 (15k), tx2 (20k), tx6 (0+2000k) and tx7 (2000k)
for https://github.com/bitcoin/bitcoin/blob/4ad83a9597e4f5340742d080e26437c5d8d203ba/src/test/mempool_tests.cpp#L225 line as at that point, it's probably not that easy to know where we are unless one uses pen and paper.
4) GetTransactionAncestry in txmempool.{h,cpp}
GetTransactionAncestry is described here: https://github.com/bitcoin/bitcoin/blob/1e3db6807d1809182c799b5f7a78843f03fbe413/src/txmempool.h#L707-L710 but the comment:
- Calculate the ancestor and descendant count for the given transaction.
- The counts include the transaction itself.
may be improved with better wording because descendants does not represent the number of descendants of the txid transaction but rather a maximum number of descendants of the txid transaction or any of the txid parents (parents are searched recursively) if I read the code correctly. In any case, it may be helpful to rename descendants to maxDescendants not to confuse a reader.
