Opt-into-RBF for RPC & bitcoin-tx #9672

luke-jr commented at 10:46 PM on February 2, 2017: member

Rebase of #7159 on top of #9592

luke-jr force-pushed on Feb 2, 2017

in src/rpc/rawtransaction.cpp:None in 5102c87d74 outdated

 376 | @@ -377,7 +377,7 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
 377 |              "      \"data\": \"hex\"      (string, required) The key is \"data\", the value is hex encoded data\n"
 378 |              "      ,...\n"
 379 |              "    }\n"
 380 | -            "3. locktime                  (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"
 381 | +            "4. opt_into_rbf              (boolean, optional, default=" + strprintf("%s", fWalletRbf) + ") Allow this transaction to be replaced by a transaction with heigher fees\n"

MarcoFalke commented at 2:03 AM on February 3, 2017:

rpc/rawtransaction.cpp:380:91: error: ‘fWalletRbf’ was not declared in this scope

luke-jr commented at 4:00 AM on February 3, 2017:

I guess we don't really want -walletrbf to work on rawtx anyway. Fixed

luke-jr force-pushed on Feb 3, 2017

in src/rpc/rawtransaction.cpp:None in ff33f416d6 outdated

 376 | @@ -377,7 +377,7 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
 377 |              "      \"data\": \"hex\"      (string, required) The key is \"data\", the value is hex encoded data\n"
 378 |              "      ,...\n"
 379 |              "    }\n"
 380 | -            "3. locktime                  (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"

ryanofsky commented at 5:51 PM on February 3, 2017:

This deleted line should be restored.

in src/rpc/rawtransaction.cpp:None in ff33f416d6 outdated

 435 |              int64_t seqNr64 = sequenceObj.get_int64();
 436 | -            if (seqNr64 < 0 || seqNr64 > std::numeric_limits<uint32_t>::max())
 437 | +            if (seqNr64 < 0 || seqNr64 > std::numeric_limits<uint32_t>::max()) {
 438 |                  throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, sequence number is out of range");
 439 | -            else
 440 | +            } else if (seqNr64 <= std::numeric_limits<uint32_t>::max() - 2 && request.params.size() > 3 && request.params[3].isFalse()) {

ryanofsky commented at 6:05 PM on February 3, 2017:

I think this if condition would be more readable if it were defined in terms of an "rbfOptOut" variable complementing the existing "rbfOptIn" variable above:

bool rbfOptIn = request.params.size() > 3 && request.params[3].isTrue();
bool rbfOptOut = request.params.size() > 3 && request.params[3].isFalse();

luke-jr commented at 7:27 PM on February 3, 2017:

This condition is being removed to check both options equally.

in src/rpc/rawtransaction.cpp:None in ff33f416d6 outdated

 376 | @@ -377,7 +377,7 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
 377 |              "      \"data\": \"hex\"      (string, required) The key is \"data\", the value is hex encoded data\n"
 378 |              "      ,...\n"
 379 |              "    }\n"
 380 | -            "3. locktime                  (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"
 381 | +            "4. opt_into_rbf              (boolean, optional, default=false) Allow this transaction to be replaced by a transaction with heigher fees\n"

ryanofsky commented at 6:14 PM on February 3, 2017:

Would be nice to exercise in an rpc test.

ryanofsky commented at 6:26 PM on February 3, 2017:

Saying default=false is kind of misleading, because if you actually pass false instead of omitting this, it can cause the "Invalid parameter combination" error to trigger. Maybe:

(boolean, optional, default=null) Whether to signal that this transaction may be replaced by another with higher fees. If false, it is an error if any input specifies an incompatible sequence number.

luke-jr commented at 7:31 PM on February 3, 2017:

When sequences aren't provided, this value does decide them, and defaults to false. I'll combine the two...

in src/wallet/wallet.cpp:None in fe9b812324 outdated

2308 | -    coinControl.fAllowWatchOnly = includeWatching;
2309 | -    coinControl.fOverrideFeeRate = overrideEstimatedFeeRate;
2310 | -    coinControl.nFeeRate = specificFeeRate;
2311 |  
2312 |      BOOST_FOREACH(const CTxIn& txin, tx.vin)
2313 |          coinControl.Select(txin.prevout);

ryanofsky commented at 6:39 PM on February 3, 2017:

You moved almost all the lines that mutate coinControl from FundTransaction to fundrawtransaction except for this one and the "coinControl.fAllowOtherInputs = true" line above. Any reason not to move these as well? This way the coin control object is fully initialized in one place and the argument can be const.

luke-jr commented at 7:29 PM on February 3, 2017:

That'd be doing too much at the caller side IMO.

jnewbery commented at 4:54 PM on April 6, 2017:

I'm confused by this as well. Can you expand a bit on why these particular members shouldn't be set by the caller?

in src/wallet/rpcwallet.cpp:None in d93462689e outdated

2543 | @@ -2544,6 +2544,7 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
2544 |                              "                              Those recipients will receive less bitcoins than you enter in their corresponding amount field.\n"
2545 |                              "                              If no outputs are specified here, the sender pays the fee.\n"
2546 |                              "                                  [vout_index,...]\n"
2547 | +                            "     \"opt_into_rbf\"           (boolean, optional) Allow this transaction to be replaced by a transaction with heigher fees\n"

ryanofsky commented at 6:42 PM on February 3, 2017:

"higher" spelling

in src/bitcoin-tx.cpp:None in 067681f9d2 outdated

 213 | +
 214 | +    // set the nSequence to MAX_INT - 2 (= RBF opt in flag)
 215 | +    int cnt = 0;
 216 | +    for (CTxIn& txin : tx.vin) {
 217 | +        if (strInIdx == "" || cnt == inIdx) {
 218 | +            txin.nSequence = std::numeric_limits<unsigned int>::max() - 2;

ryanofsky commented at 6:50 PM on February 3, 2017:

Unsigned int should be uint32_t.

Also, maybe this shouldn't overwrite sequence numbers if they already signal replacability, e.g.:

txin.nSequence = std::min(txin.nSequence, std::numeric_limits<uint32_t>::max() - 2)

luke-jr force-pushed on Feb 4, 2017

fanquake added the label RPC/REST/ZMQ on Feb 6, 2017

fanquake added the label Wallet on Feb 6, 2017

luke-jr force-pushed on Feb 10, 2017

luke-jr force-pushed on Feb 18, 2017

in src/wallet/wallet.cpp:None in 817c4691c0 outdated

2574 | @@ -2574,9 +2575,10 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2575 |                  // and in the spirit of "smallest possible change from prior
2576 |                  // behavior."
2577 |                  bool rbf = coinControl ? coinControl->signalRbf : fWalletRbf;
2578 | +                const uint32_t nSequence = rbf ? MAX_BIP125_RBF_SEQUENCE : (std::numeric_limits<unsigned int>::max() - 1);

ryanofsky commented at 10:26 PM on February 23, 2017:

Maybe change <unsigned int> to <uint32_t> while you are here.

ryanofsky commented at 10:29 PM on February 23, 2017: member

utACK c781a0400e4f374f8276f699e2954809739f5c15, all the changes since the last review look great!

ryanofsky commented at 6:18 PM on March 24, 2017: member

#9592 is merged now, so this would be good to rebase

in qa/rpc-tests/replace-by-fee.py:594 in c781a0400e outdated

 588 | @@ -586,5 +589,25 @@ def test_prioritised_transactions(self):
 589 |  
 590 |          assert(tx2b_txid in self.nodes[0].getrawmempool())
 591 |  
 592 | +    def test_rpc(self):
 593 | +        us0 = self.nodes[0].listunspent()[0]
 594 | +        ins = [us0];

jnewbery commented at 1:58 PM on April 6, 2017:

suggest you combine this with the line above (and remove trailing semicolon):

ins = [self.nodes[0].listunspent()[0]]

in qa/rpc-tests/replace-by-fee.py:596 in c781a0400e outdated

 588 | @@ -586,5 +589,25 @@ def test_prioritised_transactions(self):
 589 |  
 590 |          assert(tx2b_txid in self.nodes[0].getrawmempool())
 591 |  
 592 | +    def test_rpc(self):
 593 | +        us0 = self.nodes[0].listunspent()[0]
 594 | +        ins = [us0];
 595 | +        outs = {self.nodes[0].getnewaddress() : Decimal(1.0000000)}
 596 | +        rawtx0 = self.nodes[0].createrawtransaction(ins, outs, 0, True)

jnewbery commented at 1:59 PM on April 6, 2017:

nit: perhaps you could use named arguments to make the it clear what 0, True and 0, False arguments do.

in qa/rpc-tests/replace-by-fee.py:600 in c781a0400e outdated

 595 | +        outs = {self.nodes[0].getnewaddress() : Decimal(1.0000000)}
 596 | +        rawtx0 = self.nodes[0].createrawtransaction(ins, outs, 0, True)
 597 | +        rawtx1 = self.nodes[0].createrawtransaction(ins, outs, 0, False)
 598 | +        json0  = self.nodes[0].decoderawtransaction(rawtx0)
 599 | +        json1  = self.nodes[0].decoderawtransaction(rawtx1)
 600 | +        assert_equal(json0["vin"][0]["sequence"], 4294967293)

jnewbery commented at 2:01 PM on April 6, 2017:

suggest you assert directly on the values rather than assign them to a throwaway local variable and assert immediately. eg:

assert_equal(self.nodes[0].decoderawtransaction(rawtx0)["vin"][0]["sequence"], 4294967293)

also, please add a comment explaining the magic 4294967293 and 4294967295 numbers (or better yet, define a MAX_BIP125_RBF_SEQUENCE constant)

in src/rpc/rawtransaction.cpp:346 in c781a0400e outdated

 405 | @@ -404,6 +406,8 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
 406 |          rawTx.nLockTime = nLockTime;
 407 |      }
 408 |  
 409 | +    bool rbfOptIn = request.params.size() > 3 ? request.params[3].isTrue() : false;

jnewbery commented at 4:19 PM on April 6, 2017:

This won't throw an error if the parameter isn't a bool (and silently set rbfOptIn to false).

Prefer to use get_bool() instead of isTrue() to throw an error when the caller accidentally uses a string "true" instead of a bool true.

in src/rpc/rawtransaction.cpp:362 in c781a0400e outdated

 420 | @@ -417,16 +421,24 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
 421 |          if (nOutput < 0)
 422 |              throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, vout must be positive");
 423 |  
 424 | -        uint32_t nSequence = (rawTx.nLockTime ? std::numeric_limits<uint32_t>::max() - 1 : std::numeric_limits<uint32_t>::max());
 425 | +        uint32_t nSequence;
 426 | +        if (rbfOptIn) {

jnewbery commented at 4:46 PM on April 6, 2017:

nit: I think this would be clearer if it was an else if after the if (sequenceObj.isNum()) block

in src/wallet/rpcwallet.cpp:2686 in c781a0400e outdated

2565 | @@ -2564,20 +2566,21 @@ UniValue fundrawtransaction(const JSONRPCRequest& request)
2566 |  
2567 |      RPCTypeCheck(request.params, boost::assign::list_of(UniValue::VSTR));
2568 |  
2569 | -    CTxDestination changeAddress = CNoDestination();
2570 | +    CCoinControl coinControl;
2571 | +    coinControl.destChange = CNoDestination();
2572 |      int changePosition = -1;
2573 | -    bool includeWatching = false;
2574 | +    coinControl.fAllowWatchOnly = false;  // include watching

jnewbery commented at 4:59 PM on April 6, 2017:

I don't think this comment adds any value.

in src/wallet/rpcwallet.cpp:2690 in c781a0400e outdated

2575 |      bool lockUnspents = false;
2576 |      bool reserveChangeKey = true;
2577 | -    CFeeRate feeRate = CFeeRate(0);
2578 | -    bool overrideEstimatedFeerate = false;
2579 | +    coinControl.nFeeRate = CFeeRate(0);
2580 | +    coinControl.fOverrideFeeRate = false;

jnewbery commented at 5:03 PM on April 6, 2017:

The coinControl constructor is already setting all of these members (in CCoinControl::SetNull()). Why are you setting them again here?

in src/wallet/wallet.cpp:2688 in c781a0400e outdated

2573 | @@ -2578,9 +2574,11 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
2574 |                  // to avoid conflicting with other possible uses of nSequence,
2575 |                  // and in the spirit of "smallest possible change from prior
2576 |                  // behavior."
2577 | +                bool rbf = coinControl ? coinControl->signalRbf : fWalletRbf;
2578 | +                const uint32_t nSequence = rbf ? MAX_BIP125_RBF_SEQUENCE : (std::numeric_limits<unsigned int>::max() - 1);

jnewbery commented at 5:09 PM on April 6, 2017:

+1 for using MAX_BIP125_RBF_SEQUENCE instead of the magic subtraction you're replacing.

in src/wallet/wallet.h:938 in c781a0400e outdated

 782 | @@ -783,7 +783,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 783 |       * Insert additional inputs into the transaction by
 784 |       * calling CreateTransaction();
 785 |       */
 786 | -    bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, bool overrideEstimatedFeeRate, const CFeeRate& specificFeeRate, int& nChangePosInOut, std::string& strFailReason, bool includeWatching, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, bool keepReserveKey = true, const CTxDestination& destChange = CNoDestination());
 787 | +    bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl, bool keepReserveKey = true);

jnewbery commented at 5:11 PM on April 6, 2017:

nit: suggest you remove the unused default value for keepReserveKey

jnewbery commented at 5:12 PM on April 6, 2017: member

Needs rebase. A few nits in my review, but otherwise looks good.

You should add some test cases to /test/util/data to test the new functionality in bitcoin-tx

jonasschnelli commented at 6:20 PM on April 6, 2017: contributor

This needs rebase. The QT part is now in master.

laanwj commented at 12:01 PM on April 24, 2017: member

This needs rebase. The QT part is now in master.

Yes, can we please try to move this forward?

RPC: rawtransaction: Add RBF support for createrawtransaction 578ec80d4f

Wallet: Refactor FundTransaction to accept parameters via CCoinControl 891c5eeec2

RPC/Wallet: Add RBF support for fundrawtransaction 36bcab2356

[Tests] extend the replace-by-fee test to cover RPC rawtx features 5d26244148

[bitcoin-tx] add rbfoptin command 575cde4605

Introduce MAX_BIP125_RBF_SEQUENCE constant b005bf21a7

bitcoin-tx: rbfoptin: Avoid touching nSequence if the value is already opting in 23b0fe34f5

RPC/rawtransaction: createrawtransaction: Check opt_into_rbf when provided with either value 9a5a1d7d45

luke-jr force-pushed on Jun 5, 2017

luke-jr commented at 11:28 PM on June 5, 2017: member

Rebased

laanwj commented at 1:31 PM on June 7, 2017: member

utACK 9a5a1d7

laanwj merged this on Jun 7, 2017

laanwj closed this on Jun 7, 2017

laanwj referenced this in commit 46311e792f on Jun 7, 2017

laanwj commented at 1:49 PM on June 7, 2017: member

@luke-jr Looks like you didn't pay attention to @jnewbery's comments at all. I assumed so because two months have passed and you did a rebase since. Can you please fix them up after the fact?

jnewbery commented at 1:55 PM on June 7, 2017: member

huh? This only got a single untested ACK from the maintainer since a large rebase, yet it's already been merged. Most of my comments were nits, but I'm surprised this got merged in without tests covering the new functionality. This is essentially untested code at this point (either by reviewers or test cases).

laanwj commented at 2:01 PM on June 7, 2017: member

Code changes look good to me though. @ryanofsky also utACKed it. It's unfortunate that this moved so slowly, while the GUI functionality was already in there for ages. But yes I had missed that your changes weren't made yet. May be better to revert.

laanwj commented at 2:13 PM on June 7, 2017: member

If so, I'm not sure whether to revert entirely or partially. E.g. https://github.com/bitcoin/bitcoin/pull/9672/commits/b005bf21a7cabe827ef62f266c22adf2ee745b61 that introduces MAX_BIP125_RBF_SEQUENCE is obviously correct and an improvement.

jonasschnelli commented at 3:11 PM on June 7, 2017: contributor

post merge utACK 9a5a1d7d452eff242be1bfe155a30c1f05b2d5a4.

ryanofsky commented at 3:12 PM on June 7, 2017: member

Just my 2 cents, but I don't see why this would be reverted. Personally, when I post suggestions on a PR, my only expectation is that the PR author will take a look at them and implement any that seem to be worthwhile. If the author didn't implement an idea I really liked, I would follow up with my own PR. If I had actual pressing feedback that I thought should hold up a merge, I'd accompany it with some kind of temporary NACK.

laanwj commented at 3:16 PM on June 7, 2017: member

OK - agreement on IRC and here is to not revert this, thanks everyone

MarcoFalke locked this on Sep 8, 2021