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

jonasschnelli commented at 9:46 am on December 3, 2015: contributor

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

API changes:

Bitcoin-Tx

jonasschnelli added the label RPC on Dec 3, 2015

dcousens commented at 10:35 am on December 3, 2015: contributor

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

in src/rpcrawtransaction.cpp: in f2a24f846c outdated

342@@ -343,6 +343,8 @@ UniValue createrawtransaction(const UniValue& params, bool fHelp)
343             "      ...\n"
344             "    }\n"
345             "3. locktime                (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"
346+            "4. opt into RBF            (boolean, optional, default=false) Allow this transaction to be replaced by a transaction with height fees\n"

petertodd commented at 10:35 am on December 3, 2015:

Typo “with height fees”

jonasschnelli commented at 10:36 am on December 3, 2015:

Sorry.. meant to be “higher fees”. Will fix.

laanwj commented at 10:46 am on December 3, 2015: member

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 commented at 12:01 pm on December 3, 2015: contributor

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 commented at 12:06 pm on December 3, 2015: member

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 commented at 2:02 pm on December 3, 2015: contributor

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

gmaxwell commented at 2:16 pm on December 3, 2015: contributor

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 commented at 2:23 pm on December 3, 2015: member

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 commented at 3:01 pm on December 3, 2015: contributor

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 force-pushed on Dec 3, 2015

jonasschnelli commented at 3:46 pm on December 3, 2015: contributor

Added two bitcoin-tx related commits.

Allow to provide a sequence number (optional) for the in command (in=TXID:VOUT(:SEQUENCE_NUMBER))
Add a rbfoptin(=N) command (mutate a tx and set the input N’s [or all] RBF flag)

morcos commented at 8:55 pm on December 11, 2015: 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.

jonasschnelli force-pushed on Dec 22, 2015

jonasschnelli commented at 1:00 pm on December 22, 2015: contributor

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 commented at 1:43 am on December 29, 2015: contributor

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

jonasschnelli commented at 7:47 am on December 29, 2015: contributor

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

in src/rpcrawtransaction.cpp: in 7c73bbadc4 outdated

343@@ -343,6 +344,8 @@ UniValue createrawtransaction(const UniValue& params, bool fHelp)
344             "      ...\n"
345             "    }\n"
346             "3. locktime                (numeric, optional, default=0) Raw locktime. Non-0 value also locktime-activates inputs\n"
347+            "4. opt into RBF            (boolean, optional, default=false) Allow this transaction to be replaced by a transaction with heigher fees\n"
348+

dcousens commented at 8:02 am on December 29, 2015:

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 commented at 8:04 am on December 29, 2015:

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

jonasschnelli commented at 8:24 am on December 29, 2015:

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 commented at 8:26 am on December 29, 2015:

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 commented at 8:31 am on January 11, 2016: contributor

Here again the API changes:

API changes:

Bitcoin-Tx

Thanks for reviews.

luke-jr referenced this in commit 8940f1bee1 on Feb 13, 2016

luke-jr referenced this in commit 44fedd0774 on Feb 13, 2016

luke-jr referenced this in commit 6cb8b00be5 on Feb 13, 2016

luke-jr referenced this in commit 9bf15309a9 on Feb 13, 2016

luke-jr referenced this in commit 08fd0d00bb on Feb 13, 2016

luke-jr referenced this in commit ab4fa76e69 on Feb 13, 2016

luke-jr referenced this in commit 1e6bd57444 on Feb 13, 2016

luke-jr referenced this in commit a2d0629a71 on Feb 13, 2016

laanwj added this to the milestone 0.13.0 on Apr 1, 2016

[Refactor] CreateTransaction(): use bit flags

This will allow to add flags for RBF and other features

bca76d2875

Allow to opt-into RBF when creating a transaction 0df8bd5b50

[RPC] Add RBF support for createrawtransaction bd59a050f1

[RPC] Add RBF support for fundrawtransaction 09499a308a

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

[bitcoin-tx] allow to set nSequence number over the in= command e0bbcf76e8

[bitcoin-tx] add rbfoptin command e83ca92b7e

[RPC] createrawtransaction: add option to set the sequence number per input b64ebaf056

jonasschnelli force-pushed on Apr 4, 2016

jonasschnelli commented at 7:57 am on April 4, 2016: contributor

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 commented at 1:36 pm on April 4, 2016: contributor

ut ACK

jonasschnelli commented at 11:46 am on April 12, 2016: contributor

Closing in favor of #7865.

jonasschnelli closed this on Apr 12, 2016

luke-jr referenced this in commit 629513c518 on Jun 27, 2016

luke-jr referenced this in commit d0804def2d on Jun 28, 2016

MarcoFalke locked this on Sep 8, 2021

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