highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-11-12 06:58:57 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#25223: [kernel 2e/n] miner: Make mempool optional, stop constructing temporary empty mempools

Browse Source 
0f1a259657 miner: Make mempool optional for BlockAssembler (Carl Dong)
cc5739b27d miner: Make UpdatePackagesForAdded static (Carl Dong)
f024578b3a miner: Absorb SkipMapTxEntry into addPackageTxs (Carl Dong)

Pull request description:

  This is part of the libbitcoinkernel project: #24303, https://github.com/bitcoin/bitcoin/projects/18

  This is **_NOT_** dependent on, but is a "companion-PR" to #25215.

  ### Abstract

  This PR removes the need to construct `BlockAssembler` with temporary, empty mempools in cases where we don't want to source transactions from the mempool (e.g. in `TestChain100Setup::CreateBlock` and `generateblock`). After this PR, `BlockAssembler` will accept a `CTxMemPool` pointer and handle the `nullptr` case instead of requiring a `CTxMemPool` reference.

  An overview of the changes is best seen in the changes in the header file:

  ```diff
  diff --git a/src/node/miner.h b/src/node/miner.h
  index 7cf8e3fb9e..7e9f503602 100644
  --- a/src/node/miner.h
  +++ b/src/node/miner.h
  @@ -147,7 +147,7 @@ private:
       int64_t m_lock_time_cutoff;

       const CChainParams& chainparams;
  -    const CTxMemPool& m_mempool;
  +    const CTxMemPool* m_mempool;
       CChainState& m_chainstate;

   public:
  @@ -157,8 +157,8 @@ public:
           CFeeRate blockMinFeeRate;
       };

  -    explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool);
  -    explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options);
  +    explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool);
  +    explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool, const Options& options);

       /** Construct a new block template with coinbase to scriptPubKeyIn */
       std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
  @@ -177,7 +177,7 @@ private:
       /** Add transactions based on feerate including unconfirmed ancestors
         * Increments nPackagesSelected / nDescendantsUpdated with corresponding
         * statistics from the package selection (for logging statistics). */
  -    void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
  +    void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs);

       // helper functions for addPackageTxs()
       /** Remove confirmed (inBlock) entries from given set */
  @@ -189,15 +189,8 @@ private:
         * These checks should always succeed, and they're here
         * only as an extra check in case of suboptimal node configuration */
       bool TestPackageTransactions(const CTxMemPool::setEntries& package) const;
  -    /** Return true if given transaction from mapTx has already been evaluated,
  -      * or if the transaction's cached data in mapTx is incorrect. */
  -    bool SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
       /** Sort the package in an order that is valid to appear in a block */
       void SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries);
  -    /** Add descendants of given transactions to mapModifiedTx with ancestor
  -      * state updated assuming given transactions are inBlock. Returns number
  -      * of updated descendants. */
  -    int UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set& mapModifiedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
   };

   int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
  ```

  ### Alternatives

  Aside from approach in this current PR, we can also take the approach of moving the `CTxMemPool*` argument from the `BlockAssembler` constructor to `BlockAssembler::CreateNewBlock`, since that's where it's needed anyway. I did not push this approach because it requires quite a lot of call sites to be changed. However, I do have it coded up and can do that if people express a strong preference. This would look something like:

  ```
  BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options);
  BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const CTxMemPool* maybe_mempool);
  ```

  ### Future work

  Although wholly out of scope for this PR, we could potentially refine the `BlockAssembler` interface further, so that we have:

  ```
  BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options);
  BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, std::vector<CTransaction>& txs);
  BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const CTxMemPool& mempool);
  ```

  Whereby `TestChain100Setup::CreateBlock` and `generateblock` would call the `BlockAssembler::CreateNewBlock` that takes in `CTransaction`s and we can potentially remove `RegenerateCommitments` altogether. All other callers can use the `CTxMemPool` version.

ACKs for top commit:
  glozow:
    ACK 0f1a259657
  laanwj:
    Code review ACK 0f1a259657
  MarcoFalke:
    ACK 0f1a259657 🐊

Tree-SHA512: 2b4b1dbb43d85719f241ad1f19ceb7fc50cf764721da425a3d1ff71bd16328c4f86acff22e565bc9abee770d3ac8827a6676b66daa93dbf42dd817ad929e9448
This commit is contained in:
fanquake
2022-06-15 16:27:51 +01:00
parent fa07ee165e 0f1a259657
commit a7a36590f5
9 changed files with 56 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files

79
src/node/miner.cpp
View File

@@ -62,7 +62,7 @@ BlockAssembler::Options::Options()
nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT;
}
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options)
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool, const Options& options)
: chainparams{chainstate.m_chainman.GetParams()},
m_mempool(mempool),
m_chainstate(chainstate)
@@ -87,7 +87,7 @@ static BlockAssembler::Options DefaultOptions()
return options;
}
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool)
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool)
: BlockAssembler(chainstate, mempool, DefaultOptions()) {}
void BlockAssembler::resetBlock()
@@ -121,7 +121,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
pblocktemplate->vTxFees.push_back(-1); // updated at end
pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end
LOCK2(cs_main, m_mempool.cs);
LOCK(::cs_main);
CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip();
assert(pindexPrev != nullptr);
nHeight = pindexPrev->nHeight + 1;
@@ -138,7 +138,10 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
int nPackagesSelected = 0;
int nDescendantsUpdated = 0;
addPackageTxs(nPackagesSelected, nDescendantsUpdated);
if (m_mempool) {
LOCK(m_mempool->cs);
addPackageTxs(*m_mempool, nPackagesSelected, nDescendantsUpdated);
}
int64_t nTime1 = GetTimeMicros();
@@ -232,15 +235,19 @@ void BlockAssembler::AddToBlock(CTxMemPool::txiter iter)
}
}
int BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded,
indexed_modified_transaction_set &mapModifiedTx)
/** Add descendants of given transactions to mapModifiedTx with ancestor
* state updated assuming given transactions are inBlock. Returns number
* of updated descendants. */
static int UpdatePackagesForAdded(const CTxMemPool& mempool,
const CTxMemPool::setEntries& alreadyAdded,
indexed_modified_transaction_set& mapModifiedTx) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs)
{
AssertLockHeld(m_mempool.cs);
AssertLockHeld(mempool.cs);
int nDescendantsUpdated = 0;
for (CTxMemPool::txiter it : alreadyAdded) {
CTxMemPool::setEntries descendants;
m_mempool.CalculateDescendants(it, descendants);
mempool.CalculateDescendants(it, descendants);
// Insert all descendants (not yet in block) into the modified set
for (CTxMemPool::txiter desc : descendants) {
if (alreadyAdded.count(desc)) {
@@ -262,23 +269,6 @@ int BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& already
return nDescendantsUpdated;
}
// Skip entries in mapTx that are already in a block or are present
// in mapModifiedTx (which implies that the mapTx ancestor state is
// stale due to ancestor inclusion in the block)
// Also skip transactions that we've already failed to add. This can happen if
// we consider a transaction in mapModifiedTx and it fails: we can then
// potentially consider it again while walking mapTx. It's currently
// guaranteed to fail again, but as a belt-and-suspenders check we put it in
// failedTx and avoid re-evaluation, since the re-evaluation would be using
// cached size/sigops/fee values that are not actually correct.
bool BlockAssembler::SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx)
{
AssertLockHeld(m_mempool.cs);
assert(it != m_mempool.mapTx.end());
return mapModifiedTx.count(it) || inBlock.count(it) || failedTx.count(it);
}
void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries)
{
// Sort package by ancestor count
@@ -300,9 +290,9 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve
// Each time through the loop, we compare the best transaction in
// mapModifiedTxs with the next transaction in the mempool to decide what
// transaction package to work on next.
void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated)
void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated)
{
AssertLockHeld(m_mempool.cs);
AssertLockHeld(mempool.cs);
// mapModifiedTx will store sorted packages after they are modified
// because some of their txs are already in the block
@@ -310,7 +300,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
// Keep track of entries that failed inclusion, to avoid duplicate work
CTxMemPool::setEntries failedTx;
CTxMemPool::indexed_transaction_set::index<ancestor_score>::type::iterator mi = m_mempool.mapTx.get<ancestor_score>().begin();
CTxMemPool::indexed_transaction_set::index<ancestor_score>::type::iterator mi = mempool.mapTx.get<ancestor_score>().begin();
CTxMemPool::txiter iter;
// Limit the number of attempts to add transactions to the block when it is
@@ -319,12 +309,27 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
const int64_t MAX_CONSECUTIVE_FAILURES = 1000;
int64_t nConsecutiveFailed = 0;
while (mi != m_mempool.mapTx.get<ancestor_score>().end() || !mapModifiedTx.empty()) {
while (mi != mempool.mapTx.get<ancestor_score>().end() || !mapModifiedTx.empty()) {
// First try to find a new transaction in mapTx to evaluate.
if (mi != m_mempool.mapTx.get<ancestor_score>().end() &&
SkipMapTxEntry(m_mempool.mapTx.project<0>(mi), mapModifiedTx, failedTx)) {
++mi;
continue;
//
// Skip entries in mapTx that are already in a block or are present
// in mapModifiedTx (which implies that the mapTx ancestor state is
// stale due to ancestor inclusion in the block)
// Also skip transactions that we've already failed to add. This can happen if
// we consider a transaction in mapModifiedTx and it fails: we can then
// potentially consider it again while walking mapTx. It's currently
// guaranteed to fail again, but as a belt-and-suspenders check we put it in
// failedTx and avoid re-evaluation, since the re-evaluation would be using
// cached size/sigops/fee values that are not actually correct.
/** Return true if given transaction from mapTx has already been evaluated,
* or if the transaction's cached data in mapTx is incorrect. */
if (mi != mempool.mapTx.get<ancestor_score>().end()) {
auto it = mempool.mapTx.project<0>(mi);
assert(it != mempool.mapTx.end());
if (mapModifiedTx.count(it) || inBlock.count(it) || failedTx.count(it)) {
++mi;
continue;
}
}
// Now that mi is not stale, determine which transaction to evaluate:
@@ -332,13 +337,13 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
bool fUsingModified = false;
modtxscoreiter modit = mapModifiedTx.get<ancestor_score>().begin();
if (mi == m_mempool.mapTx.get<ancestor_score>().end()) {
if (mi == mempool.mapTx.get<ancestor_score>().end()) {
// We're out of entries in mapTx; use the entry from mapModifiedTx
iter = modit->iter;
fUsingModified = true;
} else {
// Try to compare the mapTx entry to the mapModifiedTx entry
iter = m_mempool.mapTx.project<0>(mi);
iter = mempool.mapTx.project<0>(mi);
if (modit != mapModifiedTx.get<ancestor_score>().end() &&
CompareTxMemPoolEntryByAncestorFee()(*modit, CTxMemPoolModifiedEntry(iter))) {
// The best entry in mapModifiedTx has higher score
@@ -393,7 +398,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
CTxMemPool::setEntries ancestors;
uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();
std::string dummy;
m_mempool.CalculateMemPoolAncestors(*iter, ancestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false);
mempool.CalculateMemPoolAncestors(*iter, ancestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false);
onlyUnconfirmed(ancestors);
ancestors.insert(iter);
@@ -423,7 +428,7 @@ void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpda
++nPackagesSelected;
// Update transactions that depend on each of these
nDescendantsUpdated += UpdatePackagesForAdded(ancestors, mapModifiedTx);
nDescendantsUpdated += UpdatePackagesForAdded(mempool, ancestors, mapModifiedTx);
}
}
} // namespace node

15
src/node/miner.h
View File

@@ -147,7 +147,7 @@ private:
int64_t m_lock_time_cutoff;
const CChainParams& chainparams;
const CTxMemPool& m_mempool;
const CTxMemPool* const m_mempool;
CChainState& m_chainstate;
public:
@@ -157,8 +157,8 @@ public:
CFeeRate blockMinFeeRate;
};
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool);
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options);
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool);
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool, const Options& options);
/** Construct a new block template with coinbase to scriptPubKeyIn */
std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
@@ -177,7 +177,7 @@ private:
/** Add transactions based on feerate including unconfirmed ancestors
* Increments nPackagesSelected / nDescendantsUpdated with corresponding
* statistics from the package selection (for logging statistics). */
void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs);
// helper functions for addPackageTxs()
/** Remove confirmed (inBlock) entries from given set */
@@ -189,15 +189,8 @@ private:
* These checks should always succeed, and they're here
* only as an extra check in case of suboptimal node configuration */
bool TestPackageTransactions(const CTxMemPool::setEntries& package) const;
/** Return true if given transaction from mapTx has already been evaluated,
* or if the transaction's cached data in mapTx is incorrect. */
bool SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
/** Sort the package in an order that is valid to appear in a block */
void SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries);
/** Add descendants of given transactions to mapModifiedTx with ancestor
* state updated assuming given transactions are inBlock. Returns number
* of updated descendants. */
int UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set& mapModifiedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
};
int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);