Opt-into-RBF for RPC & bitcoin-tx

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: 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:

0rpc/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: 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: 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:

0bool rbfOptIn = request.params.size() > 3 && request.params[3].isTrue();
1bool 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: 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: 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: 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: 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: 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 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

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