New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from Jun 7, 2017

Conversation

Projects
None yet
7 participants
@luke-jr
Member

luke-jr commented Feb 2, 2017

Rebase of #7159 on top of #9592

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Feb 3, 2017

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

MarcoFalke Feb 3, 2017

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

This comment has been minimized.

@luke-jr

luke-jr Feb 3, 2017

Member

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

@luke-jr

luke-jr Feb 3, 2017

Member

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

Show outdated Hide outdated src/rpc/rawtransaction.cpp
@@ -377,7 +377,7 @@ UniValue createrawtransaction(const JSONRPCRequest& request)
" \"data\": \"hex\" (string, required) The key is \"data\", the value is hex encoded data\n"
" ,...\n"
" }\n"
"3. locktime (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"

This comment has been minimized.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

This deleted line should be restored.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

This deleted line should be restored.

Show outdated Hide outdated src/rpc/rawtransaction.cpp
throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, sequence number is out of range");
else
} else if (seqNr64 <= std::numeric_limits<uint32_t>::max() - 2 && request.params.size() > 3 && request.params[3].isFalse()) {

This comment has been minimized.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

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

ryanofsky Feb 3, 2017

Contributor

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 comment has been minimized.

@luke-jr

luke-jr Feb 3, 2017

Member

This condition is being removed to check both options equally.

@luke-jr

luke-jr Feb 3, 2017

Member

This condition is being removed to check both options equally.

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

This comment has been minimized.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

Would be nice to exercise in an rpc test.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

Would be nice to exercise in an rpc test.

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

This comment has been minimized.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

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.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

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.

This comment has been minimized.

@luke-jr

luke-jr Feb 3, 2017

Member

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

@luke-jr

luke-jr Feb 3, 2017

Member

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

coinControl.fAllowOtherInputs = true;
coinControl.fAllowWatchOnly = includeWatching;
coinControl.fOverrideFeeRate = overrideEstimatedFeeRate;
coinControl.nFeeRate = specificFeeRate;
BOOST_FOREACH(const CTxIn& txin, tx.vin)
coinControl.Select(txin.prevout);

This comment has been minimized.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

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.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

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.

This comment has been minimized.

@luke-jr

luke-jr Feb 3, 2017

Member

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

@luke-jr

luke-jr Feb 3, 2017

Member

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

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

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

jnewbery Apr 6, 2017

Member

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

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

This comment has been minimized.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

"higher" spelling

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

"higher" spelling

Show outdated Hide outdated src/bitcoin-tx.cpp
int cnt = 0;
for (CTxIn& txin : tx.vin) {
if (strInIdx == "" || cnt == inIdx) {
txin.nSequence = std::numeric_limits<unsigned int>::max() - 2;

This comment has been minimized.

@ryanofsky

ryanofsky Feb 3, 2017

Contributor

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

ryanofsky Feb 3, 2017

Contributor

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!

@@ -2574,9 +2575,10 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
// 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);

This comment has been minimized.

@ryanofsky

ryanofsky Feb 23, 2017

Contributor

Maybe change to <uint32_t> while you are here.

@ryanofsky

ryanofsky Feb 23, 2017

Contributor

Maybe change to <uint32_t> while you are here.

@ryanofsky

This comment has been minimized.

Show comment
Hide comment
@ryanofsky

ryanofsky Mar 24, 2017

Contributor

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

Contributor

ryanofsky commented Mar 24, 2017

#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

Show outdated Hide outdated qa/rpc-tests/replace-by-fee.py
@@ -586,5 +589,25 @@ def test_prioritised_transactions(self):
assert(tx2b_txid in self.nodes[0].getrawmempool())
def test_rpc(self):
us0 = self.nodes[0].listunspent()[0]
ins = [us0];

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

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

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

@jnewbery

jnewbery Apr 6, 2017

Member

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

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

Show outdated Hide outdated 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)

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

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

@jnewbery

jnewbery Apr 6, 2017

Member

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

Show outdated Hide outdated 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)

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

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

jnewbery Apr 6, 2017

Member

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)

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

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

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

jnewbery Apr 6, 2017

Member

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.

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

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

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

@jnewbery

jnewbery Apr 6, 2017

Member

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

coinControl.fAllowOtherInputs = true;
coinControl.fAllowWatchOnly = includeWatching;
coinControl.fOverrideFeeRate = overrideEstimatedFeeRate;
coinControl.nFeeRate = specificFeeRate;
BOOST_FOREACH(const CTxIn& txin, tx.vin)
coinControl.Select(txin.prevout);

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

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

jnewbery Apr 6, 2017

Member

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

int changePosition = -1;
bool includeWatching = false;
coinControl.fAllowWatchOnly = false; // include watching

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

I don't think this comment adds any value.

@jnewbery

jnewbery Apr 6, 2017

Member

I don't think this comment adds any value.

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

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

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

@jnewbery

jnewbery Apr 6, 2017

Member

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

Show outdated Hide outdated src/wallet/wallet.cpp
@@ -2578,9 +2574,11 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
// to avoid conflicting with other possible uses of nSequence,
// 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);

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

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

@jnewbery

jnewbery Apr 6, 2017

Member

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

@@ -783,7 +783,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
* Insert additional inputs into the transaction by
* 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);

This comment has been minimized.

@jnewbery

jnewbery Apr 6, 2017

Member

nit: suggest you remove the unused default value for keepReserveKey

@jnewbery

jnewbery Apr 6, 2017

Member

nit: suggest you remove the unused default value for keepReserveKey

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 6, 2017

Member

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

Member

jonasschnelli commented Apr 6, 2017

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 24, 2017

Member

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

Yes, can we please try to move this forward?

Member

laanwj commented Apr 24, 2017

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

Yes, can we please try to move this forward?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 5, 2017

Member

Rebased

Member

luke-jr commented Jun 5, 2017

Rebased

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 7, 2017

Member

utACK 9a5a1d7

Member

laanwj commented Jun 7, 2017

utACK 9a5a1d7

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 7, 2017

Merge #9672: Opt-into-RBF for RPC & bitcoin-tx
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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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?

Member

laanwj commented Jun 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Jun 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).

Member

jnewbery commented Jun 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 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.

Member

laanwj commented Jun 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 7, 2017

Member

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.

Member

laanwj commented Jun 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 7, 2017

Member

post merge utACK 9a5a1d7.

Member

jonasschnelli commented Jun 7, 2017

post merge utACK 9a5a1d7.

@ryanofsky

This comment has been minimized.

Show comment
Hide comment
@ryanofsky

ryanofsky Jun 7, 2017

Contributor

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.

Contributor

ryanofsky commented Jun 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 7, 2017

Member

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

Member

laanwj commented Jun 7, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment