Opt-into-RBF for RPC & bitcoin-tx by luke-jr · Pull Request #9672 · bitcoin/bitcoin

luke-jr · 2017-02-02T22:46:56Z

Rebase of #7159 on top of #9592

maflcko · 2017-02-03T02:03:43Z

src/rpc/rawtransaction.cpp

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

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

ryanofsky · 2017-02-03T17:51:24Z

src/rpc/rawtransaction.cpp

This deleted line should be restored.

ryanofsky · 2017-02-03T18:05:45Z

src/rpc/rawtransaction.cpp

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

This condition is being removed to check both options equally.

ryanofsky · 2017-02-03T18:14:46Z

src/rpc/rawtransaction.cpp

Would be nice to exercise in an rpc test.

ryanofsky · 2017-02-03T18:26:18Z

src/rpc/rawtransaction.cpp

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.

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

ryanofsky · 2017-02-03T18:39:48Z

src/wallet/wallet.cpp

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.

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

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

ryanofsky · 2017-02-03T18:42:10Z

src/wallet/rpcwallet.cpp

"higher" spelling

ryanofsky · 2017-02-03T18:50:52Z

src/bitcoin-tx.cpp

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)

ryanofsky

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

ryanofsky · 2017-02-23T22:26:23Z

src/wallet/wallet.cpp

                // and in the spirit of "smallest possible change from prior
                // behavior."
                bool rbf = coinControl ? coinControl->signalRbf : fWalletRbf;
+                const uint32_t nSequence = rbf ? MAX_BIP125_RBF_SEQUENCE : (std::numeric_limits<unsigned int>::max() - 1);


Maybe change to <uint32_t> while you are here.

ryanofsky · 2017-03-24T18:18:47Z

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

jnewbery

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

jnewbery · 2017-04-06T13:58:51Z

qa/rpc-tests/replace-by-fee.py


+    def test_rpc(self):
+        us0 = self.nodes[0].listunspent()[0]
+        ins = [us0];


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

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

jnewbery · 2017-04-06T13:59:56Z

qa/rpc-tests/replace-by-fee.py

+        us0 = self.nodes[0].listunspent()[0]
+        ins = [us0];
+        outs = {self.nodes[0].getnewaddress() : Decimal(1.0000000)}
+        rawtx0 = self.nodes[0].createrawtransaction(ins, outs, 0, True)


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

jnewbery · 2017-04-06T14:01:50Z

qa/rpc-tests/replace-by-fee.py

+        rawtx1 = self.nodes[0].createrawtransaction(ins, outs, 0, False)
+        json0  = self.nodes[0].decoderawtransaction(rawtx0)
+        json1  = self.nodes[0].decoderawtransaction(rawtx1)
+        assert_equal(json0["vin"][0]["sequence"], 4294967293)


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)

jnewbery · 2017-04-06T16:19:35Z

src/rpc/rawtransaction.cpp

        rawTx.nLockTime = nLockTime;
    }

+    bool rbfOptIn = request.params.size() > 3 ? request.params[3].isTrue() : false;


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.

jnewbery · 2017-04-06T16:46:17Z

src/rpc/rawtransaction.cpp


-        uint32_t nSequence = (rawTx.nLockTime ? std::numeric_limits<uint32_t>::max() - 1 : std::numeric_limits<uint32_t>::max());
+        uint32_t nSequence;
+        if (rbfOptIn) {


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

jnewbery · 2017-04-06T16:54:19Z

src/wallet/wallet.cpp

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

jnewbery · 2017-04-06T16:59:39Z

src/wallet/rpcwallet.cpp

+    coinControl.destChange = CNoDestination();
    int changePosition = -1;
-    bool includeWatching = false;
+    coinControl.fAllowWatchOnly = false;  // include watching


I don't think this comment adds any value.

jnewbery · 2017-04-06T17:03:07Z

src/wallet/rpcwallet.cpp

-    CFeeRate feeRate = CFeeRate(0);
-    bool overrideEstimatedFeerate = false;
+    coinControl.nFeeRate = CFeeRate(0);
+    coinControl.fOverrideFeeRate = false;


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

jnewbery · 2017-04-06T17:09:47Z

src/wallet/wallet.cpp

                // and in the spirit of "smallest possible change from prior
                // behavior."
+                bool rbf = coinControl ? coinControl->signalRbf : fWalletRbf;
+                const uint32_t nSequence = rbf ? MAX_BIP125_RBF_SEQUENCE : (std::numeric_limits<unsigned int>::max() - 1);


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

jnewbery · 2017-04-06T17:11:29Z

src/wallet/wallet.h

     * calling CreateTransaction();
     */
-    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());
+    bool FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl, bool keepReserveKey = true);


nit: suggest you remove the unused default value for keepReserveKey

jonasschnelli · 2017-04-06T18:20:02Z

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

laanwj · 2017-04-24T12:01:12Z

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

Yes, can we please try to move this forward?

…y opting in

…vided with either value

luke-jr · 2017-06-05T23:28:18Z

Rebased

laanwj · 2017-06-07T13:31:36Z

utACK 9a5a1d7

9a5a1d7 RPC/rawtransaction: createrawtransaction: Check opt_into_rbf when provided with either value (Luke Dashjr) 23b0fe3 bitcoin-tx: rbfoptin: Avoid touching nSequence if the value is already opting in (Luke Dashjr) b005bf2 Introduce MAX_BIP125_RBF_SEQUENCE constant (Luke Dashjr) 575cde4 [bitcoin-tx] add rbfoptin command (Jonas Schnelli) 5d26244 [Tests] extend the replace-by-fee test to cover RPC rawtx features (Jonas Schnelli) 36bcab2 RPC/Wallet: Add RBF support for fundrawtransaction (Luke Dashjr) 891c5ee Wallet: Refactor FundTransaction to accept parameters via CCoinControl (Luke Dashjr) 578ec80 RPC: rawtransaction: Add RBF support for createrawtransaction (Luke Dashjr) Tree-SHA512: 446e37c617c188cc3b3fd1e2841c98eda6f4869e71cb3249c4a9e54002607d0f1e6bef92187f7894d4e0746ab449cfee89be9f6a1a8831e25c70cf912eac1570

laanwj · 2017-06-07T13:49:36Z

@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 · 2017-06-07T13:55:55Z

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 · 2017-06-07T14:01:41Z

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 · 2017-06-07T14:13:13Z

If so, I'm not sure whether to revert entirely or partially.
E.g. b005bf2 that introduces MAX_BIP125_RBF_SEQUENCE is obviously correct and an improvement.

jonasschnelli · 2017-06-07T15:11:21Z

post merge utACK 9a5a1d7.

ryanofsky · 2017-06-07T15:12:19Z

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 · 2017-06-07T15:16:08Z

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

luke-jr force-pushed the rpc_rbf branch from 82f6bdc to 5102c87 Compare February 2, 2017 22:59

maflcko reviewed Feb 3, 2017

View reviewed changes

luke-jr force-pushed the rpc_rbf branch from 5102c87 to 067681f Compare February 3, 2017 03:59

ryanofsky reviewed Feb 3, 2017

View reviewed changes

luke-jr force-pushed the rpc_rbf branch from 5eb9240 to bae478d Compare February 4, 2017 06:17

fanquake added RPC/REST/ZMQ Wallet labels Feb 6, 2017

luke-jr force-pushed the rpc_rbf branch from bae478d to b956a95 Compare February 10, 2017 08:06

luke-jr force-pushed the rpc_rbf branch from b956a95 to c781a04 Compare February 18, 2017 00:31

ryanofsky reviewed Feb 23, 2017

View reviewed changes

ryanofsky mentioned this pull request Mar 9, 2017

Refactor Bumpfee, move core functionality to CWallet #9681

Merged

jnewbery reviewed Apr 6, 2017

View reviewed changes

luke-jr mentioned this pull request Apr 8, 2017

Fix opt-in RBF reliance on compiler integer size #10172

Closed

luke-jr and others added 8 commits June 5, 2017 23:09

RPC: rawtransaction: Add RBF support for createrawtransaction

578ec80

Wallet: Refactor FundTransaction to accept parameters via CCoinControl

891c5ee

RPC/Wallet: Add RBF support for fundrawtransaction

36bcab2

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

5d26244

[bitcoin-tx] add rbfoptin command

575cde4

Introduce MAX_BIP125_RBF_SEQUENCE constant

b005bf2

bitcoin-tx: rbfoptin: Avoid touching nSequence if the value is alread…

23b0fe3

…y opting in

RPC/rawtransaction: createrawtransaction: Check opt_into_rbf when pro…

9a5a1d7

…vided with either value

luke-jr force-pushed the rpc_rbf branch from c781a04 to 9a5a1d7 Compare June 5, 2017 23:27

laanwj merged commit 9a5a1d7 into bitcoin:master Jun 7, 2017

laanwj mentioned this pull request Jun 28, 2017

Enable RBF transactions in wallet by default #9527

Closed

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

Conversation

luke-jr commented Feb 2, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Mar 24, 2017

Uh oh!

jnewbery left a comment

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!

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 6, 2017

Uh oh!

laanwj commented Apr 24, 2017

Uh oh!

luke-jr commented Jun 5, 2017

Uh oh!

laanwj commented Jun 7, 2017

Uh oh!

laanwj commented Jun 7, 2017

Uh oh!

jnewbery commented Jun 7, 2017

Uh oh!

laanwj commented Jun 7, 2017

Uh oh!

laanwj commented Jun 7, 2017

Uh oh!

jonasschnelli commented Jun 7, 2017

Uh oh!

ryanofsky commented Jun 7, 2017

Uh oh!

laanwj commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

laanwj commented Jun 7, 2017 •

edited

Loading