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

kallewoof commented at 2:55 am on April 8, 2018: member

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.

0$ ./bitcoin-cli -regtest signrawtransactionwithwallet 0200000001901c16d5ac11824ca64c9c9dbe925c83fc1af9872bb23bbc9c6cd25419e2f69c0000000000feffffff0210b2d0df0000000017a914d9214ccd777e5cce540d38c3466c2cb5545339c5874031354a0000000017a914bee793caf793996dc9f617a5ad04ec2b87c6f9538700000000
1{
2  "hex": "0200000001901c16d5ac11824ca64c9c9dbe925c83fc1af9872bb23bbc9c6cd25419e2f69c00000000494830450221008fd0d0cfd16a06f282e720129351f2756f416b916f7a0a3be8d5db0c7db107af022028dafae6ec7d30882efe101c16b5f3893254bf5385664eddadf0d3f6e479381c01feffffff0210b2d0df0000000017a914d9214ccd777e5cce540d38c3466c2cb5545339c5874031354a0000000017a914bee793caf793996dc9f617a5ad04ec2b87c6f9538700000000",
3  "complete": true,
4  "fee": 0.00003760,
5  "feerate": 0.00020000
6}

Re-write tests.

MarcoFalke added the label RPC/REST/ZMQ on Apr 8, 2018

achow101 commented at 4:06 am on April 8, 2018: member

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 commented at 4:33 am on April 8, 2018: member

@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 commented at 7:20 am on April 8, 2018: member

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

kallewoof force-pushed on Apr 8, 2018

kallewoof renamed this:
~~wallet: Show fee in results for signrawtransaction* when complete=true~~
wallet: Show fee in results for signrawtransaction* when known
on Apr 8, 2018

kallewoof commented at 9:50 pm on April 8, 2018: member

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

promag commented at 10:25 pm on April 8, 2018: member

Concept ACK.

Things to do:

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

kallewoof force-pushed on Apr 8, 2018

kallewoof commented at 11:54 pm on April 8, 2018: member

@promag Thanks, done. :)

in src/rpc/rawtransaction.cpp:1000 in f2c41c7ca4 outdated

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

promag commented at 0:00 am on April 9, 2018:

Wrong alignment.

kallewoof commented at 0:03 am on April 9, 2018:

Fixed

kallewoof force-pushed on Apr 9, 2018

promag commented at 0:04 am on April 9, 2018: member

LGTM, will test later.

kallewoof commented at 5:44 am on April 9, 2018: member

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 commented at 7:15 am on April 9, 2018: member

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

kallewoof commented at 7:21 am on April 9, 2018: member

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

promag commented at 11:17 am on April 9, 2018: member

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

kallewoof force-pushed on Apr 10, 2018

kallewoof commented at 2:16 am on April 10, 2018: member

@promag Done.

sipa commented at 4:39 pm on April 10, 2018: member

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 commented at 4:42 am on April 11, 2018: member

@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 force-pushed on May 10, 2018

NicolasDorier commented at 6:50 am on June 27, 2018: contributor

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 commented at 7:07 am on June 27, 2018: contributor

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 commented at 7:27 am on June 27, 2018: member

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 closed this on Jul 22, 2018

DrahtBot commented at 11:50 pm on July 22, 2018: member

DrahtBot reopened this on Jul 22, 2018

DrahtBot added the label Needs rebase on Sep 7, 2018

kallewoof force-pushed on Sep 10, 2018

DrahtBot removed the label Needs rebase on Sep 10, 2018

DrahtBot commented at 1:21 pm on September 21, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18115 (wallet: Pass in transactions and messages for signing instead of exporting the private keys by achow101)
#16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)
#16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)

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.

in src/rpc/rawtransaction.cpp:902 in f9a726a13a outdated

854+    bool known_inputs = true;
855     for (unsigned int i = 0; i < mtx.vin.size(); i++) {
856         CTxIn& txin = mtx.vin[i];
857         const Coin& coin = view.AccessCoin(txin.prevout);
858         if (coin.IsSpent()) {
859+            known_inputs = false;

sipa commented at 11:12 pm on November 14, 2018:

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

kallewoof commented at 5:06 am on November 15, 2018:

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?

sipa commented at 7:38 pm on November 15, 2018:

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

kallewoof commented at 1:34 am on November 16, 2018:

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

kallewoof force-pushed on Nov 16, 2018

kallewoof renamed this:
~~wallet: Show fee in results for signrawtransaction* when known~~
wallet: Show fee in results for signrawtransaction* for segwit inputs
on Nov 16, 2018

DrahtBot added the label Needs rebase on Jan 21, 2019

kallewoof force-pushed on Jan 23, 2019

DrahtBot removed the label Needs rebase on Jan 23, 2019

kallewoof force-pushed on Jan 24, 2019

DrahtBot added the label Needs rebase on Mar 5, 2019

kallewoof force-pushed on Mar 6, 2019

DrahtBot removed the label Needs rebase on Mar 6, 2019

MarcoFalke referenced this in commit c033c4b5ce on Mar 18, 2019

DrahtBot added the label Needs rebase on Mar 21, 2019

kallewoof force-pushed on Mar 23, 2019

DrahtBot removed the label Needs rebase on Mar 23, 2019

NicolasDorier commented at 6:14 am on April 2, 2019: contributor

Code review ACK. This is very useful.

DrahtBot added the label Needs rebase on Apr 10, 2019

kallewoof force-pushed on Apr 11, 2019

DrahtBot removed the label Needs rebase on Apr 11, 2019

in src/rpc/rawtransaction_util.cpp:300 in 931d1c58ef outdated

296+    if (known_inputs) {
297+        for (const CTxOut& txout : mtx.vout) {
298+            inout_amount -= txout.nValue;
299+        }
300+        result.pushKV("fee", ValueFromAmount(inout_amount));
301+        result.pushKV("feerate", ValueFromAmount(inout_amount*1000/(hex.length()>>1)));

NicolasDorier commented at 3:37 am on April 11, 2019:

should use the virtual size, not the tx size

kallewoof commented at 5:25 am on April 11, 2019:

@NicolasDorier I changed to use vsize:

0{
1  "hex": "0200000000010104f1e2bcda677ec9c76c4201ce5ab1dec28b2127fa5cc1a0555401887ed1a49b00000000171600143f7d5c060647cf38a896d2b0ad77004526eead1bfeffffff02109242060100000017a914b7036cfe5ee607f473630a51911f60f99c85260f870065cd1d0000000017a914bd8a3d83ca6e3cfa74386c0e61374a62e7eba4178702473044022058bf8809ebd3816a8270d0bd740a26f962d8deab02a7a46c008b4de7525bb15c022005dd8381cd9cd1eb48be647ccdb098d7e57a6d1fdc354911348427174e4ed43a012102cd5dd2f7d61b58a8f0a319dbd89a35be73937003e6e88313cb04e7b4bfd56fc400000000",
2  "complete": true,
3  "fee": 0.00003320,
4  "feerate": 0.00020000
5}

with

0  "vsize": 166,

gives

0> vsize=166
1> fee=0.00003320
2> 1000*(fee/vsize)
3[1] 2e-04

(2e-04 = 0.00020000)

kallewoof force-pushed on Apr 11, 2019

DrahtBot added the label Needs rebase on Apr 11, 2019

kallewoof force-pushed on Apr 11, 2019

kallewoof commented at 2:01 am on April 15, 2019: member

@MarcoFalke DrahtBot is not realizing this PR no longer needs rebasing.

DrahtBot removed the label Needs rebase on Apr 15, 2019

DrahtBot added the label Needs rebase on Nov 22, 2019

kallewoof force-pushed on Jan 4, 2020

fanquake removed the label Needs rebase on Jan 4, 2020

kallewoof force-pushed on Jan 4, 2020

DrahtBot added the label Needs rebase on Mar 4, 2020

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

ce41dec115

doc: Update release notes about fee entry 33e5b88d22

wallet: Display fee rate in signrawtransaction* 476933ffe2

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

kallewoof force-pushed on Mar 6, 2020

DrahtBot removed the label Needs rebase on Mar 6, 2020

DrahtBot commented at 9:21 pm on March 9, 2020: member

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

DrahtBot added the label Needs rebase on Mar 9, 2020

kallewoof commented at 1:52 am on March 10, 2020: member

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

kallewoof closed this on Mar 10, 2020

kallewoof deleted the branch on Mar 10, 2020

vijaydasmp referenced this in commit 7a9af07b34 on Nov 15, 2021

vijaydasmp referenced this in commit 99d82a8149 on Nov 16, 2021

vijaydasmp referenced this in commit d81cf3c0b7 on Dec 5, 2021

vijaydasmp referenced this in commit e17ac3c822 on Dec 5, 2021

vijaydasmp referenced this in commit 232eeb1ce0 on Dec 6, 2021

vijaydasmp referenced this in commit 82a9388106 on Dec 6, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts