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.

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

Mining: Select transactions using feerate-with-ancestors #7600

Merged
merged 2 commits into from Jun 16, 2016

Conversation

Projects
None yet
10 participants
@sdaftuar
Member

sdaftuar commented Feb 25, 2016

This implements "Child-pays-for-parent" transaction selection in CreateNewBlock, where we sort the mempool using (modified) feerate with ancestors and consider transactions in that order during block creation. As transactions are selected, a separate map is used to track the new feerate-with-ancestors value to use for in-mempool descendants that haven't yet been selected.

This is almost strictly better (ie, results in higher-fee blocks) than the existing mining algorithm, with some small variance due to edge cases around which transactions might get selected as the new block's size approaches the maximum allowed. Moreover the performance impact on CreateNewBlock is minimal -- I observed that the total run time of the function actually slightly improved, apparently due to TestBlockValidity being on average slightly faster (a somewhat surprising result).

The actual fee improvements observed were fairly low in the time period I looked at (October 2015), roughly a 1% fee improvement overall. I think that is driven first and foremost by blocks not really being full for a large bulk of the time period, so on a large number of data points the results were identical. Additionally, my guess is that user behavior may not have adapted to this type of transaction selection being widely deployed, and so in the future perhaps we might expect to see larger differences. I did note a handful of instances where the implementation here notably outperformed the existing algorithm, in some instances more than 20% better, but those were the exception and not the rule.

For comparison, I found no examples where the old algorithm produced a block with 0.5% or more in fees than the new algorithm (and few examples where the old algorithm's block had more fees at all, around 1% of samples).

I've labeled this PR as a work-in-progress for now, as it builds off two other open PR's (#7594 and #7598). Once those are reviewed and merged, I'll rebase this and remove the WIP label. I opened this now to help motivate the work in those two PRs.

One open question I have is whether it's worth keeping around the old transaction selection algorithm. The refactor in #7598 that this builds on makes it easy to leave it around, and I don't think it would be hard to add command line arguments for policy configuration that would enable using the older algorithm. But I don't know if that's of interest to anyone, so for now I figure we can let people mull it over and decide what to do in a future PR.

A related question to that is whether to get rid of the existing mempool "score" index. The ancestor tracking pull adds a new ancestor feerate index without removing the old feerate index, but if we decide to get rid of the mining algorithm that uses that index, then perhaps we could also eliminate this mempool index as well.

To do:

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Mar 3, 2016

Contributor

Does this algorithm claim to select the optimum descendant subtree for a given independent transaction?

Contributor

dgenr8 commented Mar 3, 2016

Does this algorithm claim to select the optimum descendant subtree for a given independent transaction?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 3, 2016

Member

This algorithm is by no means guaranteed to produce maximum fee blocks for a given mempool, if that's what you're trying to ask.

The algorithm this PR is trying to implement is straightforward:

Look at highest ancestor-feerate tx in mempool:
 - Add tx's package (tx + unconfirmed ancestors) to block
 - Remove tx's package from the mempool and re-sort (so that descendants of tx and its now in-block ancestors are treated as confirmed, and remaining in-mempool descendant's ancestor feerate is updated).

The implementation is complicated because we can't actually remove things from the mempool. To deal with that, I added a new multi_index that CreateNewBlock uses to keep track of the effective ancestor feerate/size/sigops for transactions whose in-mempool ancestors have been selected for inclusion in the block. (The only other complication is that a package might not be includable, say because it's too big or has too many sigops, but that's easy to handle.)

Anyway, getting back to the algorithm -- I think this algorithm is superior to the existing one (which iterates based on each tx's individual feerate, not looking at ancestors) for optimizing block fees, and it has the property that (what I expect to be) typical uses of CPFP should generally be captured -- namely those situations where a single child is paying a large fee for its parents.

However if there are multiple medium fee children that are chained below some low-fee parent, then the algorithm might easily fill the block without those transactions because they may not be considered until the block is full. For now though I suspect this algorithm is adequate for the current usage patterns, and perhaps we can come up with ways to monitor what's in the mempool to look for patterns of usage that would motivate further improvements to CreateNewBlock.

Member

sdaftuar commented Mar 3, 2016

This algorithm is by no means guaranteed to produce maximum fee blocks for a given mempool, if that's what you're trying to ask.

The algorithm this PR is trying to implement is straightforward:

Look at highest ancestor-feerate tx in mempool:
 - Add tx's package (tx + unconfirmed ancestors) to block
 - Remove tx's package from the mempool and re-sort (so that descendants of tx and its now in-block ancestors are treated as confirmed, and remaining in-mempool descendant's ancestor feerate is updated).

The implementation is complicated because we can't actually remove things from the mempool. To deal with that, I added a new multi_index that CreateNewBlock uses to keep track of the effective ancestor feerate/size/sigops for transactions whose in-mempool ancestors have been selected for inclusion in the block. (The only other complication is that a package might not be includable, say because it's too big or has too many sigops, but that's easy to handle.)

Anyway, getting back to the algorithm -- I think this algorithm is superior to the existing one (which iterates based on each tx's individual feerate, not looking at ancestors) for optimizing block fees, and it has the property that (what I expect to be) typical uses of CPFP should generally be captured -- namely those situations where a single child is paying a large fee for its parents.

However if there are multiple medium fee children that are chained below some low-fee parent, then the algorithm might easily fill the block without those transactions because they may not be considered until the block is full. For now though I suspect this algorithm is adequate for the current usage patterns, and perhaps we can come up with ways to monitor what's in the mempool to look for patterns of usage that would motivate further improvements to CreateNewBlock.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 4, 2016

Member

Would it be possible to have a CTxMempool::check that accurately checks the ancestor-based statistics, rather than just lower bounding them to 1 level up?

Member

sipa commented Mar 4, 2016

Would it be possible to have a CTxMempool::check that accurately checks the ancestor-based statistics, rather than just lower bounding them to 1 level up?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 17, 2016

Member

Updated now that #7594 has been merged.

Member

sdaftuar commented Mar 17, 2016

Updated now that #7594 has been merged.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Mar 17, 2016

Member

Is this still WIP?

Member

btcdrak commented Mar 17, 2016

Is this still WIP?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 17, 2016

Member

@btcdrak: I was thinking I'd leave this marked as WIP until the other dependent PR (#7598) is merged, since I'll have to rebase this on top of whatever the final version of the refactor ends up being.

I should add -- if you want to start reviewing now, please do!

Member

sdaftuar commented Mar 17, 2016

@btcdrak: I was thinking I'd leave this marked as WIP until the other dependent PR (#7598) is merged, since I'll have to rebase this on top of whatever the final version of the refactor ends up being.

I should add -- if you want to start reviewing now, please do!

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Mar 17, 2016

Member

@sdaftuar Github tasklists are maybe better for this kind of thing, and they show up as tasks even on ticket lists. https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments

Member

btcdrak commented Mar 17, 2016

@sdaftuar Github tasklists are maybe better for this kind of thing, and they show up as tasks even on ticket lists. https://github.com/blog/1375-task-lists-in-gfm-issues-pulls-comments

@sdaftuar sdaftuar changed the title from [WIP] Mining: Select transactions using feerate-with-ancestors to Mining: Select transactions using feerate-with-ancestors May 18, 2016

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar May 18, 2016

Member

Rebased on the latest #7598

Member

sdaftuar commented May 18, 2016

Rebased on the latest #7598

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 9, 2016

Member

Rebased again and built off the latest version of #7598.

Member

sdaftuar commented Jun 9, 2016

Rebased again and built off the latest version of #7598.

@seweso

This comment has been minimized.

Show comment
Hide comment
@seweso

seweso Jun 10, 2016

Very nice!

Additionally, my guess is that user behavior may not have adapted to this type of transaction selection being widely deployed, and so in the future perhaps we might expect to see larger differences.

Yes you would definitely expect bigger difference as this becomes available. As entities will be able to underpay transactions without risking getting dropped from the mempool. (for entities who continuously create transactions)

One of the dangers of the blocksize limit is that fees would go up, but have almost no reason/force to get it down. Things like this and RBF could really help with that. 👍

Is this algorithm also used to determine which transactions to drop?

seweso commented Jun 10, 2016

Very nice!

Additionally, my guess is that user behavior may not have adapted to this type of transaction selection being widely deployed, and so in the future perhaps we might expect to see larger differences.

Yes you would definitely expect bigger difference as this becomes available. As entities will be able to underpay transactions without risking getting dropped from the mempool. (for entities who continuously create transactions)

One of the dangers of the blocksize limit is that fees would go up, but have almost no reason/force to get it down. Things like this and RBF could really help with that. 👍

Is this algorithm also used to determine which transactions to drop?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 11, 2016

Member

Is this algorithm also used to determine which transactions to drop?

@seweso The algorithm for eviction of transactions from the mempool is unchanged. In a sense, the transaction eviction algorithm for the mempool is already assuming we're doing something like what this PR implements for transaction selection, in that a transaction is less likely to be evicted from the mempool if its feerate with descendants is high. But more fundamentally, mempool eviction is just a different problem than transaction selection: when selecting transactions for a block, a candidate transaction's ancestors must be included (so it's natural to sort by feerate-with-ancestors), but when considering a transaction for mempool eviction, a candidate transaction's descendants must also be removed as well (so we use max(feerate, feerate with descendants) as our sort order for removal).

Member

sdaftuar commented Jun 11, 2016

Is this algorithm also used to determine which transactions to drop?

@seweso The algorithm for eviction of transactions from the mempool is unchanged. In a sense, the transaction eviction algorithm for the mempool is already assuming we're doing something like what this PR implements for transaction selection, in that a transaction is less likely to be evicted from the mempool if its feerate with descendants is high. But more fundamentally, mempool eviction is just a different problem than transaction selection: when selecting transactions for a block, a candidate transaction's ancestors must be included (so it's natural to sort by feerate-with-ancestors), but when considering a transaction for mempool eviction, a candidate transaction's descendants must also be removed as well (so we use max(feerate, feerate with descendants) as our sort order for removal).

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 13, 2016

Member

Needs rebase

Member

MarcoFalke commented Jun 13, 2016

Needs rebase

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 13, 2016

Member

Rebased after #7598 has been merged. This PR is ready for review!

Member

sdaftuar commented Jun 13, 2016

Rebased after #7598 has been merged. This PR is ready for review!

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jun 14, 2016

Member

Concept ACK

Member

MarcoFalke commented Jun 14, 2016

Concept ACK

iit++;
}
}
}

This comment has been minimized.

@JeremyRubin

JeremyRubin Jun 14, 2016

Contributor

Nit: may as well have ++iit in the for loop because it always occurs.

for (CTxMemPool::setEntries::iterator iit = testSet.begin(); iit != testSet.end(); ++iit) {
         // Only test txs not already in the block
         if (inBlock.count(*iit)) 
             testSet.erase(iit);
 }

Edit: ignore, erasing an iterator invalidates iit.

@JeremyRubin

JeremyRubin Jun 14, 2016

Contributor

Nit: may as well have ++iit in the for loop because it always occurs.

for (CTxMemPool::setEntries::iterator iit = testSet.begin(); iit != testSet.end(); ++iit) {
         // Only test txs not already in the block
         if (inBlock.count(*iit)) 
             testSet.erase(iit);
 }

Edit: ignore, erasing an iterator invalidates iit.

if (nBlockSigOps + packageSigOps >= MAX_BLOCK_SIGOPS)
return false;
return true;
}

This comment has been minimized.

@JeremyRubin

JeremyRubin Jun 14, 2016

Contributor

nit: Can just have
return ! (nBlockSize + packageSize >= nBlockMaxSize || nBlockSigOps + packageSigOps >= MAX_BLOCK_SIGOPS)

or

return (nBlockSize + packageSize < nBlockMaxSize && nBlockSigOps + packageSigOps < MAX_BLOCK_SIGOPS)

@JeremyRubin

JeremyRubin Jun 14, 2016

Contributor

nit: Can just have
return ! (nBlockSize + packageSize >= nBlockMaxSize || nBlockSigOps + packageSigOps >= MAX_BLOCK_SIGOPS)

or

return (nBlockSize + packageSize < nBlockMaxSize && nBlockSigOps + packageSigOps < MAX_BLOCK_SIGOPS)

This comment has been minimized.

@JeremyRubin

JeremyRubin Jun 14, 2016

Contributor

nit: >= should really be just > because we allow up to the max sizes.

@JeremyRubin

JeremyRubin Jun 14, 2016

Contributor

nit: >= should really be just > because we allow up to the max sizes.

@JeremyRubin

This comment has been minimized.

Show comment
Hide comment
@JeremyRubin

JeremyRubin Jun 14, 2016

Contributor

utAck

Contributor

JeremyRubin commented Jun 14, 2016

utAck

Show outdated Hide outdated src/miner.cpp
Show outdated Hide outdated src/miner.cpp
Show outdated Hide outdated src/miner.cpp
Show outdated Hide outdated src/miner.cpp
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 15, 2016

Member

Pushed commits addressing @mrbandrews comments.

re: @JeremyRubin's nits, I'm leaving TestPackage alone, I think it's pretty readable in its current form, and the >= shouldn't matter for anything and is consistent with addScoreTx's behavior.

I'll do another round of testing locally, similar to what I did before when I opened the pull, with the current version of the code and report back.

Member

sdaftuar commented Jun 15, 2016

Pushed commits addressing @mrbandrews comments.

re: @JeremyRubin's nits, I'm leaving TestPackage alone, I think it's pretty readable in its current form, and the >= shouldn't matter for anything and is consistent with addScoreTx's behavior.

I'll do another round of testing locally, similar to what I did before when I opened the pull, with the current version of the code and report back.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 15, 2016

Member

I re-tested my code by simulating on data from 2016-02-23 -to 2016-02-29, comparing the default mining code in master against the mining code introduced here, by calling CreateNewBlock every 100 transactions.

I looked at the fees produced in the last call to CNB before each block found by the network, and the code in this PR produced higher fee blocks in each instance (894 data points).

I was also benchmarking the runtime of CNB over all invocations, and this code was negligibly faster than the old code (summing over all invocations).

Member

sdaftuar commented Jun 15, 2016

I re-tested my code by simulating on data from 2016-02-23 -to 2016-02-29, comparing the default mining code in master against the mining code introduced here, by calling CreateNewBlock every 100 transactions.

I looked at the fees produced in the last call to CNB before each block found by the network, and the code in this PR produced higher fee blocks in each instance (894 data points).

I was also benchmarking the runtime of CNB over all invocations, and this code was negligibly faster than the old code (summing over all invocations).

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 15, 2016

Member

ACK.

Beyond review, I tested this on several mining nodes for about a week (in addition to prior testing I did months back); I also took nodes running this and a tight getblocktemplate loop through a number of reorg tests with debugging turned up. Finally, rather than using sdaftuar record and replay framework, I scraped all the recent transactions out of recent blocks, reorged the chain back and put all the still-valid transactions into the mempool and confirmed it gave a valid gbt result with higher total fees than the before the patch, I tried this at a dozen different points.

Member

gmaxwell commented Jun 15, 2016

ACK.

Beyond review, I tested this on several mining nodes for about a week (in addition to prior testing I did months back); I also took nodes running this and a tight getblocktemplate loop through a number of reorg tests with debugging turned up. Finally, rather than using sdaftuar record and replay framework, I scraped all the recent transactions out of recent blocks, reorged the chain back and put all the still-valid transactions into the mempool and confirmed it gave a valid gbt result with higher total fees than the before the patch, I tried this at a dozen different points.

@mrbandrews

This comment has been minimized.

Show comment
Hide comment
@mrbandrews

mrbandrews Jun 16, 2016

Contributor

ACK.
Code review ACK and in my testing, though not nearly as comprehensive as sdaftuar and gmaxwell's testing, the new code yields higher fees by 1% to 15%.

Contributor

mrbandrews commented Jun 16, 2016

ACK.
Code review ACK and in my testing, though not nearly as comprehensive as sdaftuar and gmaxwell's testing, the new code yields higher fees by 1% to 15%.

Show outdated Hide outdated src/miner.cpp
fUsingModified = true;
} else {
// Try to compare the mapTx entry to the mapModifiedTx entry
iter = mempool.mapTx.project<0>(mi);

This comment has been minimized.

@sipa

sipa Jun 16, 2016

Member

Nit: this can move into the else branch below.

Edit: nevermind, you're using it in the comparator.

@sipa

sipa Jun 16, 2016

Member

Nit: this can move into the else branch below.

Edit: nevermind, you're using it in the comparator.

Show outdated Hide outdated src/miner.h
@@ -134,7 +135,7 @@ CBlockTemplate* BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
: pblock->GetBlockTime();
addPriorityTxs();
addScoreTxs();
addPackageTxs();

This comment has been minimized.

@sipa

sipa Jun 16, 2016

Member

The addScoreTxs method can be deleted, I think?

@sipa

sipa Jun 16, 2016

Member

The addScoreTxs method can be deleted, I think?

This comment has been minimized.

@sdaftuar

sdaftuar Jun 16, 2016

Member

I raised that question at the beginning of this pull, and no discussion has happened; I'd like to defer that to its own PR to give anyone who is clinging to the old behavior a chance to argue for keeping the code?

@sdaftuar

sdaftuar Jun 16, 2016

Member

I raised that question at the beginning of this pull, and no discussion has happened; I'd like to defer that to its own PR to give anyone who is clinging to the old behavior a chance to argue for keeping the code?

This comment has been minimized.

@sipa

sipa Jun 16, 2016

Member

Fair enough.

@sipa

sipa Jun 16, 2016

Member

Fair enough.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 16, 2016

Member

ACK f3c6551 (with tree 66fb31f4aaaf3c49ef03c93b3b8155b531359e05).

Update: ACK b428fb2656b3cdf3021faf508eb78e6831e5a276 (with tree 2e6a8674a4e7179aea0041931d68076a442d0132).

Can you squash the fixups?

Member

sipa commented Jun 16, 2016

ACK f3c6551 (with tree 66fb31f4aaaf3c49ef03c93b3b8155b531359e05).

Update: ACK b428fb2656b3cdf3021faf508eb78e6831e5a276 (with tree 2e6a8674a4e7179aea0041931d68076a442d0132).

Can you squash the fixups?

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jun 16, 2016

Member

Thanks, squashed.

Member

sdaftuar commented Jun 16, 2016

Thanks, squashed.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 16, 2016

Member

ReACK 29fac19 (tree 2e6a8674a4e7179aea0041931d68076a442d0132).

Member

sipa commented Jun 16, 2016

ReACK 29fac19 (tree 2e6a8674a4e7179aea0041931d68076a442d0132).

@sipa sipa merged commit 29fac19 into bitcoin:master Jun 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Jun 16, 2016

Merge #7600: Mining: Select transactions using feerate-with-ancestors
29fac19 Add unit tests for ancestor feerate mining (Suhas Daftuar)
c82a4e9 Use ancestor-feerate based transaction selection for mining (Suhas Daftuar)

@sipa sipa referenced this pull request Jun 16, 2016

Closed

Segregated witness #7910

5 of 7 tasks complete

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7600: Mining: Select transactions using feerate-with-ancestors
29fac19 Add unit tests for ancestor feerate mining (Suhas Daftuar)
c82a4e9 Use ancestor-feerate based transaction selection for mining (Suhas Daftuar)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7600: Mining: Select transactions using feerate-with-ancestors
29fac19 Add unit tests for ancestor feerate mining (Suhas Daftuar)
c82a4e9 Use ancestor-feerate based transaction selection for mining (Suhas Daftuar)

codablock added a commit to codablock/dash that referenced this pull request Dec 27, 2017

Merge #7600: Mining: Select transactions using feerate-with-ancestors
29fac19 Add unit tests for ancestor feerate mining (Suhas Daftuar)
c82a4e9 Use ancestor-feerate based transaction selection for mining (Suhas Daftuar)

codablock added a commit to codablock/dash that referenced this pull request Dec 28, 2017

Merge #7600: Mining: Select transactions using feerate-with-ancestors
29fac19 Add unit tests for ancestor feerate mining (Suhas Daftuar)
c82a4e9 Use ancestor-feerate based transaction selection for mining (Suhas Daftuar)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment