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

wallet: Show fee in results for signrawtransaction* for segwit inputs #12911

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@kallewoof
Copy link
Member

kallewoof commented Apr 8, 2018

This adds a "fee" field to the resulting JSON for signrawtransaction* so a user can double check the fee they're paying before sending a transaction. The field is only shown in cases where the input amounts are all known ⇔ are all segwit inputs.

$ ./bitcoin-cli -regtest signrawtransactionwithwallet 0200000001901c16d5ac11824ca64c9c9dbe925c83fc1af9872bb23bbc9c6cd25419e2f69c0000000000feffffff0210b2d0df0000000017a914d9214ccd777e5cce540d38c3466c2cb5545339c5874031354a0000000017a914bee793caf793996dc9f617a5ad04ec2b87c6f9538700000000
{
  "hex": "0200000001901c16d5ac11824ca64c9c9dbe925c83fc1af9872bb23bbc9c6cd25419e2f69c00000000494830450221008fd0d0cfd16a06f282e720129351f2756f416b916f7a0a3be8d5db0c7db107af022028dafae6ec7d30882efe101c16b5f3893254bf5385664eddadf0d3f6e479381c01feffffff0210b2d0df0000000017a914d9214ccd777e5cce540d38c3466c2cb5545339c5874031354a0000000017a914bee793caf793996dc9f617a5ad04ec2b87c6f9538700000000",
  "complete": true,
  "fee": 0.00003760,
  "feerate": 0.00020000
}
  • Re-write tests.
@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Apr 8, 2018

Concept NACK. amount is not guaranteed to be correct (default 0) for non-segwit inputs if users are specifying their own UTXOs. Since the input amount is not required for non-segwit inputs, it will be 0 and the fee will be negative. Furthermore, even if complete=true, we do not necessarily have all of the UTXOs that are being spent from so even then we can't accurately calculate the input amount.

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Apr 8, 2018

@achow101 Are you against even if it only displays fee when all input amounts are known? (I.e. add check instead of using fComplete)

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Apr 8, 2018

I'm not against it if the input amounts are known.

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch Apr 8, 2018

@kallewoof kallewoof changed the title wallet: Show fee in results for signrawtransaction* when complete=true wallet: Show fee in results for signrawtransaction* when known Apr 8, 2018

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Apr 8, 2018

@achow101 I believe I cover all cases of it being known/unknown with the updated code.

@promag

This comment has been minimized.

Copy link
Member

promag commented Apr 8, 2018

Concept ACK.

Things to do:

  • update to help message referring new response key;
  • update existing tests or add new ones;
  • release note.

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch 2 times, most recently Apr 8, 2018

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Apr 8, 2018

@promag Thanks, done. :)

src/rpc/rawtransaction.cpp Outdated
@@ -985,6 +997,7 @@ UniValue signrawtransaction(const JSONRPCRequest& request)
"{\n"
" \"hex\" : \"value\", (string) The hex-encoded raw transaction with signature(s)\n"
" \"complete\" : true|false, (boolean) If the transaction has a complete set of signatures\n"
" \"fee\" : n, (numeric) The fee (input amounts minus output amounts), if known\n"

This comment has been minimized.

@promag

promag Apr 9, 2018

Member

Wrong alignment.

This comment has been minimized.

@kallewoof

kallewoof Apr 9, 2018

Author Member

Fixed

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch Apr 9, 2018

@promag

This comment has been minimized.

Copy link
Member

promag commented Apr 9, 2018

LGTM, will test later.

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Apr 9, 2018

I pushed another commit (a35bc32) which also shows fee rate (both in btc/kb and sat/b), as someone requested it. Will squash unless people speak against the idea.

@promag

This comment has been minimized.

Copy link
Member

promag commented Apr 9, 2018

Fee rate is something the user can easily compute with the fee and hex size. Not sure about that.

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Apr 9, 2018

@promag I agree it can be easily derived, but not sure there's any drawbacks to printing it either...

@promag

This comment has been minimized.

Copy link
Member

promag commented Apr 9, 2018

@kallewoof maybe just display in one unit only? And if it goes forward maybe add the same fee rate to fundrawtransaction?

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch 2 times, most recently Apr 10, 2018

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Apr 10, 2018

@promag Done.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Apr 10, 2018

I'm not sure this is the right place. Hopefully you know the fee/feerate before deciding to sign something, and the fact that signrawtransaction happens to have all the necessary information (sometimes) is more an implementation accident than inherent to its function.

Is there a pressing use case? Otherwise I would argue to instead move PSBT forward, and add an RPC to analyse a PSBT which can give this can of information, independent from the signing logic.

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Apr 11, 2018

@sipa That is exactly the problem: I misread the input value and ended up throwing ~$10 away, which triggered my creating this PR.

Note that the feerate is also listed in fundrawtransaction -- adding it to signrawtransaction seems like a natural complement.

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch May 10, 2018

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Jun 27, 2018

I think an alternative fix to this problem, to make sure the user don't shoot himself in the foot is to have a signrawtransactionwithwallet parameter which cross check that what is sign is what is expected.

For example signrawtransactionwithwallet hex=<hex> expect={"destinations": [ { "Address": "blah", "Value": 1000 }], "maxFee": 100 }

This would actually simplify greatly calling code which right now need to verify such information manually before calling sign.

The solution of this PR is useful only when you are debugging stuff manually. An "expect" parameter on the other hand, would solve your issue, but also simplify the code because the callers would not need to verify by themselves the transaction with some custom code.

I am not against this PR, just I think it has limited scope.

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Jun 27, 2018

Talked with @kallewoof :

An easy alternative I was pointing out was also just a simple expectedMaxFee= parameter would be widely useful.

However, he pointed me out that fundrawtransaction actually show the fees. I think it makes sense to support on signtransaction if fundrawtransaction do it already.

Concept ACK.

@kallewoof

This comment has been minimized.

Copy link
Member Author

kallewoof commented Jun 27, 2018

A way to verify fees right now is to use fundrawtransaction on the resulting transaction:

  1. createrawtransaction with inputs and outputs
  2. Sign it using signrawtransaction
  3. Decode the results, using vsize as basis for fee
  4. Adjust outputs, reducing by the chosen fee
  5. Call fundrawtransaction on the result with option {\"feeRate\":CHOSENFEERATE}
  6. If returned hex is the same as the provided hex, you are good to go. As a bonus, fundrawtransaction also shows the fee rate (although this is before signing it, I guess).
  7. Sign the provided tx, and send it

With this PR:
1-4 as above
5. Sign it using signrawtransaction
6. If resulting fee rate is correct, send it.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jul 22, 2018

The last travis run for this pull request was 73 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Sep 21, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15639 (bitcoin-wallet tool: Drop libbitcoin_server.a dependency by ryanofsky)
  • #15638 (Move-only: Pull wallet code out of libbitcoin_server by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

for (unsigned int i = 0; i < mtx.vin.size(); i++) {
CTxIn& txin = mtx.vin[i];
const Coin& coin = view.AccessCoin(txin.prevout);
if (coin.IsSpent()) {
known_inputs = false;

This comment has been minimized.

@sipa

sipa Nov 14, 2018

Member

You should also set known_input = false when sigdata.witness below is false (unless it's known that every input is spending a witness output, there is no guarantee that providing incorrect amounts will result in invalid signatures).

This comment has been minimized.

@kallewoof

kallewoof Nov 15, 2018

Author Member

I'm not sure what you mean by "providing incorrect amounts". This is directly from the UTXO set of the node itself. When would it tell itself incorrect amounts?

This comment has been minimized.

@sipa

sipa Nov 15, 2018

Member

view is not the node's UTXO set, but a local variable which holds a few cached UTXO entries. Those can come from the UTXO set or mempool, but could also have been provided by the user (see the prevTxsUnival input argument to this function).

This comment has been minimized.

@kallewoof

kallewoof Nov 16, 2018

Author Member

Makes sense, thanks. I updated the code to only work with segwit inputs (i.e. added the change you requested).

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch Nov 16, 2018

@kallewoof kallewoof changed the title wallet: Show fee in results for signrawtransaction* when known wallet: Show fee in results for signrawtransaction* for segwit inputs Nov 16, 2018

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch to 7f60a26 Jan 23, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 23, 2019

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch from 6f2bb1c to fd23f91 Jan 24, 2019

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch from fd23f91 to 15dee4c Mar 6, 2019

@DrahtBot DrahtBot removed the Needs rebase label Mar 6, 2019

MarcoFalke added a commit that referenced this pull request Mar 18, 2019

Merge #13541: wallet/rpc: sendrawtransaction maxfeerate
7abd2e6 wallet/rpc: add maxfeerate parameter to testmempoolaccept (Karl-Johan Alm)
6c0a6f7 wallet/rpc: add maxfeerate parameter to sendrawtransaction (Karl-Johan Alm)
e5efacb test: Refactor vout fetches in rpc_rawtransaction (Karl-Johan Alm)

Pull request description:

  This adds a new `maxfeerate` parameter to `sendrawtransaction` which forces the node to reject a transaction whose feerate is above the given fee rate.

  This is a safety harness from shooting yourself in the foot and accidentally overpaying fees.

  See also #12911.

Tree-SHA512: efa50134a7c17c9330cfdfd48ba400e095c0a419cc45e630618d8b44929c25d780d1bb2710c1fbbb6e687eca373505b0338cdaa7f2ff4ca22636d84c31557a2e

kallewoof added some commits Apr 8, 2018

wallet: Show fee in results for signrawtransaction* when known
The fee is considered known when all inputs are segwit inputs (which means amounts are enforced/known)..

@kallewoof kallewoof force-pushed the kallewoof:sign-show-fees branch from 15dee4c to 57676cc Mar 23, 2019

@DrahtBot DrahtBot removed the Needs rebase label Mar 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.