[RPC] Add RBF opt-in possibilities to rawtx functions by jonasschnelli · Pull Request #7159 · bitcoin/bitcoin

jonasschnelli · 2015-12-03T09:46:43Z

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

dcousens · 2015-12-03T10:35:34Z

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

petertodd · 2015-12-03T10:35:37Z

src/rpcrawtransaction.cpp

Typo "with height fees"

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

laanwj · 2015-12-03T10:46:44Z

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 · 2015-12-03T12:01:26Z

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 · 2015-12-03T12:06:45Z

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 · 2015-12-03T14:02:26Z

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

gmaxwell · 2015-12-03T14:16:16Z

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 · 2015-12-03T14:23:30Z

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 · 2015-12-03T15:01:36Z

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 · 2015-12-03T15:46:48Z

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 · 2015-12-11T20:55:12Z

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 · 2015-12-22T13:00:16Z

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 · 2015-12-29T01:43:41Z

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

jonasschnelli · 2015-12-29T07:47:10Z

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

dcousens · 2015-12-29T08:02:57Z

src/rpcrawtransaction.cpp

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.

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

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.

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 · 2016-01-11T08:31:13Z

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.

This will allow to add flags for RBF and other features Github-Pull: bitcoin#7159 Rebased-From: cde33d3

Github-Pull: bitcoin#7159 Rebased-From: aeaabd6

Github-Pull: bitcoin#7159 Rebased-From: 3e21c66

Github-Pull: bitcoin#7159 Rebased-From: 00c31dc

Github-Pull: bitcoin#7159 Rebased-From: 475128a

Github-Pull: bitcoin#7159 Rebased-From: cbf2d56

Github-Pull: bitcoin#7159 Rebased-From: e17853f

… input Github-Pull: bitcoin#7159 Rebased-From: 7c73bba

This will allow to add flags for RBF and other features

… input

jonasschnelli · 2016-04-04T07:57:30Z

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 · 2016-04-04T13:36:28Z

ut ACK

jonasschnelli · 2016-04-12T11:46:34Z

Closing in favor of #7865.

jonasschnelli added the RPC/REST/ZMQ label Dec 3, 2015

petertodd reviewed Dec 3, 2015
View reviewed changes

jonasschnelli force-pushed the 2015/12/rpc_rbf branch 2 times, most recently from 34bbc8c to f84a91b Compare December 3, 2015 15:44

jonasschnelli mentioned this pull request Dec 17, 2015

RPC: Indicate which transactions are signaling opt-in RBF #7222

Merged

jonasschnelli force-pushed the 2015/12/rpc_rbf branch 2 times, most recently from d999afe to 7c73bba Compare December 22, 2015 12:59

dcousens reviewed Dec 29, 2015
View reviewed changes

This was referenced Jan 14, 2016

Add NoRBF to URI Scheme #7343

Closed

Add option to opt into full-RBF when sending funds #7132

Closed

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

[Refactor] CreateTransaction(): use bit flags

8940f1b

This will allow to add flags for RBF and other features Github-Pull: bitcoin#7159 Rebased-From: cde33d3

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

Allow to opt-into RBF when creating a transaction

44fedd0

Github-Pull: bitcoin#7159 Rebased-From: aeaabd6

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

[RPC] Add RBF support for createrawtransaction

6cb8b00

Github-Pull: bitcoin#7159 Rebased-From: 3e21c66

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

[RPC] Add RBF support for fundrawtransaction

9bf1530

Github-Pull: bitcoin#7159 Rebased-From: 00c31dc

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

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

08fd0d0

Github-Pull: bitcoin#7159 Rebased-From: 475128a

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

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

ab4fa76

Github-Pull: bitcoin#7159 Rebased-From: cbf2d56

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

[bitcoin-tx] add rbfoptin command

1e6bd57

Github-Pull: bitcoin#7159 Rebased-From: e17853f

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Feb 13, 2016

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

a2d0629

… input Github-Pull: bitcoin#7159 Rebased-From: 7c73bba

luke-jr mentioned this pull request Feb 13, 2016

Add multiple options to fundrawtransaction #7518

Closed

laanwj added this to the 0.13.0 milestone Apr 1, 2016

jonasschnelli added 8 commits April 4, 2016 09:41

[Refactor] CreateTransaction(): use bit flags

bca76d2

This will allow to add flags for RBF and other features

Allow to opt-into RBF when creating a transaction

0df8bd5

[RPC] Add RBF support for createrawtransaction

bd59a05

[RPC] Add RBF support for fundrawtransaction

09499a3

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

3051f53

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

e0bbcf7

[bitcoin-tx] add rbfoptin command

e83ca92

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

b64ebaf

… input

jonasschnelli force-pushed the 2015/12/rpc_rbf branch from 7c73bba to b64ebaf Compare April 4, 2016 07:41

This was referenced Apr 5, 2016

[Qt] Simple opt-in-RBF checkbox #7819

Closed

Add uncontroversial RBF base features #7824

Closed

jonasschnelli closed this Apr 12, 2016

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jun 28, 2016

Merge bitcoin#7159 rpc_rbf-0.12.x_on_7518

d0804de

luke-jr mentioned this pull request Feb 2, 2017

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

Merged

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

Conversation

jonasschnelli commented Dec 3, 2015

Uh oh!

dcousens commented Dec 3, 2015

Uh oh!

petertodd Dec 3, 2015

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Dec 3, 2015

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 3, 2015

Uh oh!

jonasschnelli commented Dec 3, 2015

Uh oh!

laanwj commented Dec 3, 2015

Uh oh!

jgarzik commented Dec 3, 2015

Uh oh!

gmaxwell commented Dec 3, 2015

Uh oh!

laanwj commented Dec 3, 2015

Uh oh!

jonasschnelli commented Dec 3, 2015

Uh oh!

jonasschnelli commented Dec 3, 2015

Uh oh!

morcos commented Dec 11, 2015

Uh oh!

jonasschnelli commented Dec 22, 2015

Uh oh!

dcousens commented Dec 29, 2015

Uh oh!

jonasschnelli commented Dec 29, 2015

Uh oh!

dcousens Dec 29, 2015

Choose a reason for hiding this comment

Uh oh!

dcousens Dec 29, 2015

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Dec 29, 2015

Choose a reason for hiding this comment

Uh oh!

dcousens Dec 29, 2015

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jan 11, 2016

Uh oh!

jonasschnelli commented Apr 4, 2016

Uh oh!

jgarzik commented Apr 4, 2016

Uh oh!

jonasschnelli commented Apr 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants