Remove mempool global from interfaces #19848

MarcoFalke commented at 2:50 PM on August 31, 2020: member

The chain interface has an m_node member, which has a pointer to the mempool global. Use the pointer instead of the global to prepare the removal of the mempool global. See #19556

MarcoFalke added the label Refactoring on Aug 31, 2020

promag commented at 3:03 PM on August 31, 2020: member

Correct me if I'm wrong but m_node.mempool is always set right?

MarcoFalke commented at 3:08 PM on August 31, 2020: member

https://github.com/bitcoin/bitcoin/blob/068bc211881c790225e9979eb16ce4f44fb64e01/src/init.cpp#L1374

Currently it will be set during init until shortly before shutdown is done. Though, in the future it may be nullptr for the whole runtime of the program (see also #19629 (comment))

MarcoFalke cross-referenced this on Aug 31, 2020 from issue Remove mempool global by MarcoFalke

DrahtBot commented at 4:55 PM on August 31, 2020: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19872 (Avoid locking CTxMemPool::cs recursively in some cases by hebasto)
#19572 (ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs)
#19556 (Remove mempool global by MarcoFalke)
#15719 (Wallet passive startup by ryanofsky)
#12677 (RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr)

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.

hebasto commented at 5:42 PM on August 31, 2020: member

Concept ACK.

practicalswift commented at 7:09 PM on August 31, 2020: contributor

Concept ACK

darosior commented at 7:19 PM on August 31, 2020: member

Concept ACK

DrahtBot cross-referenced this on Aug 31, 2020 from issue RPC: Add ancestor{count,size,fees} to listunspent output by luke-jr

DrahtBot cross-referenced this on Sep 1, 2020 from issue ZMQ: Create "sequence" notifier, enabling client-side mempool tracking by instagibbs

jnewbery commented at 8:31 AM on September 2, 2020: member

utACK fa5fa7e025164f9ac73741ece697e3ea4e7ed64c

DrahtBot cross-referenced this on Sep 2, 2020 from issue Wallet passive startup by ryanofsky

in src/policy/rbf.cpp:11 in fa5fa7e025 outdated

   4 | @@ -5,9 +5,10 @@
   5 |  #include <policy/rbf.h>
   6 |  #include <util/rbf.h>
   7 |  
   8 | -RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
   9 | +namespace {
  10 | +RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool* const pool) NO_THREAD_SAFETY_ANALYSIS
  11 |  {
  12 | -    AssertLockHeld(pool.cs);
  13 | +    if (pool) AssertLockHeld(pool->cs);

hebasto commented at 5:02 PM on September 2, 2020:

Could we avoid:

adding new NO_THREAD_SAFETY_ANALYSIS
conditional AssertLockHeld(): https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-conditionally-held-locks

?

MarcoFalke commented at 8:08 AM on September 3, 2020:

Fixed

ryanofsky approved

ryanofsky commented at 5:23 PM on September 2, 2020: contributor

Code review ACK fa5fa7e025164f9ac73741ece697e3ea4e7ed64c. Seems good for these calls to function without a mempool. Agree with hebasto it would be nice to clean up locking assertions within IsRBFOptIn. But if that requires more invasive changes to IsRBFOptIn, it would probably be better to do it in a targeted PR before or after this one.

MarcoFalke force-pushed on Sep 3, 2020

MarcoFalke commented at 8:08 AM on September 3, 2020: member

Changed the first commit to remove NO_THREAD_SAFETY_ANALYSIS. Let me know if this is better or if I should revert to the initial version.

in src/policy/rbf.h:27 in fa9124d935 outdated

  27 | + * @param tx   The unconfirmed transaction
  28 | + * @param pool The mempool, which may contain the tx
  29 | + *
  30 | + * @return     The rbf state. May be UNKNOWN for unconfirmed transactions not
  31 | + *             in the mempool. May be REPLACEABLE_BIP125 for transactions that
  32 | + *             conflict with an in-chain transaction.

hebasto commented at 6:38 PM on September 3, 2020:

This comment lacks FINAL value description: https://github.com/bitcoin/bitcoin/blob/fa9124d93527fd5ff741000b790a3772f2961a52/src/policy/rbf.cpp#L37

MarcoFalke commented at 6:47 AM on September 4, 2020:

It wasn't the goal to document RBFTransactionState, but since you asked for it, I've done that

hebasto changes_requested

hebasto commented at 6:39 PM on September 3, 2020: member

Approach ACK fa9124d93527fd5ff741000b790a3772f2961a52

MarcoFalke force-pushed on Sep 4, 2020

MarcoFalke commented at 6:48 AM on September 4, 2020: member

fixed up and force pushed last doc commit

hebasto approved

hebasto commented at 6:57 AM on September 4, 2020: member

ACK fadb436745515f16efb1330b36e6f3831c0ac5fb

in src/policy/rbf.cpp:44 in fadb436745 outdated

  35 | @@ -36,3 +36,10 @@ RBFTransactionState IsRBFOptIn(const CTransaction& tx, const CTxMemPool& pool)
  36 |      }
  37 |      return RBFTransactionState::FINAL;
  38 |  }
  39 | +
  40 | +RBFTransactionState IsRBFOptInEmptyMempool(const CTransaction& tx)
  41 | +{
  42 | +    static const CTxMemPool pool_empty;
  43 | +    LOCK(pool_empty.cs);
  44 | +    return IsRBFOptIn(tx, pool_empty);

jnewbery commented at 7:59 AM on September 4, 2020:

Seems like quite a roundabout way to avoid one line of code:

    // If we don't have a local mempool we can only check the transaction itself.
    return SignalsOptInRBF(tx) ? RBFTransactionState::REPLACEABLE_BIP125 : RBFTransactionState::UNKNOWN;

jnewbery commented at 8:01 AM on September 4, 2020: member

I'm not very keen on the static const mempool and have an alternative.

ACK fadb436745515f16efb1330b36e6f3831c0ac5fb

DrahtBot cross-referenced this on Sep 4, 2020 from issue Avoid locking CTxMemPool::cs recursively in some cases by hebasto

ryanofsky approved

ryanofsky commented at 7:09 PM on September 4, 2020: contributor

Code review ack fadb436745515f16efb1330b36e6f3831c0ac5fb. Since last review reverted changes to IsRBFOptIn with different IsRBFOptInEmptyMempool workaround. John's third workaround seems good too.

darosior approved

darosior commented at 7:14 PM on September 4, 2020: member

tested ACK fadb436745515f16efb1330b36e6f3831c0ac5fb

I also prefer jnewberry's patch, but not against merging this without it.

refactor: Add IsRBFOptInEmptyMempool

Co-authored-by: John Newbery <jonnynewbs@gmail.com>

fa831684e5

Remove mempool global from interfaces faef4fc9b4

doc: Add doxygen comment to IsRBFOptIn fa9ee52556

MarcoFalke force-pushed on Sep 5, 2020

MarcoFalke commented at 9:49 AM on September 5, 2020: member

Changed first commit according to feedback.

Untitled png

darosior approved

darosior commented at 9:58 AM on September 5, 2020: member

ACK fa9ee52

hebasto approved

hebasto commented at 10:07 AM on September 5, 2020: member

re-ACK fa9ee52556f493e4a896e2570ca1a3102d777d9a, since my previous review:

applied @jnewbery's patch
added @jnewbery as a co-author of the "refactor: Add IsRBFOptInEmptyMempool" commit
rebased

jnewbery commented at 10:13 AM on September 5, 2020: member

utACK fa9ee52556

MarcoFalke merged this on Sep 5, 2020

MarcoFalke closed this on Sep 5, 2020

MarcoFalke deleted the branch on Sep 5, 2020

deadalnix referenced this in commit d4e8a4c13f on Jul 15, 2021

bitcoin locked this on Feb 15, 2022