Add sane fallback for fee estimation #7296

Merged
merged 3 commits into from Jan 13, 2016

Conversation

Projects
None yet
7 participants
@morcos
Member

morcos commented Jan 5, 2016

Make fee estimation logic respect GetRequiredFee at all times.
In addition add new commandline option "-fallbackfee" to use when fee estimation does not have sufficient data.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 5, 2016

Member

utACK cc99c41

Member

MarcoFalke commented Jan 5, 2016

utACK cc99c41

@MarcoFalke

View changes

src/wallet/wallet.cpp
@@ -47,6 +47,12 @@ bool fSendFreeTransactions = DEFAULT_SEND_FREE_TRANSACTIONS;
* Override with -mintxfee
*/
CFeeRate CWallet::minTxFee = CFeeRate(DEFAULT_TRANSACTION_MINFEE);
+/**
+ * If fee estimation does not have enough data to provide estimates, the use this fee instead.

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 5, 2016

Member

Nit: , then use ...

@MarcoFalke

MarcoFalke Jan 5, 2016

Member

Nit: , then use ...

+ nFeeNeeded = fallbackFee.GetFee(nTxBytes);
+ }
+ // prevent user from paying a fee below minRelayTxFee or minTxFee
+ nFeeNeeded = std::max(nFeeNeeded, GetRequiredFee(nTxBytes));

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 5, 2016

Member

This is important on it's own. Could you split this diff (line 2236-2237) into a separate commit?

Edit: See 995b9f3

@MarcoFalke

MarcoFalke Jan 5, 2016

Member

This is important on it's own. Could you split this diff (line 2236-2237) into a separate commit?

Edit: See 995b9f3

@@ -41,6 +41,8 @@ static const unsigned int DEFAULT_KEYPOOL_SIZE = 100;
static const CAmount DEFAULT_TRANSACTION_FEE = 0;
//! -paytxfee will warn if called with a higher fee than this amount (in satoshis) per KB
static const CAmount nHighTransactionFeeWarning = 0.01 * COIN;
+//! -fallbackfee default
+static const CAmount DEFAULT_FALLBACK_FEE = 20000;

This comment has been minimized.

@MarcoFalke

MarcoFalke Jan 5, 2016

Member

I'd say 11000 is enough, we can determine a "better" DEFAULT_ for 0.13

@MarcoFalke

MarcoFalke Jan 5, 2016

Member

I'd say 11000 is enough, we can determine a "better" DEFAULT_ for 0.13

morcos added some commits Jan 5, 2016

Add sane fallback for fee estimation
Add new commandline option "-fallbackfee" to use when fee estimation does not have sufficient data.
@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 5, 2016

Member

Addressed @MarcoFalke's comments.
Although left fee = 20k satoshis until the bikeshedding is finished. I don't care want goes there, but I propose using the result of estimatefee(4) with a significantly longer time decay over the last few months. I'll post the result of that once calculated.

This is meant for 0.12 backport

Member

morcos commented Jan 5, 2016

Addressed @MarcoFalke's comments.
Although left fee = 20k satoshis until the bikeshedding is finished. I don't care want goes there, but I propose using the result of estimatefee(4) with a significantly longer time decay over the last few months. I'll post the result of that once calculated.

This is meant for 0.12 backport

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 5, 2016

@morcos Travis fails this. I guess you could add -fallbackfee=1000 to mempool_limit.py. (untested)

@morcos Travis fails this. I guess you could add -fallbackfee=1000 to mempool_limit.py. (untested)

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 5, 2016

Member

@MarcoFalke ugh.. thanks. more than just that one fail. These are probably things that should be fixed b/c the travis tests that fail are implicitly depending on estimatefee returning minrelaytxfee, which isn't a good assumption. I'm working on it

Member

morcos commented Jan 5, 2016

@MarcoFalke ugh.. thanks. more than just that one fail. These are probably things that should be fixed b/c the travis tests that fail are implicitly depending on estimatefee returning minrelaytxfee, which isn't a good assumption. I'm working on it

@wumpus

This comment has been minimized.

Show comment
Hide comment
@wumpus

wumpus Jan 6, 2016

Reminder: @wumpus on github is not a bitcoin person.

wumpus commented Jan 6, 2016

Reminder: @wumpus on github is not a bitcoin person.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jan 6, 2016

Member

Apologies.

@laanwj I think we could use this in 0.12

Member

morcos commented Jan 6, 2016

Apologies.

@laanwj I think we could use this in 0.12

@laanwj laanwj added the Wallet label Jan 7, 2016

@laanwj laanwj added this to the 0.12.0 milestone Jan 7, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 7, 2016

Member

Concept ACK.

One nit: this adds yet another -XXXfee parameter.
Although it makes sense, for the wallet we already have -mintxfee, -paytxfee, -maxtxfee this wild growth of options does get a bit confusing for users, I think.
But I don't know a better solution either.

Member

laanwj commented Jan 7, 2016

Concept ACK.

One nit: this adds yet another -XXXfee parameter.
Although it makes sense, for the wallet we already have -mintxfee, -paytxfee, -maxtxfee this wild growth of options does get a bit confusing for users, I think.
But I don't know a better solution either.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jan 8, 2016

Contributor

@laanwj it's also not clear whether those parameters are referring to the wallet or to relay policy (until you read the documentation in-depth, anyway).

Contributor

dcousens commented Jan 8, 2016

@laanwj it's also not clear whether those parameters are referring to the wallet or to relay policy (until you read the documentation in-depth, anyway).

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jan 8, 2016

Member

Concept ACK will test later today.

Member

btcdrak commented Jan 8, 2016

Concept ACK will test later today.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jan 12, 2016

Member

Tested, ACK bebe58b

Member

sdaftuar commented Jan 12, 2016

Tested, ACK bebe58b

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jan 12, 2016

Member
Member

MarcoFalke commented Jan 12, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 13, 2016

Member

utACK

Member

laanwj commented Jan 13, 2016

utACK

@laanwj laanwj merged commit bebe58b into bitcoin:master Jan 13, 2016

1 check passed

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

laanwj added a commit that referenced this pull request Jan 13, 2016

Merge pull request #7296
bebe58b SQUASHME: Fix rpc tests that assumed fallback to minRelayTxFee (Alex Morcos)
e420a1b Add sane fallback for fee estimation (Alex Morcos)
995b9f3 Always respect GetRequiredFee for wallet txs (Alex Morcos)

laanwj added a commit that referenced this pull request Jan 13, 2016

Add sane fallback for fee estimation
- Always respect GetRequiredFee for wallet txs
- Add sane fallback for fee estimation
- SQUASHME: Fix rpc tests that assumed fallback to minRelayTxFee

Add new commandline option "-fallbackfee" to use when fee estimation does not have sufficient data.

Github-Pull: #7296
Rebased-From: 995b9f3 e420a1b bebe58b
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jan 13, 2016

Member

Cherry-picked to 0.12 as a36d79b

Member

laanwj commented Jan 13, 2016

Cherry-picked to 0.12 as a36d79b

@laanwj laanwj removed the Needs backport label Feb 4, 2016

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Dec 27, 2017

Merged

Add fundrawtransaction #288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment