Add multiple options to fundrawtransaction

promag commented at 5:01 pm on February 11, 2016: member

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.

in src/wallet/rpcwallet.cpp: in 30dfa203e4 outdated

2467@@ -2468,7 +2468,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
2468                             + HelpExampleCli("sendrawtransaction", "\"signedtransactionhex\"")
2469                             );
2470 
2471-    RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VBOOL));
2472+    // RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VBOOL));

promag commented at 5:02 pm on February 11, 2016:

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

luke-jr commented at 5:14 pm on February 11, 2016:

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

in src/wallet/wallet.h: in 30dfa203e4 outdated

734@@ -735,7 +735,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
735      * Insert additional inputs into the transaction by
736      * calling CreateTransaction();
737      */
738-    bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosRet, std::string& strFailReason, bool includeWatching);
739+    bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosRet, std::string& strFailReason, bool includeWatching, const CTxDestination& destChange);

promag commented at 5:03 pm on February 11, 2016:

Don’t know if the best place is at the end.

luke-jr commented at 5:16 pm on February 11, 2016:

As good a place as any. But you need to update the implementation too…

promag commented at 5:34 pm on February 11, 2016:

Ops, missed that in git add -p.

luke-jr commented at 3:47 am on February 13, 2016:

A default CNoDestination() here might be a good idea.

luke-jr commented at 5:15 pm on February 11, 2016: member

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 force-pushed on Feb 11, 2016

promag commented at 6:34 pm on February 11, 2016: member

@luke-jr done.

in src/wallet/rpcwallet.cpp: in 97b60eb479 outdated

2471@@ -2468,7 +2472,7 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
2472                             + HelpExampleCli("sendrawtransaction", "\"signedtransactionhex\"")
2473                             );
2474 
2475-    RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VBOOL));
2476+    RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ));

luke-jr commented at 6:39 pm on February 11, 2016:

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

promag commented at 6:52 pm on February 11, 2016:

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

in src/wallet/wallet.cpp: in 97b60eb479 outdated

376@@ -377,7 +377,7 @@ bool CWallet::Verify(const string& walletFile, string& warningString, string& er
377         } catch (const boost::filesystem::filesystem_error&) {
378             // failure is ok (well, not really, but it's not worse than what we started with)
379         }
380-        
381+

luke-jr commented at 6:39 pm on February 11, 2016:

Please don’t change unrelated whitespace like this.

promag force-pushed on Feb 11, 2016

promag commented at 7:07 pm on February 11, 2016: member

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

promag force-pushed on Feb 11, 2016

in src/wallet/rpcwallet.cpp: in 7f5e281115 outdated

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

luke-jr commented at 3:47 am on February 13, 2016:

This probably needs to be defaulting to CNoDestination()

luke-jr commented at 3:48 am on February 13, 2016: member

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

promag commented at 4:28 am on February 13, 2016: member

@luke-jr agree #7159 should adopt options.

promag force-pushed on Feb 13, 2016

promag commented at 4:49 am on February 13, 2016: member

@luke-jr done.

promag force-pushed on Feb 13, 2016

laanwj added the label Wallet on Feb 13, 2016

promag force-pushed on Feb 13, 2016

promag renamed this:
~~[RPC] Add changeAddress option to fundrawtransaction~~
[RPC] Add change options to fundrawtransaction
on Feb 13, 2016

promag commented at 10:59 am on February 13, 2016: member

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

in qa/rpc-tests/fundrawtransaction.py: in c0cfc61067 outdated

600@@ -575,7 +601,7 @@ def run_test(self):
601         outputs = {self.nodes[2].getnewaddress() : watchonly_amount / 2}
602         rawtx = self.nodes[3].createrawtransaction(inputs, outputs)
603 
604-        result = self.nodes[3].fundrawtransaction(rawtx, True)
605+        result = self.nodes[3].fundrawtransaction(rawtx, {'includeWatching': True })

MarcoFalke commented at 12:54 pm on February 13, 2016:

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

promag commented at 1:39 pm on February 13, 2016:

:+1:

promag commented at 10:02 pm on March 24, 2016:

See below.

promag force-pushed on Feb 13, 2016

in qa/rpc-tests/fundrawtransaction.py: in 709eda42d0 outdated

616@@ -591,6 +617,7 @@ def run_test(self):
617         outputs = {self.nodes[2].getnewaddress() : watchonly_amount}
618         rawtx = self.nodes[3].createrawtransaction(inputs, outputs)
619 
620+        # Backward compatibility test (2nd param is includeWatching)
621         result = self.nodes[3].fundrawtransaction(rawtx, True)

promag commented at 1:47 pm on February 13, 2016:

@MarcoFalke FYI.

in src/wallet/wallet.h: in 709eda42d0 outdated

734@@ -735,7 +735,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
735      * Insert additional inputs into the transaction by
736      * calling CreateTransaction();
737      */
738-    bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosRet, std::string& strFailReason, bool includeWatching);

promag commented at 2:46 pm on February 13, 2016:

I feel we should have a better argument order. Suggestion:

0bool FundTransaction(CMutableTransaction& tx, const CTxDestination& changeDestination, int& changePosition, bool includeWatching, CAmount& nFeeRet, std::string& strFailReason);

luke-jr commented at 8:54 pm on February 13, 2016:

It doesn’t really matter as long as existing code either fails to compile or behaves the same. Note that to put destChange earlier, you would need to make it a required option.

jonasschnelli commented at 1:01 pm on February 19, 2016:

I guess we should rename nChangePosRet to nChangePosInOut and add a comment in the header that -1 as a input value will result in setting a random position.

promag force-pushed on Feb 13, 2016

in src/wallet/rpcwallet.cpp: in 49cea7936d outdated

2479+    CTxDestination changeAddress = CNoDestination();
2480+    int changePosition = -1;
2481+    bool includeWatching = false;
2482+
2483+    if (params.size() > 1) {
2484+      if (params[1].type() == UniValue::VBOOL) {

jonasschnelli commented at 12:57 pm on February 19, 2016:

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

in src/wallet/wallet.cpp: in 49cea7936d outdated

2135@@ -2134,14 +2136,17 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2136                     // add the dust to the fee.
2137                     if (newTxOut.IsDust(::minRelayTxFee))
2138                     {
2139+                      nChangePos = -1;

jonasschnelli commented at 1:03 pm on February 19, 2016:

nit: indent.

in src/wallet/wallet.cpp: in 49cea7936d outdated

2144                     {
2145-                        // Insert change txn at random position:
2146-                        nChangePosRet = GetRandInt(txNew.vout.size()+1);
2147-                        vector<CTxOut>::iterator position = txNew.vout.begin()+nChangePosRet;
2148+                        if (nChangePos == -1) {
2149+                          // Insert change txn at random position:

jonasschnelli commented at 1:03 pm on February 19, 2016:

nit: indent (2 instead 4), maybe run the clang format script.

in src/wallet/rpcwallet.cpp: in 49cea7936d outdated

2487+      else {
2488+        RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ));
2489+
2490+        UniValue options = params[1];
2491+
2492+        if (options.exists("changeAddress"))

jonasschnelli commented at 1:05 pm on February 19, 2016:

nit: Not sure if we should make this check case insensitive. Very likely, some users will try it with “changeaddress”.

luke-jr commented at 3:26 pm on February 19, 2016:

I think we already have precedent for case-sensitive options?

in src/wallet/rpcwallet.cpp: in 49cea7936d outdated

2486+      }
2487+      else {
2488+        RPCTypeCheck(params, boost::assign::list_of(UniValue::VSTR)(UniValue::VOBJ));
2489+
2490+        UniValue options = params[1];
2491+

jonasschnelli commented at 1:08 pm on February 19, 2016:

Maybe add a RPCTypeCheckObj() to ensure that changePosition, etc. are the correct types? Otherwise you get a plain JSON value is not a boolean as expected (etc.)?

promag commented at 9:37 am on March 5, 2016:

:+1:

promag commented at 10:03 pm on March 24, 2016:

See below.

jonasschnelli commented at 1:11 pm on February 19, 2016: contributor

utACK. Nice work!

I’m not sure if we should add the backward compatibility fallback (includeWatchonly = true). Because it could also lead to problems in future. If so, we should also mention in in the RPC help (something like a plain True results in includeWatchonly=true).

IMO a API break would be acceptable.

jonasschnelli added the label RPC on Feb 19, 2016

promag renamed this:
~~[RPC] Add change options to fundrawtransaction~~
Add change options to fundrawtransaction
on Feb 25, 2016

luke-jr referenced this in commit c63863af74 on Feb 25, 2016

nunofgs commented at 3:11 pm on March 2, 2016: none

@promag: it would be useful to add an option “lockUnspents: true” to ensure that chosen unspents are effectively locked before sending the transaction.

promag commented at 4:30 pm on March 2, 2016: member

@nunofgs yes, that idea was also in #7498.

promag force-pushed on Mar 5, 2016

promag renamed this:
~~Add change options to fundrawtransaction~~
Add multiple options to fundrawtransaction
on Mar 5, 2016

promag commented at 9:50 am on March 5, 2016: member

@nunofgs lockUnspents option added. Missing test for the moment.

promag commented at 9:52 am on March 5, 2016: member

Visited all comments and rebased. Please review.

promag commented at 9:53 am on March 5, 2016: member

What about adding unlockUnspents option to sendrawtransaction?

promag force-pushed on Mar 5, 2016

ruimarinho commented at 10:47 am on March 7, 2016: none

@promag I think that should be part of the standard process - as soon as the transaction is sent to the network, the unspents should be unlocked and considered spent.

jonasschnelli commented at 10:58 am on March 7, 2016: contributor

@ruimarinho: you probably mean “locking”. I think locking of the unspents should happen somewhere between sign and send. I’m fine with an option in fundrawtransaction, but IMO we should not make it the default.

promag commented at 11:02 am on March 7, 2016: member

@sipa @laanwj do you think it’s acceptable to specify a fixed fee and bypass the minimal fee calculation through iteration in CWallet::CreateTransaction?

promag commented at 11:05 am on March 7, 2016: member

@jonasschnelli sure, default for locking is false. The use case is to prevent consecutive fundrawtransactions calls selecting the same unspents.

ruimarinho commented at 11:09 am on March 7, 2016: none

@jonasschnelli if I call lockUnspent(false, [unspents]) manually, those unspents will remain unlocked until bitcoind is restarted or lockUnspent(true, [unspents]) is called. I did not see a code path where these unspents are cleaned up - removed from the “locked” list but still unavailable for future spending.

jonasschnelli commented at 11:15 am on March 7, 2016: contributor

@ruimarinho: Yes. There is no such thing as a auto-release because the intention is the keep these unspents locked.

But if you lock the unspents in fundrawtransaction by default, you need to unlock them in case you don’t sign or send them (or in cases of an error). But I agree, there are use-cases where you want to directly lock the unspent in the fundrawtransaction call (concurrency).

promag commented at 11:20 am on March 7, 2016: member

@jonasschnelli the same applies if someone locks some unspents with lockunspent and don’t use it.

jonasschnelli commented at 11:21 am on March 7, 2016: contributor

@promag: yes. Agree. The default value is debatable.

promag force-pushed on Mar 24, 2016

promag commented at 10:01 pm on March 24, 2016: member

@laanwj @sipa bump.

promag commented at 10:01 pm on March 24, 2016: member

Rebased.

in src/wallet/wallet.cpp: in a6d57fdecc outdated

1940     // Add new txins (keeping original txin scriptSig/order)
1941     BOOST_FOREACH(const CTxIn& txin, wtx.vin)
1942     {
1943         if (!coinControl.IsSelected(txin.prevout))
1944             tx.vin.push_back(txin);
1945+        }

MarcoFalke commented at 4:52 pm on March 25, 2016:

Where does this scope start?

promag commented at 10:22 pm on March 25, 2016:

It doesn’t :)

MarcoFalke commented at 9:31 am on March 29, 2016:

Mind to fix it?

jonasschnelli commented at 9:21 am on March 29, 2016: contributor

I really think this is useful! @promag can you look at the travis issues, maybe rebase it? I promise to test it afterwards. :)

promag force-pushed on Mar 29, 2016

promag commented at 5:09 pm on March 29, 2016: member

@MarcoFalke fixed @jonasschnelli rebased and renamed nChangePosRet to nChangePosInOut. I wonder if other *Ret should be renamed to *Out?

in src/wallet/wallet.cpp: in 0fcf7dbc93 outdated

1907@@ -1908,7 +1908,7 @@ bool CWallet::SelectCoins(const vector<COutput>& vAvailableCoins, const CAmount&
1908     return res;
1909 }
1910 
1911-bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosRet, std::string& strFailReason, bool includeWatching)
1912+bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount &nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, const CTxDestination& destChange)

jonasschnelli commented at 6:43 pm on March 29, 2016:

missing bool lockUnspents, argument.

jonasschnelli commented at 7:28 pm on March 29, 2016: contributor

I think this PR should be in 0.13 (once finalized)! Tested a bit after fixing the compile error (see above).

Some issues:

1) I called

fundrawtransaction 01000000000100e1f505000000001976a914f592d5ff0f4cad760d23601e32714639c244d27588ac00000000 '{"changeAddress":"mfrczJz4p5ziD4CiVTip3R7jjXJAjDKBmf", "includeWatching":true, "changePos":0}' I used changePos but the valid argument would be changePosition. An error in a such case would be nice.

2) Also, I used a invalid change address… fundrawtransaction 01000000000100e1f505000000001976a914f592d5ff0f4cad760d23601e32714639c244d27588ac00000000 '{"changeAddress":"dontforget.to.call@your.mother", "includeWatching":true, "changePosition":0}' … which was accepted (no error or warning) but not used. It used one from the wallets pool (which could be dangerous in a watchOnly-fund-environment.

detect invalid options and reject command with an error
detect invalid change addresses and reject if so

Happy to test again.

promag commented at 8:53 pm on March 29, 2016: member

@jonasschnelli Thanks, I’ll add tests to cover those cases. BTW, travis is not picking this PR?

promag force-pushed on Mar 30, 2016

promag commented at 1:05 am on March 30, 2016: member

Please review. @jonasschnelli FYI

detect invalid options and reject command with an error: 7ba5713
detect invalid change addresses and reject if so

ruimarinho commented at 2:19 am on March 30, 2016: none

@jonasschnelli, since you’re more involved in this PR, what do you think of @promag’s suggestion above?

do you think it’s acceptable to specify a fixed fee and bypass the minimal fee calculation through iteration in CWallet::CreateTransaction?

promag commented at 2:32 am on March 30, 2016: member

@ruimarinho maybe a separate PR on top of this because that is a change on CWallet::CreateTransaction.

jonasschnelli commented at 6:07 am on March 30, 2016: contributor

@ruimarinho: Yes. A common problems of Bitcoin Core pulls is the “expanding scope”. Keep it simple, add other features later in a different PR.

I think we need to distinct FRT and keep it – relatively simple. If we add options to set the fee, the only thing thats left is the coin selection then? not? Maybe someone could just grab the unspents over listunspents and do his own coin selection and create a final transaction with createrawtransaction.

And if you know the changeoutputs address, you could rewrite the transaction after FRT (on your side of the application) and change its fee. @promag: thanks. Will re-test soon.

promag commented at 7:48 am on March 30, 2016: member

And if you know the changeoutputs address, you could rewrite the transaction after FRT (on your side of the application) and change its fee. @jonasschnelli this is not entirely correct because there may be a better subset of unspents to better satisfy the output value + fixed fee. But since the fee is very low this may be a good tradeoff.

Edit:

Also there this special case when no change is created, so the application has to create one to adjust the difference.

2nd edit::

And the worst case is when the total input value of the selected unspents isn’t enough to cover the fixed fee. So I would say that having the option to specify the fixed fee is the way to go.

promag commented at 11:29 am on March 30, 2016: member

the only thing thats left is the coin selection then @jonasschnelli actually this is accomplished with createrawtransaction.

jonasschnelli commented at 2:34 pm on March 30, 2016: contributor

[…] And the worst case is when the total input value of the selected unspents isn’t enough to cover the fixed fee. So I would say that having the option to specify the fixed fee is the way to go.

Yes. This is a point.

the only thing thats left is the coin selection then

actually this is accomplished with createrawtransaction.

createrawtransaction does not use coinselection IMO. FRT does.

in src/wallet/wallet.cpp: in 06e2b0bc1d outdated

1943         if (!coinControl.IsSelected(txin.prevout))
1944+        {
1945             tx.vin.push_back(txin);
1946+
1947+            if (lockUnspents)
1948+                LockCoin(txin.prevout);

jonasschnelli commented at 2:42 pm on March 30, 2016:

You need to hold cs_wallet for this operation. I think between L1944 and L1945 you should add a LOCK2(cs_main, cs_wallet);

jonasschnelli commented at 3:07 pm on March 30, 2016: contributor

The type check works and I think it’s great. Missing LockCoin lock needs to be fixed.

promag force-pushed on Mar 30, 2016

promag commented at 3:52 pm on March 30, 2016: member

@jonasschnelli Thanks! Fixed and rebased.

ruimarinho commented at 0:11 am on March 31, 2016: none

I was not suggesting otherwise regarding merging this PR and only then creating a new one to add the fixed fee feature :) like @promag mentioned on the edited comments, I believe there is still space for such feature as at least two scenarios are not covered by standard calls.

in src/wallet/rpcwallet.cpp: in 4d60824502 outdated

2456+                            "1. \"hexstring\"           (string, required) The hex string of the raw transaction\n"
2457+                            "2. options               (object, optional)\n"
2458+                            "   {\n"
2459+                            "     \"changeAddress\"     (string, optional, default pool address) The bitcoin address to receive the change\n"
2460+                            "     \"changePosition\"    (numeric, optional, default random) The index of the change output\n"
2461+                            "     \"includeWatching\"   (boolean, optional, default false) Also select inputs which are watch only\n"

ruimarinho commented at 0:12 am on March 31, 2016:

Bikeshedding but have you considered includeWatchOnly to follow a consistent nomenclature?

promag commented at 8:04 am on March 31, 2016:

The underlying flag is allowWatchOnly. includeWatching is the old parameter name. Happy to rename if others agree.

jonasschnelli commented at 11:56 am on March 31, 2016:

IMO we should keep includeWatching.

in src/wallet/rpcwallet.cpp: in 4d60824502 outdated

2450@@ -2451,8 +2451,14 @@ UniValue fundrawtransaction(const UniValue& params, bool fHelp)
2451                             "in the wallet using importaddress or addmultisigaddress (to calculate fees).\n"
2452                             "Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n"
2453                             "\nArguments:\n"
2454-                            "1. \"hexstring\"     (string, required) The hex string of the raw transaction\n"
2455-                            "2. includeWatching (boolean, optional, default false) Also select inputs which are watch only\n"
2456+                            "1. \"hexstring\"           (string, required) The hex string of the raw transaction\n"
2457+                            "2. options               (object, optional)\n"

jonasschnelli commented at 12:01 pm on March 31, 2016:

nit: here we should mention something like : “for backward compatibility: passing in a true instead of an object will result in {"includeWatching":true}"

in src/wallet/wallet.cpp: in 4d60824502 outdated

1933         return false;
1934 
1935-    if (nChangePosRet != -1)
1936-        tx.vout.insert(tx.vout.begin() + nChangePosRet, wtx.vout[nChangePosRet]);
1937+    if (nChangePosInOut != -1)
1938+        tx.vout.insert(tx.vout.begin() + nChangePosInOut, wtx.vout[nChangePosInOut]);

jonasschnelli commented at 12:09 pm on March 31, 2016:

Here we need another check. What if nChangePosInOut = 10 but tx.vout.size()==2? We need a out-of-range exception in a such case.

promag commented at 8:10 am on April 2, 2016:

Check is in rpcwallet.cpp.

jonasschnelli commented at 7:22 am on April 5, 2016:

@promag: The check should should really be here (maybe an additional check). If someone uses CWallet::FundTransaction() with a nChangePosInOut >= wtx.vout.size(), the app crashes/throws an exceptions. Future changes might easy get trapped by this.

The problematic part is wtx.vout[nChangePosInOut]. There should be a check somewhere in CWallet::FundTransaction() that leads to a return false if a overflow has detected.

jonasschnelli commented at 12:10 pm on March 31, 2016: contributor

Allmost done. Last iteration:

needs out-of-range detection of changePosition.
needs simple documentation of backward compatibility (true instead of an parameter object)

promag force-pushed on Apr 2, 2016

promag commented at 8:10 am on April 2, 2016: member

@jonasschnelli updated and rebased. not sure where is the best place to put the new help message.

promag force-pushed on Apr 2, 2016

jonasschnelli commented at 12:01 pm on April 6, 2016: contributor

ping @promag: Would be nice if you could take the change output order check into CreateTransaction(): https://github.com/bitcoin/bitcoin/pull/7518/files#r58496250 I’m convince that this is the last issue before this is ready for merging.

Please also fix the commit order. The head commit is from March 5, but the HEAD~2 is March 30.

 0commit 4b0360f4160e059b61a21a95a6106d0eed927bd9
 1Author: João Barbosa <joao@uphold.com>
 2Date:   Sat Mar 5 09:45:57 2016 +0000
 3
 4commit c2a3ff1a08df8619f7af77ebe6c3b7661e858bc1
 5Author: João Barbosa <joao@uphold.com>
 6Date:   Wed Mar 30 02:04:22 2016 +0100
 7
 8commit 699720b3baa9e0d3dab679b1da91ab45808aa85e
 9Author: João Barbosa <joao@uphold.com>
10Date:   Wed Mar 30 00:59:29 2016 +0100

promag force-pushed on Apr 6, 2016

Add strict flag to RPCTypeCheckObj

Strict flag forces type check on all object keys.

f7dc1d32bb

Add change options to fundrawtransaction db992eadbc

Add lockUnspents option to fundrawtransaction 18be394cd8

in src/wallet/wallet.cpp: in bc4b4fb3e4 outdated

2181+                        if (nChangePosInOut == -1)
2182+                        {
2183+                            // Insert change txn at random position:
2184+                            nChangePosInOut = GetRandInt(txNew.vout.size()+1);
2185+                        }
2186+                        else if (nChangePosInOut > txNew.vout.size())

promag commented at 10:11 pm on April 6, 2016:

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

promag force-pushed on Apr 11, 2016

jonasschnelli commented at 7:20 am on April 13, 2016: contributor

Tested ACK 18be394cd8cf5b021e25fff34066f1a3a002858c.

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

btcdrak commented at 11:19 am on April 14, 2016: contributor

utACK 18be394

laanwj referenced this in commit be14ca5e8c on Apr 15, 2016

laanwj commented at 2:24 pm on April 15, 2016: member

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

41e835d (was https://github.com/bitcoin/bitcoin/pull/7518/commits/f7dc1d32bbd23ff593e63f575a2d446a96e1ea15) Add strict flag to RPCTypeCheckObj
af4fe7f (was https://github.com/bitcoin/bitcoin/pull/7518/commits/db992eadbcbde0ca14df66c120a5868938e61e83) Add change options to fundrawtransaction
f2d0944 (was https://github.com/bitcoin/bitcoin/pull/7518/commits/18be394cd8cf5b021e25fff34066f1a3a002858c) Add lockUnspents option to fundrawtransaction

laanwj closed this on Apr 15, 2016

luke-jr referenced this in commit 8f0b1ac062 on Jun 27, 2016

luke-jr referenced this in commit ebdb4aa9a2 on Jun 27, 2016

luke-jr referenced this in commit 07dc972307 on Jun 28, 2016

codablock referenced this in commit 9846028a20 on Sep 16, 2017

codablock referenced this in commit 16d9225d46 on Sep 19, 2017

codablock referenced this in commit 6c7d133801 on Dec 20, 2017

furszy referenced this in commit 0724bbbad2 on Jun 28, 2020

sidhujag referenced this in commit 49daca74b8 on Aug 31, 2020

MarcoFalke locked this on Sep 8, 2021

Add multiple options to fundrawtransaction #7518