Add multiple options to fundrawtransaction #7518

promag · Feb 11, 2016

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.

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.

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

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

Ops, missed that in git add -p.

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

luke-jr · Feb 11, 2016

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 · Feb 11, 2016

@luke-jr done.

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?

Please don't change unrelated whitespace like this.

promag · Feb 11, 2016

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

This probably needs to be defaulting to CNoDestination()

luke-jr · Feb 13, 2016

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

promag · Feb 13, 2016

@luke-jr agree #7159 should adopt options.

promag · Feb 13, 2016

@luke-jr done.

promag · Feb 13, 2016

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

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

👍

See below.

@MarcoFalke FYI.

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

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

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.

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.

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

nit: indent.

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

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

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

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

👍

See below.

jonasschnelli · Feb 19, 2016

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.

nunofgs · Mar 2, 2016

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

promag · Mar 2, 2016

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

promag · Mar 5, 2016

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

promag · Mar 5, 2016

Visited all comments and rebased. Please review.

promag · Mar 5, 2016

What about adding unlockUnspents option to sendrawtransaction?

ruimarinho · Mar 7, 2016

@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 · Mar 7, 2016

@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 · Mar 7, 2016

@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 · Mar 7, 2016

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

ruimarinho · Mar 7, 2016

@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 · Mar 7, 2016

@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 · Mar 7, 2016

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

jonasschnelli · Mar 7, 2016

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

promag · Mar 24, 2016

@laanwj @sipa bump.

promag · Mar 24, 2016

Rebased.

Where does this scope start?

It doesn't :)

Mind to fix it?

jonasschnelli · Mar 29, 2016

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

promag · Mar 29, 2016

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

missing bool lockUnspents, argument.

jonasschnelli · Mar 29, 2016

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 · Mar 29, 2016

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

promag · Mar 30, 2016

Please review.

@jonasschnelli FYI

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

ruimarinho · Mar 30, 2016

@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 · Mar 30, 2016

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

jonasschnelli · Mar 30, 2016

@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 · Mar 30, 2016

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 · Mar 30, 2016

the only thing thats left is the coin selection then

@jonasschnelli actually this is accomplished with createrawtransaction.

jonasschnelli · Mar 30, 2016

[...] 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.

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 · Mar 30, 2016

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

promag · Mar 30, 2016

@jonasschnelli Thanks! Fixed and rebased.

ruimarinho · Mar 31, 2016

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.

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

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

IMO we should keep includeWatching.

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

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.

Check is in rpcwallet.cpp.

@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 · Mar 31, 2016

Allmost done. Last iteration:

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

promag · Apr 2, 2016

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

jonasschnelli · Apr 6, 2016

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.

commit 4b0360f4160e059b61a21a95a6106d0eed927bd9
Author: João Barbosa <joao@uphold.com>
Date:   Sat Mar 5 09:45:57 2016 +0000

commit c2a3ff1a08df8619f7af77ebe6c3b7661e858bc1
Author: João Barbosa <joao@uphold.com>
Date:   Wed Mar 30 02:04:22 2016 +0100

commit 699720b3baa9e0d3dab679b1da91ab45808aa85e
Author: João Barbosa <joao@uphold.com>
Date:   Wed Mar 30 00:59:29 2016 +0100

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

jonasschnelli · Apr 13, 2016

Tested ACK 18be394.

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

btcdrak · Apr 14, 2016

utACK 18be394

laanwj · Apr 15, 2016

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

laanwj added the Wallet label Feb 13, 2016

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

promag referenced this pull request Feb 19, 2016
Merged
Add importmulti RPC call #7551

jonasschnelli added the RPC/REST/ZMQ label Feb 19, 2016

promag changed the title from [RPC] Add change options to fundrawtransaction to Add change options to fundrawtransaction Feb 25, 2016

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

Add change options to fundrawtransaction
Github-Pull: #7518 Rebased-From: 49cea7936dc44b5f3c3373c4190cb260996ef56b

c63863a

promag changed the title from Add change options to fundrawtransaction to Add multiple options to fundrawtransaction Mar 5, 2016

This was referenced Apr 7, 2016

Open

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

Closed

Add fee option to fundrawtransaction #7857

promag added some commits Mar 29, 2016

Add strict flag to RPCTypeCheckObj
Strict flag forces type check on all object keys.

f7dc1d3

Add change options to fundrawtransaction

db992ea

Add lockUnspents option to fundrawtransaction

18be394

sipa referenced this pull request Apr 15, 2016
Closed
Optional parameter to fundrawtransaction: specify change address explicitly #7880

laanwj closed this Apr 15, 2016

This was referenced Apr 16, 2016

Closed

fundrawtransaction issues after #7518 #7889

Closed

[RPC] Add bumpfee command. #7865

Closed

Optional feerate option for `fundrawtransaction` #7963

Closed

TODO for release notes 0.13.0 #7678

Closed

Are you going to add Trezor support? #8218

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

Merge #7518 fundrawtxn_changeaddress-0.12.x

07dc972

@@ -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));

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

@@ -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));

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

@@ -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 })

bitcoin/bitcoin

Join GitHub today

Add multiple options to fundrawtransaction #7518

Conversation

promag commented Feb 11, 2016

Show 2 comments Hide comments promag and 1 other commented on an outdated diff Feb 11, 2016

Show 4 comments Hide comments promag and 1 other commented on an outdated diff Feb 11, 2016

luke-jr commented Feb 11, 2016

promag commented Feb 11, 2016

Show 2 comments Hide comments luke-jr and 1 other commented on an outdated diff Feb 11, 2016

Show 1 comment Hide comment luke-jr commented on an outdated diff Feb 11, 2016

promag commented Feb 11, 2016

Show 1 comment Hide comment luke-jr commented on an outdated diff Feb 13, 2016

luke-jr commented Feb 13, 2016

promag commented Feb 13, 2016

promag commented Feb 13, 2016

laanwj added the Wallet label Feb 13, 2016

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

promag commented Feb 13, 2016

MarcoFalke commented on the diff Feb 13, 2016

promag commented on the diff Feb 13, 2016

promag commented on the diff Feb 13, 2016

promag referenced this pull request Feb 19, 2016

Show 1 comment Hide comment jonasschnelli commented on an outdated diff Feb 19, 2016

Show 1 comment Hide comment jonasschnelli commented on an outdated diff Feb 19, 2016

Show 1 comment Hide comment jonasschnelli commented on an outdated diff Feb 19, 2016

Show 2 comments Hide comments jonasschnelli and 1 other commented on an outdated diff Feb 19, 2016

jonasschnelli commented on the diff Feb 19, 2016

jonasschnelli commented Feb 19, 2016

jonasschnelli added the RPC/REST/ZMQ label Feb 19, 2016

promag changed the title from [RPC] Add change options to fundrawtransaction to Add change options to fundrawtransaction Feb 25, 2016

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

nunofgs commented Mar 2, 2016

promag commented Mar 2, 2016

promag changed the title from Add change options to fundrawtransaction to Add multiple options to fundrawtransaction Mar 5, 2016

promag commented Mar 5, 2016

promag commented Mar 5, 2016

promag commented Mar 5, 2016

ruimarinho commented Mar 7, 2016

jonasschnelli commented Mar 7, 2016

promag commented Mar 7, 2016

promag commented Mar 7, 2016

ruimarinho commented Mar 7, 2016

jonasschnelli commented Mar 7, 2016

promag commented Mar 7, 2016

jonasschnelli commented Mar 7, 2016

promag commented Mar 24, 2016

promag commented Mar 24, 2016

Show 3 comments Hide comments MarcoFalke and 1 other commented on an outdated diff Mar 25, 2016

jonasschnelli commented Mar 29, 2016

promag commented Mar 29, 2016

Show 1 comment Hide comment jonasschnelli commented on an outdated diff Mar 29, 2016

jonasschnelli commented Mar 29, 2016

promag commented Mar 29, 2016

promag commented Mar 30, 2016

ruimarinho commented Mar 30, 2016

promag commented Mar 30, 2016

jonasschnelli commented Mar 30, 2016

promag commented Mar 30, 2016

promag commented Mar 30, 2016

jonasschnelli commented Mar 30, 2016

Show 1 comment Hide comment jonasschnelli commented on an outdated diff Mar 30, 2016

jonasschnelli commented Mar 30, 2016

promag commented Mar 30, 2016

ruimarinho commented Mar 31, 2016

ruimarinho commented on the diff Mar 31, 2016

jonasschnelli commented on the diff Mar 31, 2016

jonasschnelli commented on the diff Mar 31, 2016

jonasschnelli commented Mar 31, 2016

promag commented Apr 2, 2016

jonasschnelli commented Apr 6, 2016

promag commented on the diff Apr 6, 2016

This was referenced Apr 7, 2016

promag added some commits Mar 29, 2016

jonasschnelli commented Apr 13, 2016

btcdrak commented Apr 14, 2016

sipa referenced this pull request Apr 15, 2016

laanwj added a commit that referenced this pull request Apr 15, 2016

laanwj commented Apr 15, 2016 • edited Edited 1 time laanwj Apr 15, 2016

laanwj closed this Apr 15, 2016

promag and 1 other commented on an outdated diff Feb 11, 2016

promag and 1 other commented on an outdated diff Feb 11, 2016

luke-jr and 1 other commented on an outdated diff Feb 11, 2016

luke-jr commented on an outdated diff Feb 11, 2016

luke-jr commented on an outdated diff Feb 13, 2016

jonasschnelli commented on an outdated diff Feb 19, 2016

jonasschnelli commented on an outdated diff Feb 19, 2016

jonasschnelli commented on an outdated diff Feb 19, 2016

jonasschnelli and 1 other commented on an outdated diff Feb 19, 2016

MarcoFalke and 1 other commented on an outdated diff Mar 25, 2016

jonasschnelli commented on an outdated diff Mar 29, 2016

jonasschnelli commented on an outdated diff Mar 30, 2016

laanwj commented Apr 15, 2016 •

Edited 1 time

laanwj
Apr 15, 2016