WIP: Transaction Pool Layer #13804

  22 | @@ -23,6 +23,6 @@ bool SignalsOptInRBF(const CTransaction &tx);
  23 |  // according to BIP 125
  24 |  // This involves checking sequence numbers of the transaction, as well
  25 |  // as the sequence numbers of all in-mempool ancestors.
  26 | -RBFTransactionState IsRBFOptIn(const CTransaction &tx, CTxMemPool &pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
  27 | +RBFTransactionState IsRBFOptIn(const CTransaction &tx,const CTxMemPool &pool) EXCLUSIVE_LOCKS_REQUIRED(pool.cs);

promag commented at 2:05 PM on July 31, 2018:

Commit "Stacked Transaction Pool"

nit, add space after ,.

 351 | +void CTxMemPool::UpdateForRemoveFromMempool(const setEntries& entriesToRemove, bool updateDescendants)
 352 | +{
 353 | +    return ::UpdateForRemoveFromMempool(*this, entriesToRemove, updateDescendants);
 354 | +}
 355 |  
 356 | +void CTxMemPool::Layer::  UpdateForRemoveFromMempool(const setEntries&entriesToRemove, bool updateDescendants)

promag commented at 2:06 PM on July 31, 2018:

Commit "Stacked Transaction Pool"

nit, remove extra spaces.

1013 | +boost::optional<CTxMemPool::Layer::txiter> CTxMemPool::Layer::GetIter(const uint256& txid) const
1014 |  {
1015 | -    CTxMemPool::setEntries ret;
1016 | +    const auto& it_cache = m_cache_added.find(txid);
1017 | +    if (it_cache != m_cache_added.end()) {
1018 | +        CTxMemPool::Layer::txiter{it_cache};

practicalswift commented at 4:10 PM on September 14, 2018:

Ownerless expression?

 574 | @@ -575,6 +575,9 @@ class CTxMemPool
 575 |      /** Translate a set of hashes into a set of pool iterators to avoid repeated lookups */
 576 |      setEntries GetIterSet(const std::set<uint256>& hashes) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 577 |  
 578 | +    /** Dereference the txiter */
 579 | +    static const CTxMemPoolEntry& GetEntry(txiter it) { return *it; };

practicalswift commented at 8:11 AM on September 23, 2018:

2018-09-22 21:49:48 cpplint(pr=13804): src/txmempool.h:579:  You don't need a ; after a }  [readability/braces] [4]

 724 | +         */
 725 | +        CCriticalSection& cs;
 726 | +
 727 | +        struct txiter_nested {
 728 | +            CTxMemPool::txiter _i;
 729 | +            explicit txiter_nested(CTxMemPool::txiter val) : _i(val){};

practicalswift commented at 8:11 AM on September 23, 2018:

2018-09-22 21:49:48 cpplint(pr=13804): src/txmempool.h:729:  You don't need a ; after a }  [readability/braces] [4]

 725 | +        CCriticalSection& cs;
 726 | +
 727 | +        struct txiter_nested {
 728 | +            CTxMemPool::txiter _i;
 729 | +            explicit txiter_nested(CTxMemPool::txiter val) : _i(val){};
 730 | +            txiter_nested() : _i(){};

practicalswift commented at 8:12 AM on September 23, 2018:

2018-09-22 21:49:48 cpplint(pr=13804): src/txmempool.h:730:  You don't need a ; after a }  [readability/braces] [4]

 754 | +        using txlinksMap = std::map<CTxMemPool::Layer::txiter, CTxMemPool::Layer::TxLinks, CompareIteratorByHash>;
 755 | +        /**
 756 | +         * Construct a Layer based on an existing tx pool.
 757 | +         * The caller must acquire the lock for the whole life-time of this layer.
 758 | +         */
 759 | +        Layer(const CTxMemPool& p) EXCLUSIVE_LOCKS_REQUIRED(m_tx_pool.cs)

practicalswift commented at 8:12 AM on September 23, 2018:

2018-09-22 21:49:48 cpplint(pr=13804): src/txmempool.h:759:  Single-parameter constructors should be marked explicit.  [runtime/explicit] [5]

 803 | +        void UpdateChildrenForRemoval(txiter it);
 804 | +        const setEntries& GetMemPoolChildren(txiter entry) const;
 805 | +        void removeUnchecked(txiter it, MemPoolRemovalReason reason);
 806 | +
 807 | +    private:
 808 | +

practicalswift commented at 8:13 AM on September 23, 2018:

2018-09-22 21:49:48 cpplint(pr=13804): src/txmempool.h:808:  Do not leave a blank line after "private:"  [whitespace/blank_line] [3]

 469 | -    vTxHashes.emplace_back(tx.GetWitnessHash(), newit);
 470 | +    vTxHashes.emplace_back(entry.GetTx().GetWitnessHash(), newit);
 471 |      newit->vTxHashesIdx = vTxHashes.size() - 1;
 472 |  }
 473 |  
 474 | +void CTxMemPool::Layer::addUnchecked(const CTxMemPoolEntry& entry, const CTxMemPool::Layer::setEntries& setAncestors, bool /* validFeeEstimate */ ignore)

practicalswift commented at 5:13 AM on September 26, 2018:

Use an unnamed parameter instead of ignore.

 475 | +{
 476 | +    ::addUnchecked(*this, entry, setAncestors);
 477 | +}
 478 | +
 479 | +template <typename P>
 480 | +static void removeUnchecked(P& pool, typename P::txiter it, MemPoolRemovalReason reason)

practicalswift commented at 5:13 AM on September 26, 2018:

Make MemPoolRemovalReason an unnamed parameter.

 903 | +    auto it = m_cache_added.find(hash);
 904 | +    if (it == m_cache_added.end())
 905 | +        return nullptr;
 906 | +    else
 907 | +        return it->GetSharedTx();
 908 | +    if (m_cache_removed.find(hash) != m_cache_removed.end()) return nullptr;

practicalswift commented at 5:14 AM on September 26, 2018:

This code will never get executed.

1135 | @@ -937,14 +1136,31 @@ int CTxMemPool::Expire(int64_t time) {
1136 |      RemoveStaged(stage, false, MemPoolRemovalReason::EXPIRY);
1137 |      return stage.size();
1138 |  }
1139 | +int CTxMemPool::Layer::Expire(int64_t time)

practicalswift commented at 5:15 AM on September 26, 2018:

Make time an unnamed parameter.

1315 | @@ -1058,6 +1316,13 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<COutPoint>* pvNoSpends
1316 |      }
1317 |  }
1318 |  
1319 | +void CTxMemPool::Layer::TrimToSize(size_t sizelimit, std::vector<COutPoint>* pvNoSpendsRemaining)

practicalswift commented at 5:16 AM on September 26, 2018:

Make sizelimit and pvNoSpendsRemaining unnamed parameters.

 994 | +{
 995 | +    auto it_add = m_map_next_tx_added.find(prevout);
 996 | +    if (it_add != m_map_next_tx_added.end()) return it_add->second;
 997 | +    if (m_map_next_tx_removed.find(prevout) != m_map_next_tx_removed.end()) return nullptr;
 998 | +
 999 | +    return m_tx_pool.GetConflictTx(prevout);

practicalswift commented at 12:25 PM on December 8, 2018:

Calling GetConflictTx requires holding the mutex m_tx_pool.cs

1017 | +    if (m_cache_removed.find(txid) != m_cache_removed.end()) {
1018 | +        return boost::optional<CTxMemPool::Layer::txiter>{};
1019 | +    }
1020 | +
1021 | +    // Nothing found in the cache, fall back to the inner layer:
1022 | +    const auto it_inner = m_tx_pool.GetIter(txid);

practicalswift commented at 12:26 PM on December 8, 2018:

m_tx_pool.cs must be held when calling GetIter

1210 | +{
1211 | +    const auto it = m_cache_map_links.find(entry);
1212 | +    if (it != m_cache_map_links.end()) return it->second.parents;
1213 | +
1214 | +    CTxMemPool::Layer::setEntries ret;
1215 | +    for (const auto& parent : m_tx_pool.GetMemPoolParents(boost::get<CTxMemPool::txiter>(entry))) {

practicalswift commented at 12:26 PM on December 8, 2018:

m_tx_pool.cs must be held here