Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mining: Prevent slowdown in CreateNewBlock on large mempools #9959

Merged
merged 3 commits into from
Mar 30, 2017
Merged

Mining: Prevent slowdown in CreateNewBlock on large mempools #9959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eed816a
Mining: return early when block is almost full
sdaftuar Mar 6, 2017
42cd8c8
Add benchmarking for CreateNewBlock
sdaftuar Mar 8, 2017
011124a
Update benchmarking with package statistics
sdaftuar Mar 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 36 additions & 4 deletions src/miner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,8 @@ void BlockAssembler::resetBlock()

std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
{
int64_t nTimeStart = GetTimeMicros();

resetBlock();

pblocktemplate.reset(new CBlockTemplate());
Expand Down Expand Up @@ -177,7 +179,11 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
// transaction (which in most cases can be a no-op).
fIncludeWitness = IsWitnessEnabled(pindexPrev, chainparams.GetConsensus());

addPackageTxs();
int nPackagesSelected = 0;
int nDescendantsUpdated = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might make more sense to put these on the class, rather than pass by-reference?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on a refactor of the BlockAssembler members separately (as part of a patch to exclude recent transactions from new blocks, when the fee difference is negligible), so I'd prefer to defer the decision of whether to include this in the class until I'm ready to PR that patch.

addPackageTxs(nPackagesSelected, nDescendantsUpdated);

int64_t nTime1 = GetTimeMicros();

nLastBlockTx = nBlockTx;
nLastBlockSize = nBlockSize;
Expand Down Expand Up @@ -209,6 +215,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false)) {
throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, FormatStateMessage(state)));
}
int64_t nTime2 = GetTimeMicros();

LogPrint("bench", "CreateNewBlock() packages: %.2fms (%d packages, %d updated descendants), validity: %.2fms (total %.2fms)\n", 0.001 * (nTime1 - nTimeStart), nPackagesSelected, nDescendantsUpdated, 0.001 * (nTime2 - nTime1), 0.001 * (nTime2 - nTimeStart));

return std::move(pblocktemplate);
}
Expand Down Expand Up @@ -282,16 +291,18 @@ void BlockAssembler::AddToBlock(CTxMemPool::txiter iter)
}
}

void BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded,
int BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded,
indexed_modified_transaction_set &mapModifiedTx)
{
int nDescendantsUpdated = 0;
BOOST_FOREACH(const CTxMemPool::txiter it, alreadyAdded) {
CTxMemPool::setEntries descendants;
mempool.CalculateDescendants(it, descendants);
// Insert all descendants (not yet in block) into the modified set
BOOST_FOREACH(CTxMemPool::txiter desc, descendants) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really interesting section. I got a bit nerd-sniped so apologies for long writeup.

Doing the following may be more efficient (I can code this up, but I don't want to step on your toes if you're operating here).

std::vector<txiter> diff(descendants.size());
auto end = std::set_difference(descendants.begin(), descendants.end(),
     alreadyAdded.begin(), alreadyAdded.end(), diff.begin()); // Linear time!
nDescendants += end - diff.begin();
for (auto it = diff.begin(); it != diff.end(); ++it) {
  // ....
}

It requires an extra copy compared to the naive, but they're iterators so who cares (we can also reuse the vector for the entire call at the top of updatepackages)...

Let N = alreadyAdded.size()
Let M = descendants.size()
This does O(M+N) work (I think that most implementations actually do O(max(M, N)), but the standard specified 2*(M+N-1)), while what's currently happening would appear to be O(M*log(N)).

There is also a pre-loop-check one could do if it's likely they don't intersect.

// O(1)
if (descendants.back() < alreadyAdded.front() || descendants.front() > alreadAdded.back())
    // skip set_difference, descendants - alreadyAdded = descendants

And a pre-loop narrowing one could do to make it O(min(M, N)).

// O(log(M) + log(N))
auto range1 = descendants.equal_range(alreadyAdded.begin(), alreadyAdded.end())
auto range2 = alreadyAdded.equal_range(descendants.begin(), descendants.end())
std::vector<txiter> diff(range1.second - range1.first);
auto end = std::set_difference(range1.first, range1.second,
     range2.first, range2.second, diff.begin()); // Linear time!
nDescendants += end - diff.begin();
for (auto it = diff.begin(); it != diff.end(); ++it) {
  // ....
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will try some of these out in future optimization efforts.

if (alreadyAdded.count(desc))
continue;
++nDescendantsUpdated;
modtxiter mit = mapModifiedTx.find(desc);
if (mit == mapModifiedTx.end()) {
CTxMemPoolModifiedEntry modEntry(desc);
Expand All @@ -304,6 +315,7 @@ void BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& alread
}
}
}
return nDescendantsUpdated;
}

// Skip entries in mapTx that are already in a block or are present
Expand Down Expand Up @@ -344,7 +356,7 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, CTxMemP
// 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()
void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated)
{
// mapModifiedTx will store sorted packages after they are modified
// because some of their txs are already in the block
Expand All @@ -358,6 +370,13 @@ void BlockAssembler::addPackageTxs()

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
// close to full; this is just a simple heuristic to finish quickly if the
// mempool has a lot of entries.
const int64_t MAX_CONSECUTIVE_FAILURES = 1000;
int64_t nConsecutiveFailed = 0;

while (mi != mempool.mapTx.get<ancestor_score>().end() || !mapModifiedTx.empty())
{
// First try to find a new transaction in mapTx to evaluate.
Expand Down Expand Up @@ -419,6 +438,14 @@ void BlockAssembler::addPackageTxs()
mapModifiedTx.get<ancestor_score>().erase(modit);
failedTx.insert(iter);
}

++nConsecutiveFailed;

if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight >
nBlockMaxWeight - 4000) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the additional condition(s) should be before incrementing, or we could end up just aborting immediately as we approach the max block weight even if we could easily fit a few more txs in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following; every time we add a new tx to the block we reset the counter... Can you clarify?

// Give up if we're close to full and haven't succeeded in a while
break;
}
continue;
}

Expand All @@ -439,6 +466,9 @@ void BlockAssembler::addPackageTxs()
continue;
}

// This transaction will make it in; reset the failed counter.
nConsecutiveFailed = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add more of a comment on why resetting is correct behavior here?

It seems to me at first glance, that if we fail to add something earlier, we should continue to tally those as failures?
Perhaps:

Assuming that below a certain threshold `T` of probability
(i.e.,`T = P(success at M | failure at M-1....0)` where `M = 1000`) 
of adding something to the block we want to give up,
 we expect that 
`1000>D>0. P(success at N+D+1 | failure at N+D,... success at N, failure at N-1, ...) 
> P(success at N+D | failure at N, failure at N-1, ...)`?

Maybe not the most accurate, but would be helpful for future reviewers trying to understand what the intention is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incremented whenever we fail (not just for failures after we're above a certain block weight), so it needs to be reset when adding a new tx.


// Package can be added. Sort the entries in a valid order.
std::vector<CTxMemPool::txiter> sortedEntries;
SortForBlock(ancestors, iter, sortedEntries);
Expand All @@ -449,8 +479,10 @@ void BlockAssembler::addPackageTxs()
mapModifiedTx.erase(sortedEntries[i]);
}

++nPackagesSelected;

// Update transactions that depend on each of these
UpdatePackagesForAdded(ancestors, mapModifiedTx);
nDescendantsUpdated += UpdatePackagesForAdded(ancestors, mapModifiedTx);
}
}

Expand Down
11 changes: 7 additions & 4 deletions src/miner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,10 @@ class BlockAssembler
void AddToBlock(CTxMemPool::txiter iter);

// Methods for how to add transactions to a block.
/** Add transactions based on feerate including unconfirmed ancestors */
void addPackageTxs();
/** 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);

// helper functions for addPackageTxs()
/** Remove confirmed (inBlock) entries from given set */
Expand All @@ -199,8 +201,9 @@ class BlockAssembler
/** Sort the package in an order that is valid to appear in a block */
void SortForBlock(const CTxMemPool::setEntries& package, CTxMemPool::txiter entry, std::vector<CTxMemPool::txiter>& sortedEntries);
/** Add descendants of given transactions to mapModifiedTx with ancestor
* state updated assuming given transactions are inBlock. */
void UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set &mapModifiedTx);
* state updated assuming given transactions are inBlock. Returns number
* of updated descendants. */
int UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set &mapModifiedTx);
};

/** Modify the extranonce in a block */
Expand Down