Skip to content

Fee fixes#4465

Merged
laanwj merged 1 commit intobitcoin:masterfrom
cozz:cozz11
Jul 8, 2014
Merged

Fee fixes#4465
laanwj merged 1 commit intobitcoin:masterfrom
cozz:cozz11

Conversation

@cozz
Copy link
Contributor

@cozz cozz commented Jul 5, 2014

  • fix bug in CWallet::CreateTransaction. If dPriorityNeeded is -1, then dPriority >= dPriorityNeeded is always true,
    so we always "break;"
  • CFeeRate::GetFee(..) currently returns zero for very small nSatoshisPerK.
    For example, if you set the voluntary fee to 1 satoshi, and the tx has less than 1000 bytes,
    payTxFee.GetFee(nTxBytes) returns zero, this results in CWallet::GetMinimumFee(..) to return
    pool.estimateFee(..) instead of the voluntary fee. So its not possible to set the voluntary fee as low as 1 satoshi.
  • Add fallback hard-coded AllowFree to coin control, because CreateTransaction also does.
  • Handle voluntary fee case in coin control. nPayFee was set to zero if
    dPriority >= dPriorityNeeded, even in the voluntary fee case. And we need to consider, that
    CreateTransaction pays at least for 1000 bytes in the voluntary fee case.

Just a question, why did -mintxfee move from Testing options to Wallet options?
I dont think we should bother people with this, otherwise we wouldnt need smart fee in the first place.

@cozz cozz mentioned this pull request Jul 5, 2014
@laanwj
Copy link
Member

laanwj commented Jul 7, 2014

Just a question, why did -mintxfee move from Testing options to Wallet options?

Because it is a wallet option. If you think it should be hidden as a debugging option put it behind the -help-debug flag.

@laanwj laanwj added the Wallet label Jul 7, 2014
@gavinandresen
Copy link
Contributor

ACK. Good catch on the dPriorityNeeded = -1 bug.

I could quibble with the CFeeRate::GetFee() change (you cannot set a voluntary fee rate less than the hard-coded minRelayTxFee), but the code change is fine belt-and-suspenders.

@BitcoinPullTester
Copy link

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4465_d88af560111863c3e9c1ae855dcc287f04dffb02/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@cozz
Copy link
Contributor Author

cozz commented Jul 8, 2014

update:

  • rebased and hide -mintxfee by default from the command line help

@laanwj laanwj merged commit d88af56 into bitcoin:master Jul 8, 2014
laanwj added a commit that referenced this pull request Jul 8, 2014
d88af56 Fee fixes (Cozz Lovan)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants