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

[qt] Update coin control and smartfee labels #6887

Merged
merged 2 commits into from Nov 5, 2015

Conversation

Projects
None yet
6 participants
@MarcoFalke
Member

MarcoFalke commented Oct 25, 2015

This will sync the qt labels with the wallet's fee code.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Oct 25, 2015

Member

So the idea here is to use the min relay fee instead of the wallet's configured fee, whichever is higher?

Member

luke-jr commented Oct 25, 2015

So the idea here is to use the min relay fee instead of the wallet's configured fee, whichever is higher?

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 25, 2015

Member

Yes, it's just what the wallet already does. This should make qt consistent again. C.f.

  • 037b4f1 (Make mining fee policy match relay fee policy) and
  • aa279d6 (Enforce minRelayTxFee on wallet created tx)
Member

MarcoFalke commented Oct 25, 2015

Yes, it's just what the wallet already does. This should make qt consistent again. C.f.

  • 037b4f1 (Make mining fee policy match relay fee policy) and
  • aa279d6 (Enforce minRelayTxFee on wallet created tx)

@laanwj laanwj added the GUI label Oct 26, 2015

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Oct 27, 2015

Contributor

ut ACK

Contributor

jgarzik commented Oct 27, 2015

ut ACK

@@ -2120,6 +2120,11 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, CReserveKey& reservekey)
return true;
}
CAmount CWallet::GetRequiredFee(unsigned int nTxBytes)
{
return std::max(minTxFee.GetFee(nTxBytes), ::minRelayTxFee.GetFee(nTxBytes));

This comment has been minimized.

@jonasschnelli

jonasschnelli Oct 29, 2015

Member

Sad to see another coupling between the Wallet and the Mempool/Node. But it's already all over.
Ideally the wallet should not directly access global mempool variables instead it should get data over a general interface.

@jonasschnelli

jonasschnelli Oct 29, 2015

Member

Sad to see another coupling between the Wallet and the Mempool/Node. But it's already all over.
Ideally the wallet should not directly access global mempool variables instead it should get data over a general interface.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Oct 29, 2015

Member

Makes sense.
utACK.

Member

jonasschnelli commented Oct 29, 2015

Makes sense.
utACK.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 3, 2015

Member

@morcos This is addressing your issue #5202. Mind to review?

Member

MarcoFalke commented Nov 3, 2015

@morcos This is addressing your issue #5202. Mind to review?

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 3, 2015

Member

I think most of the pull makes sense, but I'd save removing the AbsurdFee check until we replace it with something better. Having the only effective high fee check be the ATMP check is dicey since that depends on minRelayTxFee which people might be adjusting for other reasons. Also I'm not sure how this is meant to work with #6726. I don't like the idea of having less and less checks for accidental high fees. I just think we should rework them to be better organized.

Member

morcos commented Nov 3, 2015

I think most of the pull makes sense, but I'd save removing the AbsurdFee check until we replace it with something better. Having the only effective high fee check be the ATMP check is dicey since that depends on minRelayTxFee which people might be adjusting for other reasons. Also I'm not sure how this is meant to work with #6726. I don't like the idea of having less and less checks for accidental high fees. I just think we should rework them to be better organized.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 4, 2015

Member

I agree with @morcos. The absurdfee check is just a belt-and-suspenders check, in case something goes wrong due to a bug in another part of the code. I don't feel good about removing it.
I also don't think that change belongs in a "Update coin control and smartfee labels" pull.

Member

laanwj commented Nov 4, 2015

I agree with @morcos. The absurdfee check is just a belt-and-suspenders check, in case something goes wrong due to a bug in another part of the code. I don't feel good about removing it.
I also don't think that change belongs in a "Update coin control and smartfee labels" pull.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Nov 4, 2015

Member

@laanwj I also don't think that change belongs in a "Update coin control and smartfee labels" pull.

Still, the fee label behaves wrong and needs update, but I force dropped the last commit to open a new PR for this...

Member

MarcoFalke commented Nov 4, 2015

@laanwj I also don't think that change belongs in a "Update coin control and smartfee labels" pull.

Still, the fee label behaves wrong and needs update, but I force dropped the last commit to open a new PR for this...

@laanwj laanwj merged commit 53238ff into bitcoin:master Nov 5, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Nov 5, 2015

Merge pull request #6887
53238ff Clarify what minrelaytxfee does (MarcoFalke)
abd8b76 [qt] Properly display required fee instead of minTxFee (MarcoFalke)

@MarcoFalke MarcoFalke deleted the MarcoFalke:MarcoFalke-2015-qtMaxMin_Fee_and_Max_Fee branch Nov 5, 2015

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