[Qt] Add Smartfee to GUI #5200
[Qt] Add Smartfee to GUI #5200
Conversation
|
Whoa, great work |
src/wallet.cpp
Outdated
There was a problem hiding this comment.
Changing this core default is probably accidental?
There was a problem hiding this comment.
This is the default for the wallet only, not the relay fee minimum. (just pointing out; not saying it's intentional or not).
There was a problem hiding this comment.
Sure, its intentional, we have reduced the minrelayfee from 10000 to 1000 satoshis in 0.9. But this did not make transactions cheaper for the people, because the wallet still paid 10000 (which is good, to ensure everybodys txs get relayed). After the 0.9 release there were even blogs about "How to make your bitcoin transactions cheaper", they told people to start bitcoin with -mintxfee=0.00001000 (1000 satoshis). Back then this was a little dangerous, as your tx may not be relayed, but now its safe. This is just the second step of staging.
So, yes, we should definitely reduce the mintxfee to 1000 satoshis for the 0.10 release,
See #3305.
Bitcoin transactions are super cheap these days as we also removed the rounding, a 192 bytes transaction only costs 192 satoshis. Its actually 1 satoshi per byte.
There was a problem hiding this comment.
OK - agreed, although it could be argued this needs to be in a separate change from the GUI changes, as it affects the RPC wallet as well
|
Agreed. This is a really nice change! I like how the slider starts at normal and you have to consciously select a lower confirm target. If we end up using #5159 or something similar, you might want to change the uninitialized error message. It's possible to consistently not get an answer for a very low confirmation target like 1, because there is no fee that is reliably high enough and it seems kind of easy to not notice that's whats happening and you're accidentally putting a very low fee. Could the slider even snap back to the lowest confirm target which works? |
src/qt/coincontroldialog.cpp
Outdated
There was a problem hiding this comment.
Seems like it might be nice to move this call outside of the loops in updateLabels and updateView since its just the same call over and over again for each input.
|
update: addressed inline comments @morcos Yes, if our estimation code has an answer for 25, but not for 1, it would be confusing to tell the user in order to get 'fast' confirmation, you need to pay a lower fee than for 'normal'. |
|
Looks good to me! A question: "Send as free transaction if possible" - doesn't this toggle apply to all three settings equally, Recommended Minimum and Custom? That feels more logical to me. Some minor nits:
|
|
Grammatical corrections:
What is the difference between "Send as free transaction if possible" and "Minimum"? They sound like the same thing to me... |
|
@laanwj Yes, I had that checkbox apply to all three, but then I realized that this is not possible with the current logic of CreateTransaction. Its not a problem, but it requires CreateTransaction to be redesigned again. If you want that, I can do this, I just did not, to avoid critical changes. In the current master, if you pay a voluntary fee, you never send free. @luke-jr I never heard gratis transaction before in bitcoinland. According to the dictionary, free can be used as synonym. I dont see a problem with it. |
|
@cozz "free transaction relay" has been used for nodes that don't perform IsStandard filtering of transactions. It's better to use unambiguous terms, even if you accept the newer redefinition of "free" to include "gratis". If gratis is possible, it would always be the minimum. So what is the difference? |
|
"gratis" is an obscure word. How about "zero-fee". |
|
So lets replace "free" with "zero-fee" then. About the other thing, I can see that "minimum" may seem to imply "zero-fee", but its not. If we move the free-checkbox below, to apply to all three, this should become clearer. |
|
So what would "Minimum" do if you uncheck it? O.o |
|
minimum is a radio button. you must select 1 of 3. |
|
I mean selected Minimum and unchecked "zero-fee" - it doesn't make sense... if zero-fee is possible, it's always the minimum. |
|
Personally I think we're not addressing the biggest issue. Given that you are selecting the fee rate you want to pay before your inputs are selected (unless you're using coin control) you have very little control on the total fee you're paying. I think before we could have a really effective GUI for paying fees we'd have to address #4082. There is a complicated interaction between the inputs you use, the speed you desire for confirmation, and the fee you're paying. It makes a lot of sense to use your small inputs when you don't care about speed (b/c you have to use a lot of inputs for the same value tx, but get to pay less per input used) and to use your big inputs when you do care about the speed of your tx (since each input used has a high fee per / kb cost). This is without even taking into account coin age to use your old coins to take advantage of priority when it saves you the most in fees. But I don't know if optimizing all of this falls under the purview of the core wallet. |
|
@morcos yes, I agree to what you say, there is a lot of room for improvement. The point is that the current fee/priority selection, selects something thats confirms. And floats. Thats good. Even if way too high in some edge cases, its better than the hardcoded fee/priority values we had before. And people who know better than our smartfee, they can use coin control/custom fee. Of course its bad, that sophisticated users may end up paying less fee than others in bitcoinland, but I guess there is not much we can do here in regards to the 0.10 release. |
|
update:
Added as separate commit for easier review. Should be squashed before merge. There were less changes to CreateTransaction required than
|
|
When this says "to be confirmed within 25 blocks", what metric is it using for confirmation? The usual 6 blocks? If it's just 25 blocks to be mined, perhaps "to begin confirmation within 25 blocks" would be better. It might be desirable to see the Smart-Fee estimate for mining/confirmation time even for Custom fee selection (move it outside the options?). What does the new "Minimize" button do, since we still have the checkbox under Custom? Does this under any conditions allow sending with a lower fee than the "minimum"? We still don't have a mechanism for getting "unstuck" :/ |
|
I think having the "zero-fee" option apply to both is confusing, because I think it still uses the txConfirmTarget you set in the top section. I like the idea of having a top section which uses smart fee-logic (and has the zero-fee checkbox) and a bottom custom section (which either has the min-fee checkbox or makes it easy to set the min fee as the custom fee). |
|
@luke-jr Our estimation API is supposed to take a target (like 11 blocks) and returns the fee, not the other way round. Also the estimations are not really good at the moment. As morcos said, even if you just pay the minfee, you get almost always included after 8 blocks. But our estimation code tells you 25 blocks, for a even higher fee. So I'll leave that for the future, once our estimations are better. Minimize button minimizes, see initial screenshots, the button is not new. Its not possible to send a non-sense (like 1 satoshi): 0 < fee < minRelayFee @morcos yes, but txConfirmTarget is set to 25 internally for custom. I think thats fine. So you can choose to send a free transaction for both, recommended and custom. |
|
ok, changed to "Estimated to begin confirmation within x block(s)." |
|
|
|
squashed |
src/wallet.cpp
Outdated
There was a problem hiding this comment.
I thought you moved this change to #5209 so that it has its own discussion. You should remove it here.
|
@laanwj ok, I just rebased and skipped that commit |
c1c9d5b [Qt] Add Smartfee to GUI (Cozz Lovan) e7876b2 [Wallet] Prevent user from paying a non-sense fee (Cozz Lovan) ed3e5e4 [Wallet] Add global boolean whether to pay at least the custom fee (default=true) (Cozz Lovan) 0ed9675 [Wallet] Add global boolean whether to send free transactions (default=true) (Cozz Lovan)
|
This doesn't seem to work well with #5159 currently... |
|
The GUI slider just shows what estimatefee returns. You can test the estimatefee rpc-call to check. |
8 participants
By default a minimized label is shown. It updates automatically, if smartfee has been chosen.
Smartfee not initialized -> pay minfee
Smartfee initialized
Fee removed from optionsdialog