doc: Fix several references in txmempool comments #21436

pull kiminuo wants to merge 1 commits into bitcoin:master from kiminuo:feature/2021-03-mempool-nits changing 2 files +12 −12

kiminuo commented at 12:11 PM on March 14, 2021: contributor

It seems that not all documentation comments were updated in #19478 correctly.
kiminuo renamed this:
~~Documentation fixes after #19478~~
doc: Fixes after #19478
on Mar 14, 2021
fanquake added the label Docs on Mar 14, 2021
jonatack commented at 2:12 PM on March 14, 2021: member

Concept ACK, can you pick up the remaining documentation fixes noted in #19478?

(While here, could fixup s/must updated/must update/ in txmempool.h::L610)
kiminuo force-pushed on Mar 14, 2021
kiminuo commented at 9:27 PM on March 14, 2021: contributor

Yes, the docs were objectively wrong, I probably fucked up the autocomplete when changing it, and if someone wants to fix it before I get around to it that's dandy. Just noting that the prior name -- setMemPoolChildren -- was wrong (no such member existed) for like 5 years and no one fixed it (and even more wrong -- there was no field anywhere), so I sort of doubt anyone is actually reading the docs to learn how the code works anyways.

(https://github.com/bitcoin/bitcoin/pull/19478#issuecomment-688417629) @JeremyRubin FWIW, I read the docs today - by chance and out of curiosity - and the documentation got me confused. So count me in. :)

(While here, could fixup s/must updated/must update/ in txmempool.h::L610)

👍 Fixed.

can you pick up the remaining documentation fixes noted in #19478? @jonatack My original goal was to learn more about mempool today, so my contribution here is meant as fixing "obviously wrong things" and certainly not details as I don't know them.

There are two remaining cases returned by git grep --extended-regexp '(setMemPool|mapLinks|CTxMemPool::(Parents|m_children))' (see #19478 (comment)). It would be great to fix them too but I'm not sure about proper fixes.
kiminuo requested review from jonatack on Mar 15, 2021
RiccardoMasutti approved
RiccardoMasutti commented at 7:46 PM on March 15, 2021: contributor

ACK 09adbcc

MarcoFalke commented at 7:34 AM on March 22, 2021: member

$ git grep --extended-regexp '(setMemPool|mapLinks|CTxMemPool::(Parents|m_children))' src
src/txmempool.h: * the set of in-mempool direct parents and direct children in mapLinks.  Within
src/txmempool.h: * mapLinks may not be correct (and therefore functions like

MarcoFalke commented at 8:37 AM on March 22, 2021: member

Also, please adjust the title to something more meaningful. Otherwise it will be hard to understand what the changes are by looking at the title.
kiminuo renamed this:
~~doc: Fixes after #19478~~
doc: Fix several txmempool comments
on Mar 22, 2021
kiminuo commented at 8:46 AM on March 22, 2021: contributor
@MarcoFalke
$ git grep --extended-regexp '(setMemPool|mapLinks|CTxMemPool::(Parents|m_children))' src src/txmempool.h: * the set of in-mempool direct parents and direct children in mapLinks. Within src/txmempool.h: * mapLinks may not be correct (and therefore functions like
As I wrote above:

There are two remaining cases returned by git grep --extended-regexp '(setMemPool|mapLinks|CTxMemPool::(Parents|m_children))' (see #19478 (comment)). It would be great to fix them too but I'm not sure about proper fixes.
kiminuo renamed this:
~~doc: Fix several txmempool comments~~
doc: Fix several references in txmempool comments
on Mar 22, 2021

in src/txmempool.h:823 in 09adbcc01a outdated

 819 | @@ -820,8 +820,8 @@ class CTxMemPool
 820 |  
 821 |      /** Before calling removeUnchecked for a given transaction,
 822 |       *  UpdateForRemoveFromMempool must be called on the entire (dependent) set
 823 | -     *  of transactions being removed at the same time.  We use each
 824 | -     *  CTxMemPoolEntry's setMemPoolParents in order to walk ancestors of a
 825 | +     *  of transactions being removed at the same time.  We use

JeremyRubin commented at 4:08 PM on March 23, 2021:

each should remain

kiminuo commented at 4:48 PM on March 23, 2021:

Addressed. Thanks.

kiminuo commented at 7:41 PM on March 23, 2021:

Thanks!

in src/txmempool.h:443 in 09adbcc01a outdated

 440 |   * - update the new entry's direct parents to include the new tx as a child
 441 |   * - update all ancestors of the transaction to include the new tx's size/fee
 442 |   *
 443 |   * When a transaction is removed from the mempool, we must:
 444 | - * - update all in-mempool parents to not track the tx in setMemPoolChildren
 445 | + * - update all in-mempool parents to not track the tx in CTxMemPoolEntry::m_children

JeremyRubin commented at 4:09 PM on March 23, 2021:

should also probably make this "each"

kiminuo commented at 4:48 PM on March 23, 2021:

Addressed. Thanks.

kiminuo commented at 7:41 PM on March 23, 2021:

Thanks!

JeremyRubin commented at 4:13 PM on March 23, 2021: contributor

These mostly look like correct incremental improvements.

I still worry the docs are generally stale/misleading, but that's a much larger effort that shouldn't preclude minor fixups.
kiminuo force-pushed on Mar 23, 2021

in src/txmempool.cpp:119 in 106f9bb0ab outdated

 118 | @@ -119,7 +119,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashes
 119 |      // Iterate in reverse, so that whenever we are looking at a transaction
 120 |      // we are sure that all in-mempool descendants have already been processed.
 121 |      // This maximizes the benefit of the descendant cache and guarantees that
 122 | -    // CTxMemPool::m_children will be updated, an assumption made in
 123 | +    // CTxMemPoolEntry::m_children will be updated, an assumption made in

jarolrod commented at 3:52 AM on May 27, 2021:

while here maybe refactor into a doxygen comment

glozow commented at 8:37 AM on September 2, 2021:

+1, I think the assumptions made by the function are something that should be in the doxygen comment above its signature in txmempool.h

jarolrod commented at 3:52 AM on May 27, 2021: member

Concept ACK
fanquake requested review from glozow on Sep 2, 2021
in src/txmempool.h:680 in 106f9bb0ab outdated
```
 676 | @@ -677,7 +677,7 @@ class CTxMemPool
 677 |       *  limitDescendantSize = max size of descendants any ancestor can have
 678 |       *  errString = populated with error reason if any limits are hit
 679 |       *  fSearchForParents = whether to search a tx's vin for in-mempool parents, or
 680 | -     *    look up parents from mapLinks. Must be true for entries not in the mempool
 681 | +     *    look up parents from CTxMemPoolEntry::m_parents. Must be true for entries not in the mempool
```
glozow commented at 8:42 AM on September 2, 2021:

This is just my opinion, but I think it's more helpful to refer to the specific parameter here. Same thing for the other docs.

* look up parents from entry.m_parents. Must be true for entries not in the mempool
kiminuo commented at 7:06 PM on September 2, 2021:

I think the suggestion is very good in general. But let me just show what I would actually love:

* look up parents from <see cref="entry.m_parents"/>. Must be true for entries not in the mempool

(Syntax borrowed from C#)

This is, of course, just made up syntax but it would be great to reference parameter's member (m_parents) in Doxygen. The only problem is that I'm unable to find the syntax for Doxygen, if it actually supports it.

Do you possibly know?
kiminuo commented at 7:53 PM on September 2, 2021:

:tada: \link CTxMemPoolEntry::m_children updateIt.m_children \endlink seems to do the job.

Resources:

https://www.doxygen.nl/manual/autolink.html

https://www.doxygen.nl/manual/examples/autolink/html/class_autolink___test.html

Edit: This is an example from generated Doxygen page:
mzumsande commented at 3:59 PM on January 5, 2022:

Not sure if there have been discussions about this, but my feeling is that most devs read the source rather than the actual doxygen-generated file (at least I do), so I don't like special commands like \link ... \endlink that improve the doxygen-generated documentation, but come at the cost of disturbing the reading flow when looking at the source.

kiminuo commented at 10:00 AM on January 17, 2022:

I understand your point of view. An alternative point of view is that doxygen can warn us that a reference no longer exists so that it can be fixed in a PR and it would not be necessary to dig where something changed and why (this PR).

Personally, I don't think it would be that bad to have \link ... \endlink (I agree it's not visually nice). However, we already use doxygen syntax so a clear rule explaining why this is "too much" should be presented I think.

Having said that, I can remove it if you insist but I would rather not.
glozow commented at 8:46 AM on September 2, 2021: member

ACK 106f9bb

The new comments look correct to me. Feel free to ignore my suggestions, they're just my opinions.

Edit: also, if you have the chance in this PR or another, should remove all mentions of mapLinks as a followup to #19478 #21436 (comment)
Documentation fixes after #19478 (Remove CTxMempool::mapLinks data structure member) e54a411c32
kiminuo force-pushed on Sep 2, 2021
DrahtBot commented at 6:47 AM on September 3, 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:
- #21464 (Mempool Update Cut-Through Optimization by JeremyRubin)
- #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
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.
glozow commented at 10:09 AM on September 3, 2021: member

Remove other mentions of mapLinks as well? #21436 (comment)
kiminuo commented at 10:24 AM on September 3, 2021: contributor

Remove other mentions of mapLinks as well? #21436 (comment)

I'm not sure how to fix it really as I said here: #21436 (comment) and here #21436 (comment). I'm open to suggestions!
DrahtBot commented at 4:38 AM on January 25, 2022: member



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
DrahtBot added the label Needs rebase on Jan 25, 2022
kiminuo closed this on Jan 25, 2022
kiminuo deleted the branch on Jan 25, 2022
DrahtBot locked this on Jan 25, 2023