Lower bound priority

morcos commented at 4:06 pm on November 13, 2015: member

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)

laanwj added the label Mempool on Nov 13, 2015

morcos force-pushed on Nov 14, 2015

sdaftuar commented at 5:56 pm on November 17, 2015: member

utACK

sipa commented at 9:40 pm on November 17, 2015: member

Concept ACK

morcos force-pushed on Nov 17, 2015

morcos commented at 9:46 pm on November 17, 2015: member

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

luke-jr commented at 5:59 am on November 18, 2015: member

What are the benefits of this over #6357 ?

sipa commented at 7:54 am on November 18, 2015: member

It’s simpler.

Remove default arguments for CTxMemPoolEntry() 5945819717

Modify variable names for entry height and priority 71f1d9fd4a

Change GetPriority calculation.

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.

c0353064dd

in src/coins.cpp: in bea369e0ed outdated

239@@ -239,8 +240,9 @@ double CCoinsViewCache::GetPriority(const CTransaction &tx, int nHeight) const
240         const CCoins* coins = AccessCoins(txin.prevout.hash);
241         assert(coins);
242         if (!coins->IsAvailable(txin.prevout.n)) continue;
243-        if (coins->nHeight < nHeight) {
244+        if (coins->nHeight <= nHeight) {

petertodd commented at 8:50 pm on November 20, 2015:

Why was this line changed?

petertodd commented at 8:54 pm on November 20, 2015:

Oh wait, I get it, never mind.

sipa commented at 9:24 pm on November 26, 2015:

Good catch, but indeed :)

in src/txmempool.h: in bea369e0ed outdated

86 
87     const CTransaction& GetTx() const { return this->tx; }
88+    /**
89+     * Fast calculation of lower bound of current priority as update
90+     * from entry priority.  Only originally in-chain inputs will age
91+     */

petertodd commented at 9:09 pm on November 20, 2015:

“Only originally in-chain inputs will age” <- looks like you’re missing something here.

morcos commented at 9:33 pm on November 20, 2015:

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

petertodd commented at 10:19 pm on November 20, 2015:

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:

0 CTxMemPoolEntry(const CTxMemPoolEntry& other);
1
2 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 commented at 9:17 pm on November 20, 2015: contributor

utACK, modulo comment nit.

jtimon commented at 12:59 pm on November 23, 2015: contributor

utACK

dcousens commented at 1:53 am on November 24, 2015: contributor

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

sdaftuar commented at 1:35 pm on November 24, 2015: member

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 commented at 2:41 pm on November 24, 2015: contributor

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 commented at 9:27 pm on November 26, 2015: member

utACK

sipa added this to the milestone 0.12.0 on Nov 28, 2015

morcos force-pushed on Nov 30, 2015

morcos commented at 1:26 pm on November 30, 2015: member

rebased

laanwj merged this on Nov 30, 2015

laanwj closed this on Nov 30, 2015

laanwj referenced this in commit eb7741605b on Nov 30, 2015

in src/txmempool.h: in c0353064dd

62@@ -63,9 +63,10 @@ class CTxMemPoolEntry
63     size_t nModSize; //! ... and modified size for priority
64     size_t nUsageSize; //! ... and total memory usage
65     int64_t nTime; //! Local time when entering the mempool
66-    double dPriority; //! Priority when entering the mempool

jtimon commented at 1:47 pm on December 2, 2015:

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

morcos commented at 2:32 pm on December 2, 2015:

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.

jtimon commented at 0:17 am on December 19, 2015:

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 ?

random-zebra referenced this in commit 6bc917e859 on Jul 6, 2020

MarcoFalke locked this on Sep 8, 2021

Lower bound priority #7008