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

Mempool: Add tracking of ancestor packages #7594

Merged
merged 6 commits into from Mar 17, 2016

Conversation

Projects
None yet
5 participants
@sdaftuar
Member

sdaftuar commented Feb 24, 2016

This implements caching of count, size, (modified) fee, and sigops of each mempool entry with its unconfirmed ancestors.

This is in preparation for adding support to CreateNewBlock for transaction selection based on fees-with-ancestors, for which I'll open another pull request soon.

Note that in order to keep accurate ancestor state, CTxMemPool::removeForBlock has to walk the descendants of each in-block transaction (so that it's ancestor state can be reflected for the ancestor which was confirmed). The performance hit appears to be relatively insignificant (at least on the mempools I was looking at from 10/1/15 - 10/10/15): it amounted to an average 0.3ms, from 10.9ms to 11.2ms, on my almost 2 year old hardware. Moreover, I think if this performance was ever an issue we could do a bigger re-engineering of ConnectBlock so that block relay isn't held up by updating the mempool state.

I would like to also expose this ancestor information via RPC, but will hold off until after #7292, so I might do that in a future PR rather than in this one. Also, the unit tests in this PR will be improved somewhat after #7539.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 5, 2016

Member

Needed rebase.

@sipa Regarding your comment about adding exact ancestor state checking to CTxMemPool::check() (#7600 (comment)), my only concern with adding that was slowing down the test suite to a potentially unacceptable level. I'm timing that now...

Member

sdaftuar commented Mar 5, 2016

Needed rebase.

@sipa Regarding your comment about adding exact ancestor state checking to CTxMemPool::check() (#7600 (comment)), my only concern with adding that was slowing down the test suite to a potentially unacceptable level. I'm timing that now...

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 8, 2016

Member

I tried adding complete ancestor-state checks to CTxMemPool::check() and timed the rpc tests (with -extended), and I saw negligible difference in times (<5%), so I've gone ahead and pushed that commit here.

I do expect this to be substantially slower, but I assume that since we can now run on mainnet with -checkmempool=<N> with N > 1 that we can just use bigger values of N if necessary.

Member

sdaftuar commented Mar 8, 2016

I tried adding complete ancestor-state checks to CTxMemPool::check() and timed the rpc tests (with -extended), and I saw negligible difference in times (<5%), so I've gone ahead and pushed that commit here.

I do expect this to be substantially slower, but I assume that since we can now run on mainnet with -checkmempool=<N> with N > 1 that we can just use bigger values of N if necessary.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 9, 2016

Member

Untested ACK

Member

sipa commented Mar 9, 2016

Untested ACK

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Mar 12, 2016

Contributor

In CalculateMempoolAncestors, how about wrapping the limit checks in if (fSearchForParents)? These checks should be unnecessary if entry is already in the mempool.

Contributor

dgenr8 commented Mar 12, 2016

In CalculateMempoolAncestors, how about wrapping the limit checks in if (fSearchForParents)? These checks should be unnecessary if entry is already in the mempool.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 14, 2016

Member

@dgenr8 I agree that refactoring that code would make that function clearer, but as the content of CalculateMemPoolAncestors() hasn't changed in this PR, I'd like to defer that refactor to a separate pull, to avoid making this PR any more difficult to review than it already is.

Member

sdaftuar commented Mar 14, 2016

@dgenr8 I agree that refactoring that code would make that function clearer, but as the content of CalculateMemPoolAncestors() hasn't changed in this PR, I'd like to defer that refactor to a separate pull, to avoid making this PR any more difficult to review than it already is.

sdaftuar added some commits Oct 19, 2015

Rename CTxMemPool::remove -> removeRecursive
remove is no longer called non-recursively, so simplify the logic
and eliminate an unnecessary parameter
Remove work limit in UpdateForDescendants()
The work limit served to prevent the descendant walking algorithm from doing
too much work by marking the parent transaction as dirty.  However to implement
ancestor tracking, it's not possible to similarly mark those descendant
transactions as dirty without having to calculate them to begin with.

This commit removes the work limit altogether.  With appropriate
chain limits (-limitdescendantcount) the concern about doing too much
work inside this function should be mitigated.
Add ancestor tracking to mempool
This implements caching of ancestor state to each mempool entry, similar to
descendant tracking, but also including caching sigops-with-ancestors (as that
metric will be helpful to future code that implements better transaction
selection in CreatenewBlock).
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Mar 14, 2016

Member

Needed rebase.

Member

sdaftuar commented Mar 14, 2016

Needed rebase.

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Mar 14, 2016

Contributor

Ok. Looking closer I see there's no real performance hit. I thought otherwise at first glance.

Contributor

dgenr8 commented Mar 14, 2016

Ok. Looking closer I see there's no real performance hit. I thought otherwise at first glance.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 16, 2016

Member

ACK.

Tested by running with -checkmempool=400 for a few days.

Member

sipa commented Mar 16, 2016

ACK.

Tested by running with -checkmempool=400 for a few days.

@laanwj laanwj merged commit ce019bf into bitcoin:master Mar 17, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Mar 17, 2016

Merge #7594: Mempool: Add tracking of ancestor packages
ce019bf Check all ancestor state in CTxMemPool::check() (Suhas Daftuar)
e2eeb5d Add ancestor feerate index to mempool (Suhas Daftuar)
72abd2c Add ancestor tracking to mempool (Suhas Daftuar)
76a7632 Remove work limit in UpdateForDescendants() (Suhas Daftuar)
5de2baa Rename CTxMemPool::remove -> removeRecursive (Suhas Daftuar)
7659438 CTxMemPool::removeForBlock now uses RemoveStaged (Suhas Daftuar)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 17, 2016

Member

utACK ce019bf

Member

laanwj commented Mar 17, 2016

utACK ce019bf

kyuupichan referenced this pull request in kyuupichan/BitcoinUnlimited Apr 27, 2017

Merge #7594: Mempool: Add tracking of ancestor packages
ce019bf Check all ancestor state in CTxMemPool::check() (Suhas Daftuar)
e2eeb5d Add ancestor feerate index to mempool (Suhas Daftuar)
72abd2c Add ancestor tracking to mempool (Suhas Daftuar)
76a7632 Remove work limit in UpdateForDescendants() (Suhas Daftuar)
5de2baa Rename CTxMemPool::remove -> removeRecursive (Suhas Daftuar)
7659438 CTxMemPool::removeForBlock now uses RemoveStaged (Suhas Daftuar)

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

Merge #7594: Mempool: Add tracking of ancestor packages
ce019bf Check all ancestor state in CTxMemPool::check() (Suhas Daftuar)
e2eeb5d Add ancestor feerate index to mempool (Suhas Daftuar)
72abd2c Add ancestor tracking to mempool (Suhas Daftuar)
76a7632 Remove work limit in UpdateForDescendants() (Suhas Daftuar)
5de2baa Rename CTxMemPool::remove -> removeRecursive (Suhas Daftuar)
7659438 CTxMemPool::removeForBlock now uses RemoveStaged (Suhas Daftuar)

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

Merge #7594: Mempool: Add tracking of ancestor packages
ce019bf Check all ancestor state in CTxMemPool::check() (Suhas Daftuar)
e2eeb5d Add ancestor feerate index to mempool (Suhas Daftuar)
72abd2c Add ancestor tracking to mempool (Suhas Daftuar)
76a7632 Remove work limit in UpdateForDescendants() (Suhas Daftuar)
5de2baa Rename CTxMemPool::remove -> removeRecursive (Suhas Daftuar)
7659438 CTxMemPool::removeForBlock now uses RemoveStaged (Suhas Daftuar)

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

Merge #7594: Mempool: Add tracking of ancestor packages
ce019bf Check all ancestor state in CTxMemPool::check() (Suhas Daftuar)
e2eeb5d Add ancestor feerate index to mempool (Suhas Daftuar)
72abd2c Add ancestor tracking to mempool (Suhas Daftuar)
76a7632 Remove work limit in UpdateForDescendants() (Suhas Daftuar)
5de2baa Rename CTxMemPool::remove -> removeRecursive (Suhas Daftuar)
7659438 CTxMemPool::removeForBlock now uses RemoveStaged (Suhas Daftuar)

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

Merge #7594: Mempool: Add tracking of ancestor packages
ce019bf Check all ancestor state in CTxMemPool::check() (Suhas Daftuar)
e2eeb5d Add ancestor feerate index to mempool (Suhas Daftuar)
72abd2c Add ancestor tracking to mempool (Suhas Daftuar)
76a7632 Remove work limit in UpdateForDescendants() (Suhas Daftuar)
5de2baa Rename CTxMemPool::remove -> removeRecursive (Suhas Daftuar)
7659438 CTxMemPool::removeForBlock now uses RemoveStaged (Suhas Daftuar)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment