doc: Fix several references in txmempool comments by kiminuo · Pull Request #21436 · bitcoin/bitcoin

kiminuo · 2021-03-14T12:11:48Z

It seems that not all documentation comments were updated in #19478 correctly.

jonatack

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 · 2021-03-14T21:27:16Z

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.

(#19478 (comment))

@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.

RiccardoMasutti

ACK 09adbcc

maflcko · 2021-03-22T07:34:10Z

$ 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

maflcko · 2021-03-22T08:37:29Z

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 · 2021-03-22T08:46:43Z

@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.

src/txmempool.h

JeremyRubin · 2021-03-23T16:13:53Z

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.

jarolrod

Concept ACK

jarolrod · 2021-05-27T03:52:34Z

src/txmempool.cpp

    // we are sure that all in-mempool descendants have already been processed.
    // This maximizes the benefit of the descendant cache and guarantees that
-    // CTxMemPool::m_children will be updated, an assumption made in
+    // CTxMemPoolEntry::m_children will be updated, an assumption made in


while here maybe refactor into a doxygen comment

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

glozow

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)

glozow · 2021-09-02T08:37:51Z

src/txmempool.cpp

    // we are sure that all in-mempool descendants have already been processed.
    // This maximizes the benefit of the descendant cache and guarantees that
-    // CTxMemPool::m_children will be updated, an assumption made in
+    // CTxMemPoolEntry::m_children will be updated, an assumption made in


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

glozow · 2021-09-02T08:42:31Z

src/txmempool.h

     *  errString = populated with error reason if any limits are hit
     *  fSearchForParents = whether to search a tx's vin for in-mempool parents, or
-     *    look up parents from mapLinks. Must be true for entries not in the mempool
+     *    look up parents from CTxMemPoolEntry::m_parents. Must be true for entries not in the mempool


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.

Suggested change

* look up parents from CTxMemPoolEntry::m_parents. Must be true for entries not in the mempool

* look up parents from entry.m_parents. Must be true for entries not in the mempool

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?

🎉 \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:

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.

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.

…data structure member)

DrahtBot · 2021-09-03T06:47:46Z

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 · 2021-09-03T10:09:40Z

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

kiminuo · 2021-09-03T10:24:13Z

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 · 2022-01-25T04:38:34Z

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

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

kiminuo changed the title ~~Documentation fixes after #19478~~ doc: Fixes after #19478 Mar 14, 2021

fanquake added the Docs label Mar 14, 2021

jonatack reviewed Mar 14, 2021

View reviewed changes

kiminuo force-pushed the feature/2021-03-mempool-nits branch from d8a942c to 09adbcc Compare March 14, 2021 21:20

kiminuo requested a review from jonatack March 15, 2021 11:22

RiccardoMasutti approved these changes Mar 15, 2021

View reviewed changes

kiminuo changed the title ~~doc: Fixes after #19478~~ doc: Fix several txmempool comments Mar 22, 2021

kiminuo changed the title ~~doc: Fix several txmempool comments~~ doc: Fix several references in txmempool comments Mar 22, 2021

JeremyRubin reviewed Mar 23, 2021

View reviewed changes

src/txmempool.h Outdated Show resolved Hide resolved

JeremyRubin reviewed Mar 23, 2021

View reviewed changes

src/txmempool.h Outdated Show resolved Hide resolved

kiminuo force-pushed the feature/2021-03-mempool-nits branch from 09adbcc to 106f9bb Compare March 23, 2021 16:46

jarolrod reviewed May 27, 2021

View reviewed changes

fanquake requested a review from glozow September 2, 2021 03:33

glozow reviewed Sep 2, 2021

View reviewed changes

Documentation fixes after bitcoin#19478 (Remove CTxMempool::mapLinks …

e54a411

…data structure member)

kiminuo force-pushed the feature/2021-03-mempool-nits branch from 106f9bb to e54a411 Compare September 2, 2021 20:15

DrahtBot mentioned this pull request Sep 3, 2021

Mempool Update Cut-Through Optimization #21464

Merged

DrahtBot mentioned this pull request Dec 5, 2021

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

DrahtBot added the Needs rebase label Jan 25, 2022

kiminuo closed this Jan 25, 2022

kiminuo deleted the feature/2021-03-mempool-nits branch January 25, 2022 07:28

bitcoin locked and limited conversation to collaborators Jan 25, 2023

	* look up parents from CTxMemPoolEntry::m_parents. Must be true for entries not in the mempool
	* look up parents from entry.m_parents. Must be true for entries not in the mempool

Conversation

kiminuo commented Mar 14, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

kiminuo commented Mar 14, 2021

Uh oh!

RiccardoMasutti left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 22, 2021

Uh oh!

maflcko commented Mar 22, 2021

Uh oh!

kiminuo commented Mar 22, 2021

Uh oh!

Uh oh!

Uh oh!

JeremyRubin commented Mar 23, 2021

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod May 27, 2021

Choose a reason for hiding this comment

Uh oh!

glozow Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

glozow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

glozow Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

kiminuo Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiminuo Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

kiminuo Jan 17, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

glozow commented Sep 3, 2021

Uh oh!

kiminuo commented Sep 3, 2021

Uh oh!

DrahtBot commented Jan 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

glozow left a comment •

edited

Loading

kiminuo Sep 2, 2021 •

edited

Loading

kiminuo Sep 2, 2021 •

edited

Loading

DrahtBot commented Sep 3, 2021 •

edited

Loading