Skip to content
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

Add transaction fee estimation and show fee in view. #44

Closed
wants to merge 18 commits into from

Conversation

jrick
Copy link
Member

@jrick jrick commented Feb 24, 2016

This isn't a strict port of the fee estimation from btcwallet as #10 would indicate. It's actually better, since the actual fee/kB of the estimated transaction size is used to determine whether enough fee was added, instead of incrementing the fee by the relay fee for every full kB boundary.

@jrick jrick force-pushed the feeestimation branch 3 times, most recently from 826a129 to 67e7966 Compare February 24, 2016 16:44
@jrick
Copy link
Member Author

jrick commented Feb 24, 2016

There is also a small display issue in the create transaction view that is introduced by this change. If a transaction can not be constructed (either because one of the outputs is invalid or there is not enough previous output value to spend), the Transaction Fee and Remaining Balance labels will show just "BTC" with no amount, instead of hiding those values completely.

@jrick
Copy link
Member Author

jrick commented Feb 24, 2016

Fee estimation is also slightly off when comparing the actual fee/kB (I just sent a transaction with an actual fee/kB of 991 satoshis, when it should have been >=1000). This is because the transaction overhead constants are best case scenarios. These have to be changed to be worst case, since this fee estimation algorithm does not do the iterative fee incrementing like btcwallet does.

@jrick
Copy link
Member Author

jrick commented Feb 24, 2016

Fixed the fee calculation to use a better (worst case) serialize size estimate. Transactions are now created with actual fees/kB which are just over the relay fee.

@jrick jrick force-pushed the feeestimation branch 2 times, most recently from 702aaa0 to 3610ce0 Compare February 25, 2016 21:15
@jrick
Copy link
Member Author

jrick commented Feb 25, 2016

EstimateSerializeSize is wrong. The output count needs to be increment if a change output is being added.

@jrick
Copy link
Member Author

jrick commented Feb 26, 2016

I'm currently moving all of the transaction construction code from the AccountViewModel (in RecalculatePendingTransaction) into a static, pure function in the Bitcoin project. Because that construction code requires a way to fetch inputs for some target amount, I am passing in that functionality using a delegate rather than depending on the RPC client directly. The advantages of this design are testability (testing this code in the view model would be tricky, due to other issues with the view models depending on classes like MessageBox), flexibility (any input fetching code can be substituted here, not just rpc results), and modularity (the code could be imported by a command line tool, for example, and be able to construct transactions with the same smart fee estimation).

jrick added 3 commits March 3, 2016 14:03
This updates the protocol buffer specifier to the latest used by
dcrwallet, and bumps the required RPC server version for the API
breaks.
@jrick
Copy link
Member Author

jrick commented Apr 1, 2016

In retrospect, I'm going to modify the RPC API to allow incremental utxo selection all under a single database transaction, using gRPC streams. This closely models the new txauthor.InputSource btcwallet type, and it should fit in very well with this Paymetheus code, since this Paymetheus PR was the inspiration for that package.

jrick and others added 10 commits May 11, 2016 09:56
Remaining issues should be fixed on feature branches and submitted
with PRs.
Simplify the UI by always showing the "generate address" button.
Click again to generate a new address, replacing the old.

Improve the xaml so it doesn't look like it got generated by 15
different automated tools.

Make the address manually selectable so it can be copied, since the
label under the address is lying and it is not automatically copied to
clipboard yet.

Fixes btcsuite#13.
While here, improve error messages for when an incorrect public
passphrase is used during wallet opening.

Fixes #8.
read from ini

add public default options to the App

pass defaults down

set defaults if available

for some reason config options are allowed in multiple sections; someone should get a severe larting for that

use c# magic instead of testing for null

moar string.IsNullOrEmpty

pre select section instead of using a string; from jrick

refactor data -> ini

save defaults

do not mess with cert path
Clean up default discovery and remove a maze of try catch statements.

Fixes btcsuite#42.
This change requires a newer version of the wallet's RPC API.

Fixes #10.
Fixes btcsuite#11.
@jrick
Copy link
Member Author

jrick commented Jul 1, 2016

Closing, overwrote this branch with the decred fee estimation work by accident.

@jrick jrick closed this Jul 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants