refactor: CTxMempool constructor clean up #20222

ellemouton · 2020-10-22T17:49:49Z

This PR cleans up the CTxMemPool interface by including the ratio used to determine when a mempool sanity check should run in the constructor of CTxMempool instead of using nCheckFrequency which required a cast from a double to a uint32_t. Since nCheckFrequency (now called m_check_ratio) is set in the constructor and only every read from there after, it can be turned into a const and no longer needs to be guarded by the 'cs' lock.

Since nCheckFrequency/m_check_ratio no longer needs to lock the 'cs' mutux, mutex lock line in the "CTxMempool::check" function can be moved below where the m_check_ratio variable is checked. Since the variable is 0 by default (meaning that "CTxMempool::check" will most likely not run its logic) this saves us from unnecessarily grabbing the lock.

jnewbery · 2020-10-22T17:52:56Z

Concept ACK! Removing unnecessary interface functions, making constant values const, reducing mutex locking and avoiding casts between int and float types are all excellent improvements 🙂

practicalswift · 2020-10-22T18:46:40Z

Concept ACK

Very nice first-time contribution @ellemouton! Warm welcome as a contributor! :)

DrahtBot · 2020-10-22T22:34:34Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Disable fee estimation in blocksonly mode (by removing the fee estimates global) #18766 (Disable fee estimation in blocksonly mode (by removing the fee estimates global) by darosior)
txmempool: split epoch logic into class #18017 (txmempool: split epoch logic into class by ajtowns)
refactor: Nuke policy/fees->mempool circular dependencies #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
Add fee_est tool for debugging fee estimation code #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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

Concept ACK 😄 what a cool first contribution! Code looks good to me. Linter is failing on trailing whitespace in the CTxMemPool constructor doxygen comments

src/init.cpp

src/txmempool.cpp

jnewbery

utACK c65a593

I've left two tiny style comments. Feel free to take or leave.

Use a ratio instead of a frequency that requires a double to int cast for determining how often a mempool sanity check should run.

src/txmempool.cpp

…gument Since m_check_ratio is only set once and since the CTxMemPool object is no longer a global variable, m_check_ratio can be passed into the constructor of CTxMemPool. Since it is only read from after initialization, m_check_ratio can also be made a const and hence no longer needs to be guarded by the cs mutex.

Shorten the CTxMemPool initializer list using default initialization for members that dont depend on the constuctor parameters.

jnewbery · 2020-10-23T12:43:16Z

utACK f15e780

glozow

utACK f15e780

theStack · 2020-10-25T20:26:27Z

Concept ACK
Welcome as a new contributor @ellemouton, very nice first PR! 👍

It would be very interesting on why the original version of the mempool sanity check (introduced in #6776) used a random range of 2^32, needing rather complicated floating-point logic... I guess back then the motivation was to avoid a (potentially expensive) modulo operation?

ellemouton · 2020-10-27T05:40:51Z

Thanks @theStack 😄

Yeah, good question. I will need to look into that a bit!

sipa · 2020-10-27T05:44:15Z

@theStack That's exactly the reason.

jnewbery · 2020-10-27T09:24:43Z

the motivation was to avoid a (potentially expensive) modulo operation?

Correct me if I'm wrong, but GetRand() no longer includes a modulo operation, so we can switch to using GetRand() without worrying about that.

ellemouton · 2020-10-31T09:59:49Z

cool, had a look now and I think @jnewbery is correct, GetRand() used to use a modulo operation but that changed in commit 152146e.

theStack

Code Review ACK f15e780

src/init.cpp

theStack · 2020-11-01T18:16:24Z

the motivation was to avoid a (potentially expensive) modulo operation?

Correct me if I'm wrong, but GetRand() no longer includes a modulo operation, so we can switch to using GetRand() without worrying about that.

Agreed, GetRand() uses FastRandomContext::randrange() now, which in turn generates a random number within a specific range by repeatedly generating random numbers with as many bits as the range (which shouldn't take more than 2 rounds on average if I'm not mistaken).

maflcko · 2020-12-01T09:01:31Z

ACK f15e780 👘

Show signature and timestamp

Signature:

Timestamp of file with hash 548f07ab88d61d2cf2c5d77ad9d0bd700f42cfa1d87e97bf8649532160b609a5 -

f15e780 refactor: Clean up CTxMemPool initializer list (Elle Mouton) e331069 refactor: Make CTxMemPool::m_check_ratio a const and a constructor argument (Elle Mouton) 9d4b4b2 refactor: Avoid double to int cast for nCheckFrequency (Elle Mouton) Pull request description: This PR cleans up the CTxMemPool interface by including the ratio used to determine when a mempool sanity check should run in the constructor of CTxMempool instead of using nCheckFrequency which required a cast from a double to a uint32_t. Since nCheckFrequency (now called m_check_ratio) is set in the constructor and only every read from there after, it can be turned into a const and no longer needs to be guarded by the 'cs' lock. Since nCheckFrequency/m_check_ratio no longer needs to lock the 'cs' mutux, mutex lock line in the "CTxMempool::check" function can be moved below where the m_check_ratio variable is checked. Since the variable is 0 by default (meaning that "CTxMempool::check" will most likely not run its logic) this saves us from unnecessarily grabbing the lock. ACKs for top commit: jnewbery: utACK f15e780 MarcoFalke: ACK f15e780 👘 glozow: utACK bitcoin@f15e780 theStack: Code Review ACK f15e780 Tree-SHA512: d83f3b5311ca128847b621e5e999c7e1bf0f4e6261d4cc090fb13e229a0f7eecd66ad997f654f50a838baf708d1515740aa3bffc244909a001d01fd5ae398b68

Summary: PR description: > This PR cleans up the CTxMemPool interface by including the ratio used to determine when a mempool sanity check should run in the constructor of CTxMempool instead of using nCheckFrequency which required a cast from a double to a uint32_t. Since nCheckFrequency (now called m_check_ratio) is set in the constructor and only every read from there after, it can be turned into a const and no longer needs to be guarded by the 'cs' lock. > > Since nCheckFrequency/m_check_ratio no longer needs to lock the 'cs' mutux, mutex lock line in the "CTxMempool::check" function can be moved below where the m_check_ratio variable is checked. Since the variable is 0 by default (meaning that "CTxMempool::check" will most likely not run its logic) this saves us from unnecessarily grabbing the lock. Use a ratio instead of a frequency that requires a double to int cast for determining how often a mempool sanity check should run. This is a backport of [[bitcoin/bitcoin#20222 | core#20222]] [1/3] bitcoin/bitcoin@9d4b4b2 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10841

…gument Summary: Since m_check_ratio is only set once and since the CTxMemPool object is no longer a global variable, m_check_ratio can be passed into the constructor of CTxMemPool. Since it is only read from after initialization, m_check_ratio can also be made a const and hence no longer needs to be guarded by the cs mutex. This is a backport of [[bitcoin/bitcoin#20222 | core#20222]] [2/3] bitcoin/bitcoin@e331069 Depends on D10841 Test Plan: With TSAN: `ninja && ninja check check-functional` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10842

Summary: Shorten the CTxMemPool initializer list using default initialization for members that dont depend on the constuctor parameters. This is a backport of [[bitcoin/bitcoin#20222 | core#20222]] [3/3] bitcoin/bitcoin@f15e780 Depends on D10842 Test Plan: `ninja all check-all` eviewers: Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10843

DrahtBot added Mempool Refactoring labels Oct 22, 2020

glozow reviewed Oct 23, 2020

View reviewed changes

src/init.cpp Show resolved Hide resolved

ellemouton force-pushed the ctxmempool_refactor branch from a62501c to c65a593 Compare October 23, 2020 04:18

jnewbery reviewed Oct 23, 2020

View reviewed changes

src/txmempool.cpp Show resolved Hide resolved

jnewbery reviewed Oct 23, 2020

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

jnewbery reviewed Oct 23, 2020

View reviewed changes

ellemouton force-pushed the ctxmempool_refactor branch from c65a593 to 70768e1 Compare October 23, 2020 12:13

refactor: Avoid double to int cast for nCheckFrequency

9d4b4b2

Use a ratio instead of a frequency that requires a double to int cast for determining how often a mempool sanity check should run.

jnewbery reviewed Oct 23, 2020

View reviewed changes

src/txmempool.cpp Outdated Show resolved Hide resolved

ellemouton force-pushed the ctxmempool_refactor branch from 70768e1 to f15e780 Compare October 23, 2020 12:39

ellemouton added 2 commits October 23, 2020 14:41

refactor: Clean up CTxMemPool initializer list

f15e780

Shorten the CTxMemPool initializer list using default initialization for members that dont depend on the constuctor parameters.

glozow reviewed Oct 23, 2020

View reviewed changes

DrahtBot mentioned this pull request Oct 28, 2020

tree-wide: De-globalize ChainstateManager #20158

Closed

12 tasks

theStack approved these changes Nov 1, 2020

View reviewed changes

src/init.cpp Show resolved Hide resolved

maflcko merged commit cd72033 into bitcoin:master Dec 1, 2020

dongcarl mentioned this pull request Dec 4, 2020

tests: Create or use existing properly initialized NodeContexts #20323

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

refactor: CTxMempool constructor clean up #20222

refactor: CTxMempool constructor clean up #20222

Uh oh!

Conversation

ellemouton commented Oct 22, 2020

Uh oh!

jnewbery commented Oct 22, 2020

Uh oh!

practicalswift commented Oct 22, 2020

Uh oh!

DrahtBot commented Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

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!

Uh oh!

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnewbery commented Oct 23, 2020

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Oct 25, 2020

Uh oh!

ellemouton commented Oct 27, 2020

Uh oh!

sipa commented Oct 27, 2020

Uh oh!

jnewbery commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ellemouton commented Oct 31, 2020

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theStack commented Nov 1, 2020

Uh oh!

maflcko commented Dec 1, 2020

Uh oh!

Uh oh!

DrahtBot commented Oct 22, 2020 •

edited

Loading

glozow left a comment •

edited

Loading

jnewbery commented Oct 27, 2020 •

edited

Loading