refactor: CTxMempool constructor clean up #20222

1. 
   ellemouton commented at 5:49 pm on October 22, 2020: contributor

   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.

2. 
   jnewbery commented at 5:52 pm on October 22, 2020: member

   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 :slightly_smiling_face:
3. 
   practicalswift commented at 6:46 pm on October 22, 2020: contributor

   Concept ACK

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

4. DrahtBot added the label Mempool on Oct 22, 2020
5. DrahtBot added the label Refactoring on Oct 22, 2020
6. 
   DrahtBot commented at 10:34 pm on October 22, 2020: 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:

   • #18766 (Disable fee estimation in blocksonly mode (by removing the fee estimates global) by darosior)
   • #18017 (txmempool: split epoch logic into class by ajtowns)
   • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
   • #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.

7. in src/init.cpp:1393 in a62501cc78 outdated

   1397-        int ratio = std::min<int>(std::max<int>(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000);
   1398-        if (ratio != 0) {
   1399-            node.mempool->setSanityCheck(1.0 / ratio);
   1400-        }
   1401-    }
   1402+    int check_ratio = std::min<int>(std::max<int>(args.GetArg("-checkmempool", chainparams.DefaultConsistencyChecks() ? 1 : 0), 0), 1000000);
   

   ---

   ---

   

   glozow commented at 3:26 am on October 23, 2020:

   I understand this behavior isn’t being changed but… default ratio is 0 or 1 (never or always?), unless user gives a value? :O

   ---

   

   ellemouton commented at 4:00 am on October 23, 2020:

   Indeed! The default for all chains is 0 except for regtest which has a default of 1.
8. 
   glozow commented at 3:48 am on October 23, 2020: member

   Concept ACK 😄 what a cool first contribution! Code looks good to me. Linter is failing on trailing whitespace in the CTxMemPool constructor doxygen comments
9. ellemouton force-pushed on Oct 23, 2020
10. in src/txmempool.cpp:617 in c65a593817 outdated

    613@@ -619,13 +614,12 @@ static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& m
    614 
    615 void CTxMemPool::check(const CCoinsViewCache *pcoins) const
    616 {
    617-    LOCK(cs);
    618-    if (nCheckFrequency == 0)
    619-        return;
    620+    if (m_check_ratio == 0) return;
    

    ---

    ---

    

    jnewbery commented at 8:04 am on October 23, 2020:

    You can join the if (m_check_ratio == 0) with the return; line in the first commit instead of the second commit.

    ---

    

    ellemouton commented at 12:18 pm on October 23, 2020:

    updated 👍
11. in src/txmempool.cpp:619 in c65a593817 outdated

    618-    if (nCheckFrequency == 0)
    619-        return;
    620+    if (m_check_ratio == 0) return;
    621 
    622-    if (GetRand(std::numeric_limits<uint32_t>::max()) >= nCheckFrequency)
    623+    if (GetRand(m_check_ratio) >= 1)
    

    ---

    ---

    

    jnewbery commented at 8:05 am on October 23, 2020:

    This can be joined with the line below (in the first commit)

    ---

    

    ellemouton commented at 12:18 pm on October 23, 2020:

    updated 👍
12. 
    jnewbery commented at 8:06 am on October 23, 2020: member

    utACK c65a5938177cdbc975a983873495d38392fd82d1

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

13. ellemouton force-pushed on Oct 23, 2020
14. refactor: Avoid double to int cast for nCheckFrequency

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

    9d4b4b2c2c
15. in src/txmempool.cpp:617 in 70768e1418 outdated

    614@@ -620,12 +615,11 @@ static void CheckInputsAndUpdateCoins(const CTransaction& tx, CCoinsViewCache& m
    615 void CTxMemPool::check(const CCoinsViewCache *pcoins) const
    616 {
    617     LOCK(cs);
    

    ---

    ---

    

    jnewbery commented at 12:34 pm on October 23, 2020:

    Oops. You’ve added this back in your rebase.

    ---

    

    ellemouton commented at 12:37 pm on October 23, 2020:

    Thanks for catching that! Will fix 👍
16. ellemouton force-pushed on Oct 23, 2020
17. refactor: Make CTxMemPool::m_check_ratio a const and a constructor argument

    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.

    e3310692d0
18. refactor: Clean up CTxMemPool initializer list

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

    f15e780b9e
19. 
    jnewbery commented at 12:43 pm on October 23, 2020: member

    utACK f15e780b9e57554c723bc02aa41150ecf3e3a8c9
20. 
    glozow commented at 8:34 pm on October 23, 2020: member

    utACK https://github.com/bitcoin/bitcoin/pull/20222/commits/f15e780b9e57554c723bc02aa41150ecf3e3a8c9
21. 
    theStack commented at 8:26 pm on October 25, 2020: member

    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?

22. 
    ellemouton commented at 5:40 am on October 27, 2020: contributor

    Thanks @theStack 😄

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

23. 
    sipa commented at 5:44 am on October 27, 2020: member

    @theStack That’s exactly the reason.
24. 
    jnewbery commented at 9:24 am on October 27, 2020: member

    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.

25. 
    ellemouton commented at 9:59 am on October 31, 2020: contributor

    cool, had a look now and I think @jnewbery is correct, GetRand() used to use a modulo operation but that changed in commit 152146e782d401aa1ce7d989d62306aabc85f22e.
26. in src/init.cpp:1393 in e3310692d0 outdated

    1388@@ -1389,14 +1389,9 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
    1389     assert(!node.connman);
    1390     node.connman = MakeUnique<CConnman>(GetRand(std::numeric_limits<uint64_t>::max()), GetRand(std::numeric_limits<uint64_t>::max()), args.GetBoolArg("-networkactive", true));
    1391 
    1392-    // Make mempool generally available in the node context. For example the connection manager, wallet, or RPC threads,
    1393-    // which are all started after this, may use it from the node context.
    

    ---

    ---

    

    theStack commented at 6:02 pm on November 1, 2020:

    Just to get sure: was the removal of this comments intended?

    ---

    

    ellemouton commented at 5:38 am on November 3, 2020:

    I think the comment is outdated and refers to making the mempool available in the node context as a raw pointer. But now the mempool is constructed here.

    ---

    

    jnewbery commented at 9:09 am on November 3, 2020:

    Exactly right. This comment should have been removed in the commit here: https://github.com/bitcoin/bitcoin/commit/fafb381af8279b2d2ca768df0bf68d7eb036a2f9#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69dR1367. Previously, we were taking an existing pointer and making it available in the node context. Now we’re constructing a new mempool here.
27. theStack approved
28. 
    theStack commented at 6:05 pm on November 1, 2020: member

    Code Review ACK f15e780b9e57554c723bc02aa41150ecf3e3a8c9
29. 
    theStack commented at 6:16 pm on November 1, 2020: member

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

30. 
    MarcoFalke commented at 9:01 am on December 1, 2020: member

    ACK f15e780b9e57554c723bc02aa41150ecf3e3a8c9 👘

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK f15e780b9e57554c723bc02aa41150ecf3e3a8c9 👘
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUi4bQv7BHwX3R87hsf4rDy4IO4itsm9oYcMsTz6EMYkRFJhiD7Kb6+3Co3CR6wo
     8S3iuKBJdiLePALCH+u27iqQ+pJEH7evrHZWQYwM7jEuCO0CerHkPONZwcQTl8/V7
     9OOmEF7bfCMOJtrx2+yjgkg6HfL7ly+L5zwHt7UQdyXUdSJg03kdjd0CEOfh2yfaW
    10hA3+rH5SMLtfULhYGqM8XXXn0Ek619gjTla4axZ0Eh5ytPxorvxoxqW6SRqDrHjM
    11iU4Zs3qJcNgP64teR5IVtnBZE9FCR3puQzH0UD0ddvIpRo1b6agaJ2JOtM69lrcw
    12Hkb4y2rF+vSUzXZYog1HnPzxeMiMlPDHjm0Maaygx897igHBr0Tnmksog5WGwliS
    13vV5cDHhEKLdDVTaVCeK78dxys0Ohhvvj9m4y7Xu42y0a5Qey9gQSYOetT0MA/5zQ
    14+C+YWV5lyigypdM9jO1DwwelPAb1orIvGt91b96kuowU3DCArIcYmUKFUcS0oKbh
    15mI+1MDT/
    16=AV9k
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 548f07ab88d61d2cf2c5d77ad9d0bd700f42cfa1d87e97bf8649532160b609a5 -

31. MarcoFalke merged this on Dec 1, 2020
32. MarcoFalke closed this on Dec 1, 2020

    ---

33. sidhujag referenced this in commit fae901e4b9 on Dec 1, 2020
34. Fabcien referenced this in commit 4577440479 on Jan 19, 2022
35. Fabcien referenced this in commit ae6af22fb1 on Jan 19, 2022
36. Fabcien referenced this in commit f62674e361 on Jan 19, 2022
37. DrahtBot locked this on Feb 15, 2022

Contributors
ellemouton jnewbery practicalswift DrahtBot glozow theStack sipa MarcoFalke

Labels
Refactoring Mempool