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

luke-jr commented at 3:49 am on March 31, 2020: member

Rebase of #12911

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}

fanquake added the label RPC/REST/ZMQ on Mar 31, 2020

DrahtBot commented at 8:18 am on March 31, 2020: 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:

#20581 (Don’t make “in” parameters look like “out”/“in-out” parameters: pass by ref to const instead of ref to non-const by practicalswift)
#19426 (refactor: Change * to & in MutableTransactionSignatureCreator by MarcoFalke)

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.

jonasschnelli commented at 8:29 am on March 31, 2020: contributor

Concept ACK. Light code review ACK 9a5468e6d9e851503bfda9ffa9f9d0995291e890. I found it a bit confusing that you pass the optional CAmount sometimes as reference sometimes a pointer.

in test/functional/feature_segwit.py:299 in 9a5468e6d9 outdated

291+        for o in tx["vout"]:
292+            if o["scriptPubKey"]["addresses"][0] == addr:
293+                n = o["n"]
294+                value = Decimal(o["value"])
295+                break
296+        assert n > -1 # failure means we could not find the address in the outputs despite sending to it

MarcoFalke commented at 1:41 am on April 3, 2020:

This can be a one-liner: output = next(o for o in tx["vout"] if o["scriptPubKey"]["addresses"][0] == addr)

luke-jr commented at 0:57 am on April 23, 2020:

Maybe, but that’s a bit unreadable?

in test/functional/feature_segwit.py:285 in 9a5468e6d9 outdated

279@@ -279,6 +280,34 @@ def run_test(self):
280         # Mine a block to clear the gbt cache again.
281         self.nodes[0].generate(1)
282 
283+        self.log.info("Signing with all-segwit inputs reveals fee rate")
284+        # Test that signing a tx with all-segwit inputs returns the fee
285+        # in the results

MarcoFalke commented at 1:41 am on April 3, 2020:

Can remove the comment which is redundant to the log statement?

in test/functional/feature_segwit.py:297 in 9a5468e6d9 outdated

292+            if o["scriptPubKey"]["addresses"][0] == addr:
293+                n = o["n"]
294+                value = Decimal(o["value"])
295+                break
296+        assert n > -1 # failure means we could not find the address in the outputs despite sending to it
297+        assert_approx(value, 1.0, 0.01) # failure means we got an unexpected amount of coins, despite trying to send 1

MarcoFalke commented at 1:42 am on April 3, 2020:

This should assert the precise amount?

in test/functional/feature_segwit.py:309 in 9a5468e6d9 outdated

304+        txsize = self.nodes[0].decoderawtransaction(signed['hex'])['vsize']
305+        exp_feerate = 1000 * fee / Decimal(txsize)
306+        assert_approx(signed["feerate"], exp_feerate, Decimal("0.00000010"))
307+        # discrepancy = 100000000 * (exp_feerate - signed["feerate"])
308+        # assert -10 < discrepancy < 10
309+        assert Decimal(signed["fee"]) == fee

MarcoFalke commented at 1:46 am on April 3, 2020:

0        assert_equal(Decimal(signed["fee"]), fee)

To get debug information on failure

MarcoFalke commented at 1:46 am on April 3, 2020: member

Concept ACK

luke-jr force-pushed on Apr 23, 2020

DrahtBot added the label Needs rebase on Apr 27, 2020

luke-jr force-pushed on Aug 19, 2020

DrahtBot removed the label Needs rebase on Aug 20, 2020

script: Return total sum of input amounts from SignTransaction when available 6bd00d0854

luke-jr force-pushed on Aug 20, 2020

luke-jr commented at 1:45 am on August 20, 2020: member

Rebased again

in src/rpc/rawtransaction.cpp:747 in a1da1f98d4 outdated

743@@ -744,6 +744,8 @@ static UniValue signrawtransactionwithkey(const JSONRPCRequest& request)
744                     {
745                         {RPCResult::Type::STR_HEX, "hex", "The hex-encoded raw transaction with signature(s)"},
746                         {RPCResult::Type::BOOL, "complete", "If the transaction has a complete set of signatures"},
747+                        {RPCResult::Type::NUM, "fee", /* optional */ true, "The fee (input amounts minus output amounts), if known"},

MarcoFalke commented at 9:17 am on August 23, 2020:

The type is STR_AMOUNT, not NUM

luke-jr commented at 9:40 pm on October 30, 2020:

Fixed

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

126ed4db59

wallet: Display fee rate in signrawtransaction* 54aa714cd9

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

doc: Update release notes about fee entry ddb8d028f2

luke-jr force-pushed on Oct 30, 2020

DrahtBot commented at 8:09 pm on December 6, 2020: member

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

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot added the label Needs rebase on Dec 6, 2020

in src/rpc/rawtransaction_util.cpp:277 in ddb8d028f2

273@@ -274,12 +274,13 @@ void SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
274 
275     // Script verification errors
276     std::map<int, std::string> input_errors;
277+    Optional<CAmount> inputs_amount_sum;

fanquake commented at 2:02 am on March 15, 2021:

Please use std::optional in new code.

luke-jr commented at 0:50 am on July 7, 2021: member

Closing due to lack of interest.

luke-jr closed this on Jul 7, 2021

DrahtBot locked this on Aug 16, 2022

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

Conflicts