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

Remove priority estimation #7730

Closed
wants to merge 3 commits into from

Conversation

@morcos
Copy link
Member

commented Mar 21, 2016

This removes the functionality behind estimatepriority and estimatesmartpriority. The rpc calls are now deprecated. estimatepriority will return -1 always and estimatesmartpriority will return 1e24 if the mempool is currently limited and -1 otherwise. The result of this behavior is that free transactions (if selected using the now debug option -sendfreetransactions) can be created and sent if the mempool is not currently limited and the transaction's priority is above the hard coded AllowFree threshold.

This is effectively the behavior in place already as priority estimates do not appear until confirmation targets over 50 which aren't currently supported.

A side effect of this is that now all transactions (that aren't dependent on unformed inputs) are now considered data points for fee estimation. Even though some transactions may be mined due to their priority instead of fee, this is still safe because the threshold for fee estimation is very high (95%).

@morcos morcos force-pushed the morcos:removePriEst branch Mar 21, 2016

@dcousens

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2016

concept ACK

@luke-jr

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

So this essentially changes priority from "fully supported for usage" to "fallback only", at least from the perspective of Core's wallet. Concept ACK.

@morcos morcos force-pushed the morcos:removePriEst branch Mar 22, 2016

@petertodd

This comment has been minimized.

Copy link
Contributor

commented Mar 30, 2016

utACK 2f3c306

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented May 9, 2016

Concept ACK, would be good to see this rebased.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 9, 2016

Concept ACK

@sipa

This comment has been minimized.

Copy link
Member

commented May 9, 2016

Needs rebase.

@morcos morcos force-pushed the morcos:removePriEst branch May 18, 2016

@morcos

This comment has been minimized.

Copy link
Member Author

commented May 18, 2016

trivial rebase

@morcos morcos force-pushed the morcos:removePriEst branch May 19, 2016

@petertodd

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

utACK 9a936e3

@MarcoFalke

View changes

src/policy/fees.cpp Outdated
nBestSeenHeight = nFileBestSeenHeight;
if (nVersionRequired < 129900) {

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 20, 2016

Member

nit: could be a const.

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 30, 2016

Member

Also needs bump

@@ -166,8 +165,8 @@ double TxConfirmStats::EstimateMedianVal(int confTarget, double sufficientTxVal,
}
}

LogPrint("estimatefee", "%3d: For conf success %s %4.2f need %s %s: %12.5g from buckets %8g - %8g Cur Bucket stats %6.2f%% %8.1f/(%.1f+%d mempool)\n",

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke May 20, 2016

Member

nit: (unrelated to this pull)

I think estimatefee is missing from debugCategories.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 20, 2016

utACK 9a936e3

@sipa

This comment has been minimized.

Copy link
Member

commented Aug 18, 2016

Rebase please?

src/test/policyestimator_tests.cpp Outdated
tx.vin[0].prevout.n = 10000*blocknum+100*j+k;
uint256 hash = tx.GetHash();
mpool.addUnchecked(hash, entry.Fee(feeV[k/4][j]).Time(GetTime()).Priority(priV[k/4][j]).Height(blocknum).FromTx(tx, &mpool));
mpool.addUnchecked(hash, entry.Fee(feeV[j]).Time(GetTime()).Priority(0).Height(blocknum).FromTx(tx, &mpool));
CTransaction btx;
if (mpool.lookup(hash, btx))

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 30, 2016

Member

Needs merge conflict solved here due to 288d85d

@sipa

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

Do we want this for 0.14?

@morcos morcos force-pushed the morcos:removePriEst branch to aacda22 Nov 4, 2016

@morcos

This comment has been minimized.

Copy link
Member Author

commented Nov 4, 2016

Sorry for the delay.. I rebased including bumping the version numbers, and I just wanted to flag another minor change I made since the previous version had been acked. This seems like the correct way to handle reading old version files.

@morcos morcos force-pushed the morcos:removePriEst branch to 9ea4ed9 Nov 4, 2016

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 5, 2016

utACK 9ea4ed9.

Mind to reword or squash the "SQUASHME" commit?

@MarcoFalke MarcoFalke added this to the 0.14.0 milestone Nov 5, 2016

laanwj added a commit that referenced this pull request Nov 7, 2016
Merge #7730: Remove priority estimation
0bd581a add release notes for removal of priority estimation (Alex Morcos)
b2322e0 Remove priority estimation (Alex Morcos)
@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 7, 2016

Merged (and squashed) via 3c03dc2

@laanwj laanwj closed this Nov 7, 2016

@sipa sipa referenced this pull request Jan 10, 2017
16 of 18 tasks complete
codablock added a commit to codablock/dash that referenced this pull request Jan 13, 2018
Merge bitcoin#7730: Remove priority estimation
0bd581a add release notes for removal of priority estimation (Alex Morcos)
b2322e0 Remove priority estimation (Alex Morcos)
@dagurval dagurval referenced this pull request Jun 19, 2018
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
Merge bitcoin#7730: Remove priority estimation
0bd581a add release notes for removal of priority estimation (Alex Morcos)
b2322e0 Remove priority estimation (Alex Morcos)
CryptoCentric added a commit to absolute-community/absolute that referenced this pull request Feb 15, 2019
Merge bitcoin#7730: Remove priority estimation
0bd581a add release notes for removal of priority estimation (Alex Morcos)
b2322e0 Remove priority estimation (Alex Morcos)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.