[RPC] Add RBF opt-in possibilities to rawtx functions #7159

Closed
wants to merge 8 commits into
from

Projects

None yet

7 participants

@jonasschnelli
Member

This allows to opt into RBF over the RPCs createrawtransaction (wallet-less) and fundrawtransaction (requires wallet).

API changes:

createrawtransaction [{\"txid\":\"id\",\"vout\":n, \"sequence\":},...] 
{\"address\":amount,\"data\":\"hex\",...} ( locktime ) ( opt into Replace-By-Fee )[bool]
fundrawtransaction \"hexstring\" ( includeWatching ) ( opt into Replace-By-Fee )[bool]

Bitcoin-Tx

in=TXID:VOUT(:SEQUENCE_NUMBER)
rbfoptin(=N)  Set RBF opt-in sequence number for input N (if not provided, opt-in all available inputs
@jonasschnelli jonasschnelli added the RPC label Dec 3, 2015
@dcousens
Contributor
dcousens commented Dec 3, 2015

Perhaps just parameterize the setting of sequence on inputs? That way we can do other things too?

@petertodd petertodd and 1 other commented on an outdated diff Dec 3, 2015
src/rpcrawtransaction.cpp
@@ -343,6 +343,8 @@ UniValue createrawtransaction(const UniValue& params, bool fHelp)
" ...\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 height fees\n"
@petertodd
petertodd Dec 3, 2015 Member

Typo "with height fees"

@jonasschnelli
jonasschnelli Dec 3, 2015 Member

Sorry.. meant to be "higher fees". Will fix.

@laanwj
Member
laanwj commented Dec 3, 2015

I tend to agree with @dcousens, maybe this is slightly more 'user friendly', but allowing setting the underlying values is more in the spirit of raw transaction usage.
Also bitcoin-tx needs a similar change.

@jonasschnelli
Member

Perhaps just parameterize the setting of sequence on inputs? That way we can do other things too?

For createrawtransaction this could make sense, although, I think both should be possible (a nSequence per input as well as a general RBF-opt-in). I prefer exposing relevant features (RBF is relevant IMO) over a clear interface. Manual setting the nSequence to int.max-2 is not user friendly and can be misunderstood (root cause is maybe the way how we use/extend existing consensus fields like nSequence).

For fundrawtransaction is disagree with exposing nSequence instead of a RFB-opt-in bool. Also i don't think we should pass around a vector of sequence number (per input) to CreateTransaction() (or similar).

Also bitcoin-tx needs a similar change.

Right. There I guess instead of a RFB opt-in we could offer a easy way to set the nSequence number per input.

@laanwj
Member
laanwj commented Dec 3, 2015

Sure, for fundrawtransaction it is different, as that is a wallet call (so the wallet will determine inputs for you). My comment just applies to createrawtransaction.

@jgarzik
Member
jgarzik commented Dec 3, 2015

Don't extend createrawtransaction without also extending bitcoin-tx -- that's the preferred place for pure function work unconnected to wallet.

@gmaxwell
Member
gmaxwell commented Dec 3, 2015

Anti-sniping (which I think createrawtransaction should do by default) is an argument against bitcoin-tx; even absent that: the complete application of createrawtransaction is not pure function-- you had to get the txids for the coins from somewhere... but sure this should have parity in bitcoin-tx.

I think createrawtransaction should gain the ability to manually set sequences per input. No more parameters are needed: just add "seq" to the dictionary that takes the vin/txid fields now.

I don't see any harm in having a replacable flag that sets the default sequence to MAX-2.

@laanwj
Member
laanwj commented Dec 3, 2015

Anti-sniping (which I think createrawtransaction should do by default) is an argument against bitcoin-tx;

It's not an argument against bitcoin-tx. A program can get the current block number from anywhere and pass it to createrawtransaction or bitcoin-tx alike. No need to create a tight coupling. Some parts of transaction creation are not pure function, that doesn't mean that steps in the process cannot be.

@jonasschnelli
Member

IIRC createrawtransaction does not do the nLockTime anti-sniping (only CWallet::CreateTransaction() = RPC sendto* or fundrawtransaction does).

I agree with @jgarzik that bitcoin-tx should also support nSequence and/or RBF opt-in. I check now if it makes sense to extend bitcoin-tx within this PR (or if it require to much work in terms or parameter parsing and syntax).

@jonasschnelli
Member

Added two bitcoin-tx related commits.

  1. Allow to provide a sequence number (optional) for the in command (in=TXID:VOUT(:SEQUENCE_NUMBER))
  2. Add a rbfoptin(=N) command (mutate a tx and set the input N's [or all] RBF flag)
@morcos
Contributor
morcos commented Dec 11, 2015

I think createrawtransaction should gain the ability to manually set sequences per input. No more parameters are needed: just add "seq" to the dictionary that takes the vin/txid fields now.

Are you going to add this? I think its a good idea.

@jonasschnelli
Member

I think createrawtransaction should gain the ability to manually set sequences per input. No more parameters are needed: just add "seq" to the dictionary that takes the vin/txid fields now.

Are you going to add this? I think its a good idea.

Rebased & added a commit that allows to set the sequence per input when using createrawtransaction.

@dcousens
Contributor

@jonasschnelli what is the syntax OOI? (haven't had a chance to read the impl. yet)

@jonasschnelli
Member

@dcousens: The inputs object in createrawtransation now can have a sequence-value next to txid and vout.

@dcousens dcousens and 1 other commented on an outdated diff Dec 29, 2015
src/rpcrawtransaction.cpp
@@ -343,6 +344,8 @@ UniValue createrawtransaction(const UniValue& params, bool fHelp)
" ...\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"
+
@dcousens
dcousens Dec 29, 2015 Contributor

This really isn't necessary, the RBF rules are very clear such that if sequence is specified at all (aka, < 0xffffffff - 2?), then it will be used.
Its as much work by the user to just add the sequence field per input, so we could just omit this opt-in RBF parameter entirely.

@dcousens
dcousens Dec 29, 2015 Contributor

It would also be less confusing what rbfOptIn does in a context of it being disabled on the CLI.

@jonasschnelli
jonasschnelli Dec 29, 2015 Member

But is setting all inputs sequence to INT-MAX-2 an adequate API for creating a transaction that is suitable for RBF? IMO a clear Boolean should be preferred.

@dcousens
dcousens Dec 29, 2015 Contributor

But is setting all inputs sequence to INT-MAX-2 an adequate API for creating a transaction that is suitable for RBF?

IMHO, yes.

@jonasschnelli
Member

Here again the API changes:

API changes:

createrawtransaction [{\"txid\":\"id\",\"vout\":n, \"sequence\":},...] 
{\"address\":amount,\"data\":\"hex\",...} ( locktime ) ( opt into Replace-By-Fee )[bool]
fundrawtransaction \"hexstring\" ( includeWatching ) ( opt into Replace-By-Fee )[bool]

Bitcoin-Tx

in=TXID:VOUT(:SEQUENCE_NUMBER)
rbfoptin(=N)  Set RBF opt-in sequence number for input N (if not provided, opt-in all available inputs

Thanks for reviews.

@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Refactor] CreateTransaction(): use bit flags
This will allow to add flags for RBF and other features

Github-Pull: #7159
Rebased-From: cde33d3
8940f1b
@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr Allow to opt-into RBF when creating a transaction
Github-Pull: #7159
Rebased-From: aeaabd6
44fedd0
@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [RPC] Add RBF support for createrawtransaction
Github-Pull: #7159
Rebased-From: 3e21c66
6cb8b00
@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [RPC] Add RBF support for fundrawtransaction
Github-Pull: #7159
Rebased-From: 00c31dc
9bf1530
@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Tests] extend the replace-by-fee test to cover RPC rawtx features
Github-Pull: #7159
Rebased-From: 475128a
08fd0d0
@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [bitcoin-tx] allow to set nSequence number over the in= command
Github-Pull: #7159
Rebased-From: cbf2d56
ab4fa76
@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [bitcoin-tx] add rbfoptin command
Github-Pull: #7159
Rebased-From: e17853f
1e6bd57
@luke-jr luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [RPC] createrawtransaction: add option to set the sequence number per…
… input

Github-Pull: #7159
Rebased-From: 7c73bba
a2d0629
@laanwj laanwj added this to the 0.13.0 milestone Apr 1, 2016
jonasschnelli added some commits Dec 3, 2015
@jonasschnelli jonasschnelli [Refactor] CreateTransaction(): use bit flags
This will allow to add flags for RBF and other features
bca76d2
@jonasschnelli jonasschnelli Allow to opt-into RBF when creating a transaction 0df8bd5
@jonasschnelli jonasschnelli [RPC] Add RBF support for createrawtransaction bd59a05
@jonasschnelli jonasschnelli [RPC] Add RBF support for fundrawtransaction 09499a3
@jonasschnelli jonasschnelli [Tests] extend the replace-by-fee test to cover RPC rawtx features 3051f53
@jonasschnelli jonasschnelli [bitcoin-tx] allow to set nSequence number over the in= command e0bbcf7
@jonasschnelli jonasschnelli [bitcoin-tx] add rbfoptin command e83ca92
@jonasschnelli jonasschnelli [RPC] createrawtransaction: add option to set the sequence number per…
… input
b64ebaf
@jonasschnelli
Member

Rebased.

IMO the fundrawtransaction and the bitcoin-tx are okay.
The only question is if we want the higher level function in createrawtransaction (setting the sequence-no per input is possible now).

@jgarzik
Member
jgarzik commented Apr 4, 2016

ut ACK

@jonasschnelli
Member

Closing in favor of #7865.

@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016
@luke-jr luke-jr Merge #7159 rpc_rbf-0.12.x_on_7518 d0804de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment