Add multiple options to fundrawtransaction #7518

promag · 2016-02-11T17:01:55Z

This PR allows to specify an address to receive the change of the fund. Currently fundrawtransaction accepts a boolean for the second parameter. With this change the second parameter can be either a boolean or a JSON object with the following optional keys: includeWatching, changeAddress and changePosition.

promag · 2016-02-11T17:02:39Z

src/wallet/rpcwallet.cpp

@@ -2468,7 +2468,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
                            + HelpExampleCli("sendrawtransaction", "\"signedtransactionhex\"")
                            );

-    RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VBOOL));
+    // RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VBOOL));


Mixed feelings about how to do this since the second argument can be one of two types.

IIRC you can just leave off the second type for RPCTypeCheck and it will skip it. Then do the check by hand.

luke-jr · 2016-02-11T17:15:22Z

You need to update the help. I suggest replacing the Boolean with the Object entirely, and just supporting the boolean in code as a backward compatibility thing.

promag · 2016-02-11T18:34:30Z

@luke-jr done.

luke-jr · 2016-02-11T18:39:14Z

src/wallet/rpcwallet.cpp

@@ -2468,7 +2472,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
                            + HelpExampleCli("sendrawtransaction", "\"signedtransactionhex\"")
                            );

-    RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VBOOL));
+    RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ));


The VOBJ check should be independent of RPCTypeCheck if you want to support boolean for backward compatibility.

I see, so the help says it accepts a JSON object, but the implementation allows both?

promag · 2016-02-11T19:07:13Z

@luke-jr ok, added a test for the changeAddress option.

luke-jr · 2016-02-13T03:47:40Z

src/wallet/rpcwallet.cpp

@@ -2478,15 +2482,31 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
    if (origTx.vout.size() == 0)
        throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output");

+    CBitcoinAddress changeAddress;


This probably needs to be defaulting to CNoDestination()

luke-jr · 2016-02-13T03:48:43Z

Also, this conflicts with #7159 , so one of them will need to rebase. I think #7159 should adopt the options Object used here.

promag · 2016-02-13T04:28:33Z

@luke-jr agree #7159 should adopt options.

promag · 2016-02-13T04:49:45Z

@luke-jr done.

promag · 2016-02-13T10:59:02Z

Added changePosition option to specify the desired vout index of change address.

maflcko · 2016-02-13T12:54:19Z

qa/rpc-tests/fundrawtransaction.py

@@ -575,7 +601,7 @@ def run_test(self):
        outputs = {self.nodes[2].getnewaddress() : watchonly_amount / 2}
        rawtx = self.nodes[3].createrawtransaction(inputs, outputs)

-        result = self.nodes[3].fundrawtransaction(rawtx, True)
+        result = self.nodes[3].fundrawtransaction(rawtx, {'includeWatching': True })


Can you leave one of them and mention that this is testing backward compatibility?

promag · 2016-02-13T13:47:45Z

qa/rpc-tests/fundrawtransaction.py

@@ -591,6 +617,7 @@ def run_test(self):
        outputs = {self.nodes[2].getnewaddress() : watchonly_amount}
        rawtx = self.nodes[3].createrawtransaction(inputs, outputs)

+        # Backward compatibility test (2nd param is includeWatching)
        result = self.nodes[3].fundrawtransaction(rawtx, True)


@MarcoFalke FYI.

jonasschnelli · 2016-02-19T12:57:30Z

src/wallet/rpcwallet.cpp

+    bool includeWatching = false;
+
+    if (params.size() > 1) {
+      if (params[1].type() == UniValue::VBOOL) {


If we go for the backward compatibility way: I guess then this code part should be commented as "backward compatibility bool only fallback".

promag · 2016-04-06T22:11:10Z

src/wallet/wallet.cpp

+                            // Insert change txn at random position:
+                            nChangePosInOut = GetRandInt(txNew.vout.size()+1);
+                        }
+                        else if (nChangePosInOut > txNew.vout.size())


@jonasschnelli I think the best place to check if index is out of range is here.

Strict flag forces type check on all object keys.

jonasschnelli · 2016-04-13T07:20:56Z

Tested ACK 18be394.

This is very useful.
It allows to fully decouple the private keys from the node/wallet.

btcdrak · 2016-04-14T11:19:26Z

utACK 18be394

f2d0944 Add lockUnspents option to fundrawtransaction (João Barbosa) af4fe7f Add change options to fundrawtransaction (João Barbosa) 41e835d Add strict flag to RPCTypeCheckObj (João Barbosa)

laanwj · 2016-04-15T14:24:37Z

Merged this as be14ca5, with a trivial rebase (only overlap in test framework) to master

41e835d (was f7dc1d3) Add strict flag to RPCTypeCheckObj
af4fe7f (was db992ea) Add change options to fundrawtransaction
f2d0944 (was 18be394) Add lockUnspents option to fundrawtransaction

f2d0944 Add lockUnspents option to fundrawtransaction (João Barbosa) af4fe7f Add change options to fundrawtransaction (João Barbosa) 41e835d Add strict flag to RPCTypeCheckObj (João Barbosa)

fc81158 [QA] Add test_change_position case to rpc_fundrawtransaction.py (random-zebra) dd35760 [QA] Add test_option_feerate to rpc_fundrawtransaction functional test (random-zebra) 5bca4f4 Add more clear interface for CoinControl.h regarding individual feerate (random-zebra) 169bc3b [RPC] add feerate option to fundrawtransaction (random-zebra) 87dbdf8 [QA] Test new options in rpc_fundrawtransaction functional test (random-zebra) bc9dc67 Add lockUnspents option to fundrawtransaction (random-zebra) a3ac191 Add change options to fundrawtransaction (random-zebra) 0c1f7ba Add strict flag to RPCTypeCheckObj (random-zebra) d655b42 Use CCoinControl selection in CWallet::FundTransaction (random-zebra) 76c8d54 [QA] Test watchonly addrs in fundrawtransaction tests (random-zebra) 134c5d2 Implement watchonly support in fundrawtransaction (random-zebra) 1b153e5 Update importaddress help to push its use to script-only (random-zebra) 7b4eb6d Add importpubkey method to import a watch-only pubkey (random-zebra) 816dabb Add p2sh option to importaddress to import redeemScripts (random-zebra) 60a20a4 Split up importaddress into helper functions (random-zebra) cbffa80 Add logic to track pubkeys as watch-only, not just scripts (random-zebra) 12b38b0 Add have-pubkey distinction to ISMINE flags (random-zebra) fab6556 Exempt unspendable transaction outputs from dust checks (random-zebra) ab407ff [Tests] Fix and enable fundrawtransaction functional tests (random-zebra) bc44ba0 [wallet] allow transaction without change if keypool is empty (random-zebra) a2f8071 [wallet] CreateTransaction: simplify change address check (random-zebra) 761e60e Add fundrawtransaction RPC method (random-zebra) ccb18dd Add FundTransaction method to wallet (random-zebra) 692b827 Add DummySignatureCreator which just creates zeroed sigs (random-zebra) Pull request description: based on top of - [x] #1662 This introduces a new wallet function, `CWallet::FundTransaction()` (and exposes it via RPC with `fundrawtransaction`), to fill a tx containing only vouts (or not enough vins to cover the vouts) with unspent coins from the wallet. `fundrawtransaction` will not modify existing inputs, and will add one change output (if needed) to the outputs. It will not sign the inputs (so can include also watch-only or multi-sig inputs, if enabled). backported from: - bitcoin#6088 - bitcoin#17219 [`*`] - bitcoin#6417 - bitcoin#6444 - bitcoin#6415 - bitcoin#6828 - bitcoin#7296 (only bebe58b) - bitcoin#7506 - bitcoin#7518 - bitcoin#7967 adapting the tests for the (more recent) framework. [`*`] Note: this has been included to be able to call `fundrawtransaction` without the need for an unencrypted wallet (for the change address key) ACKs for top commit: furszy: re ACK fc81158 . Fuzzbawls: ACK fc81158 Tree-SHA512: 10235ce6e672a1cfd4ae2cad9312864c82971f6a4aa1a8ed9489d85156f5c4126c293180a7f1b86b7c65d07caab484e9a6d7a87ebf032bee55adb98d3e08e7b9

…bt also lock manually selected coins 6d1f513 [rpc] fundrawtransaction, walletcreatefundedpsbt lock manually selected coins (Sjors Provoost) Pull request description: When using `fundrawtransaction` and `walletcreatefundedpsbt` with `lockUnspents`, it would only lock automatically selected coins, not manually selected coins. That doesn't make much sense to me if the goal is to prevent accidentally double-spending yourself before you broadcast a transaction. Note that when creating a transaction, manually selected coins are automatic "unlocked" (or more accurately: the lock is ignored). Earlier versions of this PR introduced an error when a locked coin is manually selected, but this idea was abandoned after some discussion. An application that uses this RPC should either rely on automatic coin selection (with `lockUnspents`) or handle lock concurrency itself with manual coin selection. In particular it needs to make sure to avoid/pause calls with automatic coin selection between calling `lockunspent` and the subsequent spending RPC. See bitcoin#7518 for historical background. ACKs for top commit: meshcollider: Code review ACK 6d1f513 fjahr: Code review ACK 6d1f513 Tree-SHA512: 8773c788d92f2656952e1beac147ba9956b8c5132d474e0880e4c89ff53642928b4cbfcd1cb3d17798b9284f02618a8830c93a9f7a4733e5bded96adff1d5d4d

promag reviewed Feb 11, 2016
View reviewed changes

promag force-pushed the enhancement/fundrawtransaction-with-changeaddress branch from 30dfa20 to 97b60eb Compare February 11, 2016 17:48

luke-jr reviewed Feb 11, 2016
View reviewed changes

promag force-pushed the enhancement/fundrawtransaction-with-changeaddress branch from 97b60eb to 6235274 Compare February 11, 2016 19:06

promag force-pushed the enhancement/fundrawtransaction-with-changeaddress branch from 6235274 to 7f5e281 Compare February 11, 2016 22:43

luke-jr reviewed Feb 13, 2016
View reviewed changes

promag force-pushed the enhancement/fundrawtransaction-with-changeaddress branch from 7f5e281 to 9ef5802 Compare February 13, 2016 04:47

promag force-pushed the enhancement/fundrawtransaction-with-changeaddress branch 3 times, most recently from 8b3caf3 to 84b2bb9 Compare February 13, 2016 05:00

laanwj added the Wallet label Feb 13, 2016

promag force-pushed the enhancement/fundrawtransaction-with-changeaddress branch from 84b2bb9 to c0cfc61 Compare February 13, 2016 10:58

promag changed the title ~~[RPC] Add changeAddress option to fundrawtransaction~~ [RPC] Add change options to fundrawtransaction Feb 13, 2016

maflcko reviewed Feb 13, 2016
View reviewed changes

promag force-pushed the enhancement/fundrawtransaction-with-changeaddress branch 2 times, most recently from 809044c to 709eda4 Compare February 13, 2016 13:47

promag reviewed Feb 13, 2016
View reviewed changes

promag force-pushed the enhancement/fundrawtransaction-with-changeaddress branch from 709eda4 to 49cea79 Compare February 13, 2016 15:23

promag mentioned this pull request Feb 19, 2016

Add importmulti RPC call #7551

Merged

jonasschnelli reviewed Feb 19, 2016
View reviewed changes

promag reviewed Apr 6, 2016
View reviewed changes

maflcko mentioned this pull request Apr 7, 2016

Travis sometimes misses commits/pulls travis-ci/travis-ci#5863

Closed

promag mentioned this pull request Apr 11, 2016

Add fee option to fundrawtransaction #7857

Closed

João Barbosa added 3 commits April 11, 2016 10:26

Add strict flag to RPCTypeCheckObj

f7dc1d3

Strict flag forces type check on all object keys.

Add change options to fundrawtransaction

db992ea

Add lockUnspents option to fundrawtransaction

18be394

promag force-pushed the enhancement/fundrawtransaction-with-changeaddress branch from bc4b4fb to 18be394 Compare April 11, 2016 09:26

sipa mentioned this pull request Apr 15, 2016

Optional parameter to fundrawtransaction: specify change address explicitly #7880

Closed

laanwj closed this Apr 15, 2016

paveljanik mentioned this pull request Apr 16, 2016

fundrawtransaction issues after #7518 #7889

Closed

jonasschnelli mentioned this pull request Apr 27, 2016

[RPC] Add bumpfee command. #7865

Closed

laanwj mentioned this pull request Apr 28, 2016

Optional feerate option for fundrawtransaction #7963

Closed

laanwj mentioned this pull request Jun 8, 2016

TODO for release notes 0.13.0 #7678

Closed

16 tasks

jonasschnelli mentioned this pull request Jun 19, 2016

Are you going to add Trezor support? #8218

Closed

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016

Merge bitcoin#7518 fundrawtxn_changeaddress-0.12.x

07dc972

dagurval mentioned this pull request Dec 27, 2017

Add fundrawtransaction bitcoinxt/bitcoinxt#288

Merged

Sjors mentioned this pull request Mar 6, 2020

rpc: fundrawtransaction and walletcreatefundedpsbt also lock manually selected coins #18244

Merged

random-zebra mentioned this pull request Jun 4, 2020

[Wallet][RPC] FundTransaction - fundrawtransaction PIVX-Project/PIVX#1663

Merged

1 task

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add multiple options to fundrawtransaction #7518

Add multiple options to fundrawtransaction #7518

Uh oh!

Conversation

promag commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Feb 11, 2016

Uh oh!

promag commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Feb 13, 2016

Uh oh!

promag commented Feb 13, 2016

Uh oh!

promag commented Feb 13, 2016

Uh oh!

promag commented Feb 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Apr 13, 2016

Uh oh!

btcdrak commented Apr 14, 2016

Uh oh!

laanwj commented Apr 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

laanwj commented Apr 15, 2016 •

edited

Loading