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

[Wallet] Improve minimum absolute fee GUI options #7096

Merged
merged 4 commits into from Dec 1, 2015

Conversation

Projects
None yet
5 participants
@jonasschnelli
Member

jonasschnelli commented Nov 25, 2015

Currently, settxfee RPC call is broken.
It is supposed to take a feerate (fee per KB), instead it uses the given feerate as absolute fee. We have even started taking this misconduct into RPC tests (https://github.com/bitcoin/bitcoin/blob/0.11/qa/rpc-tests/wallet.py#L107). (fixed by #7103)

This PR fixes the settxfee RPC call (by fixing the underlaying problem) and decouples the UI CoinControl function for absolute min. fees from the global wallet state. Absolute fees without actually controlling the amount and type/size of inputs makes no sense and therefore this PR "hides" the absolute minimum fee behind the coin control feature and will only be enabled if the user has manually selected inputs.

Includes work from @sipa. Thanks!

Supersedes: #6708
Fixes: #6479

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 25, 2015

Member

Supersedes: #6708
Fixes: #6479

Member

jonasschnelli commented Nov 25, 2015

Supersedes: #6708
Fixes: #6479

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 25, 2015

Member

Concept ACK. Haven't looked closely at the code yet.

Currently, settxfee RPC call is broken.

That's pretty serious. Just curious: when did this break, did this problem make it into any release?

Member

laanwj commented Nov 25, 2015

Concept ACK. Haven't looked closely at the code yet.

Currently, settxfee RPC call is broken.

That's pretty serious. Just curious: when did this break, did this problem make it into any release?

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 25, 2015

Member

That's pretty serious. Just curious: when did this break, did this problem make it into any release?

I try to unroll the bug-history right now. But I'm pretty sure the 0.11er releases are affected because this RPC tests (which tests the buggy behavior) is from the 0.11er branch https://github.com/bitcoin/bitcoin/blob/v0.11.0/qa/rpc-tests/wallet.py#L107).

Member

jonasschnelli commented Nov 25, 2015

That's pretty serious. Just curious: when did this break, did this problem make it into any release?

I try to unroll the bug-history right now. But I'm pretty sure the 0.11er releases are affected because this RPC tests (which tests the buggy behavior) is from the 0.11er branch https://github.com/bitcoin/bitcoin/blob/v0.11.0/qa/rpc-tests/wallet.py#L107).

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 25, 2015

Member

I think the problem source lays in #5200 ... especially this line was a dangerous change (https://github.com/bitcoin/bitcoin/pull/5200/files#diff-d7618bdc04db23aa74d6a5a4198c58fdR1635)

During the subtractFeeFromAmount work we started to take the wrong behavior into RPC tests: #5831

However: this PR needs a backport to 0.11.

Member

jonasschnelli commented Nov 25, 2015

I think the problem source lays in #5200 ... especially this line was a dangerous change (https://github.com/bitcoin/bitcoin/pull/5200/files#diff-d7618bdc04db23aa74d6a5a4198c58fdR1635)

During the subtractFeeFromAmount work we started to take the wrong behavior into RPC tests: #5831

However: this PR needs a backport to 0.11.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 26, 2015

Member

Supersedes: #6708

Let's not call it "supersedes". It's an alternative bug fix with a similar result. Someone needs to check if the GUI changes are ok this way.

Member

MarcoFalke commented Nov 26, 2015

Supersedes: #6708

Let's not call it "supersedes". It's an alternative bug fix with a similar result. Someone needs to check if the GUI changes are ok this way.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 26, 2015

Member

I think it would be best to split this into two pulls:

  • Fix settxfee RPC
  • Improve the GUI

At least the former (which is straightforward to test) must be backported to 0.11.

Member

laanwj commented Nov 26, 2015

I think it would be best to split this into two pulls:

  • Fix settxfee RPC
  • Improve the GUI

At least the former (which is straightforward to test) must be backported to 0.11.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 26, 2015

Member

A quick fix was already opened (#6649). This PR would fix the settxfee RPC issue. But, if one uses the PRC together with the GUI, it can result in a wrong behavior. If a user presses the radio button "Use a minimum absolute fee of", suddenly all RPC send commands use the former settxfee feerate as absolute fee.

We could merge and backport 6649 and fix the GUI within this PR.

Member

jonasschnelli commented Nov 26, 2015

A quick fix was already opened (#6649). This PR would fix the settxfee RPC issue. But, if one uses the PRC together with the GUI, it can result in a wrong behavior. If a user presses the radio button "Use a minimum absolute fee of", suddenly all RPC send commands use the former settxfee feerate as absolute fee.

We could merge and backport 6649 and fix the GUI within this PR.

@MarcoFalke

View changes

Show outdated Hide outdated qa/rpc-tests/wallet.py
#send a tx with value in a string (PR#6380 +)
txId = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), "2")
txObj = self.nodes[0].gettransaction(txId)
assert_equal(txObj['amount'], Decimal('-2.00000000'))

This comment has been minimized.

@MarcoFalke
@MarcoFalke
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 30, 2015

Member

@jonasschnelli Mind to address the rebase "nit"?

Member

MarcoFalke commented Nov 30, 2015

@jonasschnelli Mind to address the rebase "nit"?

sipa and others added some commits Nov 25, 2015

[Qt] improve minimum absolute fee option
- Only display the minimum absolute fee control if CoinControl is enabled

@jonasschnelli jonasschnelli changed the title from [Wallet] fix settxfee and improve minimum absolute fee GUI options to [Wallet] Improve minimum absolute fee GUI options Nov 30, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 30, 2015

Member

Rebased and – after #7103 – it's now GUI only.

Member

jonasschnelli commented Nov 30, 2015

Rebased and – after #7103 – it's now GUI only.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Nov 30, 2015

Member

ACK

Member

btcdrak commented Nov 30, 2015

ACK

@laanwj laanwj merged commit ff723da into bitcoin:master Dec 1, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Dec 1, 2015

Merge pull request #7096
ff723da [Qt] improve minimum absolute fee option - Only display the minimum absolute fee control if CoinControl is enabled (Jonas Schnelli)
31b508a [Qt] make use of the nMinimumTotalFee (absolute) in coincontrols fee calculation (Jonas Schnelli)
80462dd [Qt] use ASYMP_UTF8 (≈) whenever we show a fee that is not absolute (Jonas Schnelli)
ecc7c82 Move fPayAtLeastCustomFee function to CC (Pieter Wuille)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment