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.

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

Lower bound priority #7008

Merged
merged 3 commits into from Nov 30, 2015
Merged

Lower bound priority #7008

merged 3 commits into from Nov 30, 2015

Conversation

@morcos
Copy link
Member

morcos commented Nov 13, 2015

I think this is a good compromise with regards to priority for 0.12 release. It is a calculation of priority in which only inputs that are in the chain at the time the transaction enters the mempool are aged.

It is a minimal amount of code and changes the CTxMemPoolEntry::GetPriority function to return a lower bound of the correctly computed priority as opposed to an upper bound. This would allow the fast mining code in #6898 to use a priority calculation that at least ages the originally in chain inputs as opposed to using only starting priority.
I believe this better strikes the right balance between preserving the ability to meet previous policy objectives and adding too much complication to the code for a concept that is deprecated. I plan to close #6357 in favor of this.

One caveat: In the event of a reorg, if one of the originally in-chain inputs is now no longer in chain (or has been moved to a different height) the calculation will no longer be a strict lower bound on priority because it will not take this into account.

It should be kept in mind though, that this is a clear improvement on the current calculation.

(Built on top of #7007 so the tests pass)

(EDIT: For anyone that has already reviewed #6357, this is almost a strict subset)

@sdaftuar
Copy link
Member

sdaftuar commented Nov 17, 2015

utACK

@sipa
Copy link
Member

sipa commented Nov 17, 2015

Concept ACK

@morcos morcos force-pushed the morcos:lowerBoundPriority branch to bea369e Nov 17, 2015
@morcos
Copy link
Member Author

morcos commented Nov 17, 2015

This was rebased now that #7020 was merged, and I removed the concept of default arguments from the CTxMemPoolEntry constructor.

@morcos morcos mentioned this pull request Nov 18, 2015
@luke-jr
Copy link
Member

luke-jr commented Nov 18, 2015

What are the benefits of this over #6357 ?

@sipa
Copy link
Member

sipa commented Nov 18, 2015

morcos added 3 commits Jun 30, 2015
Compute the value of inputs that already are in the chain at time of mempool entry and only increase priority due to aging for those inputs.  This effectively changes the CTxMemPoolEntry's GetPriority calculation from an upper bound to a lower bound.
@@ -239,8 +240,9 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const
const CCoins* coins = AccessCoins(txin.prevout.hash);
assert(coins);
if (!coins->IsAvailable(txin.prevout.n)) continue;
if (coins->nHeight < nHeight) {
if (coins->nHeight <= nHeight) {

This comment has been minimized.

Copy link
@petertodd

petertodd Nov 20, 2015

Contributor

Why was this line changed?

This comment has been minimized.

Copy link
@petertodd

petertodd Nov 20, 2015

Contributor

Oh wait, I get it, never mind.

This comment has been minimized.

Copy link
@sipa

sipa Nov 26, 2015

Member

Good catch, but indeed :)

/**
* Fast calculation of lower bound of current priority as update
* from entry priority. Only originally in-chain inputs will age
*/

This comment has been minimized.

Copy link
@petertodd

petertodd Nov 20, 2015

Contributor

"Only originally in-chain inputs will age" <- looks like you're missing something here.

This comment has been minimized.

Copy link
@morcos

morcos Nov 20, 2015

Author Member

I'm happy to make it clearer, but that was the intended wording except for perhaps a missing ".".

This comment has been minimized.

Copy link
@petertodd

petertodd Nov 20, 2015

Contributor

How about you change it to "Only inputs that were originally in-chain will age."

On 20 November 2015 16:34:30 GMT-05:00, Alex Morcos notifications@github.com wrote:

 CTxMemPoolEntry(const CTxMemPoolEntry& other);

 const CTransaction& GetTx() const { return this->tx; }
  • /**
  • \* Fast calculation of lower bound of current priority as update
    
  • \* from entry priority.  Only originally in-chain inputs will
    
    age
  • */
    

I'm happy to make it clearer, but that was the intended wording except
for perhaps a missing ".".


Reply to this email directly or view it on GitHub:
https://github.com/bitcoin/bitcoin/pull/7008/files#r45521266

@petertodd
Copy link
Contributor

petertodd commented Nov 20, 2015

utACK, modulo comment nit.

@jtimon
Copy link
Member

jtimon commented Nov 23, 2015

utACK

@dcousens
Copy link
Contributor

dcousens commented Nov 24, 2015

Is it worth changing this in light of the eventual removal of priority?
If it is, ACK.

@sdaftuar
Copy link
Member

sdaftuar commented Nov 24, 2015

Added a commit to address @petertodd's comment nit.

@dcousens Yes I think this change is needed for 0.12, since all the priority code won't be eliminated yet. This seems like the best compromise for 0.12.

@jtimon
Copy link
Member

jtimon commented Nov 24, 2015

In any case, merging this makes other mempool related PRs more readable, it's small and it doesn't preclude the future elimination of the priority code (and there's currently no rebased version of a priority-removing PR that I know of). So even if I'm very much in favor of directly removing the priority, I would be happy to see this merged soon.

I think some squashing could be done here though, at the very least for the comment fixup commit.

@sipa
Copy link
Member

sipa commented Nov 26, 2015

utACK

@sipa sipa added this to the 0.12.0 milestone Nov 28, 2015
@morcos morcos force-pushed the morcos:lowerBoundPriority branch 2 times, most recently to c035306 Nov 30, 2015
@morcos
Copy link
Member Author

morcos commented Nov 30, 2015

rebased

@laanwj laanwj merged commit c035306 into bitcoin:master Nov 30, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
laanwj added a commit that referenced this pull request Nov 30, 2015
c035306 Change GetPriority calculation. (Alex Morcos)
71f1d9f Modify variable names for entry height and priority (Alex Morcos)
5945819 Remove default arguments for CTxMemPoolEntry() (Alex Morcos)
@@ -63,9 +63,10 @@ class CTxMemPoolEntry
size_t nModSize; //! ... and modified size for priority
size_t nUsageSize; //! ... and total memory usage
int64_t nTime; //! Local time when entering the mempool
double dPriority; //! Priority when entering the mempool

This comment has been minimized.

Copy link
@jtimon

jtimon Dec 2, 2015

Member

What was the motivation to bike-sheed dPriority and nHeight?

This comment has been minimized.

Copy link
@morcos

morcos Dec 2, 2015

Author Member

This was originally the first part of the changes necessary for #6357 (now #7149)
It was necessary to distinguish between the two stored priorities and heights. The original "entry" priority and height and the latest calculated "cached" priority and the height that was calculated at.

It seemed logical to me to leave those changes in given that we didn't know whether the second half would be merged or not and given that they are more accurate names anyway.

This comment has been minimized.

Copy link
@jtimon

jtimon Dec 19, 2015

Member

It was necessary to distinguish between the two stored priorities and heights. The original "entry" priority and height and the latest calculated "cached" priority and the height that was calculated at.

I guess if the rename had been introduced when it was necessary instead of here I wouldn't have complained.

It seemed logical to me to leave those changes in given that we didn't know whether the second half would be merged or not and given that they are more accurate names anyway.

I'm confused, how are CTxMemPoolEntry::nHeight and CTxMemPoolEntry::dPriority "less accurate" than CTxMemPoolEntry::entryHeight and CTxMemPoolEntry::entryPriority ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

8 participants
You can’t perform that action at this time.