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

kallewoof · 2018-04-08T02:55:01Z

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 · 2018-04-08T04:06:32Z

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 · 2018-04-08T04:33:10Z

@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 · 2018-04-08T07:20:15Z

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

kallewoof · 2018-04-08T21:50:18Z

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

promag · 2018-04-08T22:25:00Z

Concept ACK.

Things to do:

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

kallewoof · 2018-04-08T23:54:28Z

@promag Thanks, done. :)

src/rpc/rawtransaction.cpp

promag · 2018-04-09T00:04:11Z

LGTM, will test later.

kallewoof · 2018-04-09T05:44:18Z

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 · 2018-04-09T07:15:38Z

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

kallewoof · 2018-04-09T07:21:46Z

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

promag · 2018-04-09T11:17:43Z

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

kallewoof · 2018-04-10T02:16:34Z

@promag Done.

sipa · 2018-04-10T16:39:54Z

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 · 2018-04-11T04:42:02Z

@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.

NicolasDorier · 2018-06-27T06:50:50Z

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 · 2018-06-27T07:07:35Z

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 · 2018-06-27T07:27:26Z

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

createrawtransaction with inputs and outputs
Sign it using signrawtransaction
Decode the results, using vsize as basis for fee
Adjust outputs, reducing by the chosen fee
Call fundrawtransaction on the result with option {\"feeRate\":CHOSENFEERATE}
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).
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 · 2018-07-22T23:50:08Z

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.

The fee is considered known when all inputs are segwit inputs (which means amounts are enforced/known)..

DrahtBot · 2020-03-09T21:21:26Z

🐙 This pull request conflicts with the target branch and needs rebase.

kallewoof · 2020-03-10T01:52:46Z

This used to be a fairly straightforward patch, but after #18115 this becomes way too complex to warrant the change.

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 bitcoin#12911. Tree-SHA512: efa50134a7c17c9330cfdfd48ba400e095c0a419cc45e630618d8b44929c25d780d1bb2710c1fbbb6e687eca373505b0338cdaa7f2ff4ca22636d84c31557a2e

maflcko added the RPC/REST/ZMQ label Apr 8, 2018

kallewoof force-pushed the sign-show-fees branch from 6e0473a to be60ddc Compare April 8, 2018 21:49

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 force-pushed the sign-show-fees branch 2 times, most recently from 02b37ec to f2c41c7 Compare April 8, 2018 23:53

promag reviewed Apr 9, 2018

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

kallewoof force-pushed the sign-show-fees branch from f2c41c7 to 9122a75 Compare April 9, 2018 00:03

kallewoof force-pushed the sign-show-fees branch 2 times, most recently from d5257f2 to f973ce1 Compare April 10, 2018 02:16

kallewoof force-pushed the sign-show-fees branch from f973ce1 to b7159aa Compare May 10, 2018 06:44

kallewoof mentioned this pull request Jun 27, 2018

wallet/rpc: sendrawtransaction maxfeerate #13541

Merged

DrahtBot closed this Jul 22, 2018

kallewoof force-pushed the sign-show-fees branch from bba2e57 to 74dba91 Compare January 4, 2020 02:22

fanquake removed the Needs rebase label Jan 4, 2020

kallewoof force-pushed the sign-show-fees branch 2 times, most recently from 871623e to 345f8f9 Compare January 4, 2020 03:01

This was referenced Feb 11, 2020

wallet: Pass in transactions and messages for signing instead of exporting the private keys #18115

Merged

rpc: Auto-format RPCResult #17809

Merged

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Merged

This was referenced Feb 21, 2020

External signer support - Wallet Box edition #16546

Merged

UI external signer support (e.g. hardware wallet) #16549

Closed

refactor: consolidate sendmany and sendtoaddress code #18202

Merged

DrahtBot added the Needs rebase label Mar 4, 2020

kallewoof and others added 4 commits March 6, 2020 15:51

wallet: Show fee in results for signrawtransaction* when known

ce41dec

The fee is considered known when all inputs are segwit inputs (which means amounts are enforced/known)..

doc: Update release notes about fee entry

33e5b88

wallet: Display fee rate in signrawtransaction*

476933f

test: add test to segwit tests for fee rate when signing raw tx

47b2ba2

kallewoof force-pushed the sign-show-fees branch from 345f8f9 to 47b2ba2 Compare March 6, 2020 07:13

DrahtBot removed the Needs rebase label Mar 6, 2020

DrahtBot added the Needs rebase label Mar 9, 2020

kallewoof closed this Mar 10, 2020

kallewoof deleted the sign-show-fees branch March 10, 2020 01:52

luke-jr mentioned this pull request Mar 31, 2020

RPC: Show fee in results for signrawtransaction* for segwit inputs #18479

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Conversation

kallewoof commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Apr 8, 2018

Uh oh!

achow101 commented Apr 8, 2018

Uh oh!

kallewoof commented Apr 8, 2018

Uh oh!

promag commented Apr 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Apr 8, 2018

Uh oh!

Uh oh!

promag commented Apr 9, 2018

Uh oh!

kallewoof commented Apr 9, 2018

Uh oh!

promag commented Apr 9, 2018

Uh oh!

kallewoof commented Apr 9, 2018

Uh oh!

promag commented Apr 9, 2018

Uh oh!

kallewoof commented Apr 10, 2018

Uh oh!

sipa commented Apr 10, 2018

Uh oh!

kallewoof commented Apr 11, 2018

Uh oh!

NicolasDorier commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Jun 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Jun 27, 2018

Uh oh!

DrahtBot commented Jul 22, 2018

Uh oh!

DrahtBot commented Mar 9, 2020

Uh oh!

kallewoof commented Mar 10, 2020

Uh oh!

Uh oh!

kallewoof commented Apr 8, 2018 •

edited

Loading

achow101 commented Apr 8, 2018 •

edited

Loading

promag commented Apr 8, 2018 •

edited

Loading

NicolasDorier commented Jun 27, 2018 •

edited

Loading

NicolasDorier commented Jun 27, 2018 •

edited

Loading