Skip to content
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

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

Closed

Conversation

petertodd
Copy link
Contributor

Easy first-step for those who are using third-party scripts like my replace-by-fee-tools

@petertodd
Copy link
Contributor Author

There's a good argument for making this default to true - BlockCypher is rejecting Bitcoin Core txs as "might be doublespent", and whether or not the tx fee is high enough to satisfy such services isn't reliable anyway. Defaulting to true also makes it much easier to give people help on fixing a stuck transaction after the fact, even without explicit support in Bitcoin Core yet.

@petertodd petertodd force-pushed the 2015-11-opt-into-full-rbf-option branch from 4f72c8c to 4ab6bbc Compare November 27, 2015 19:05
@TheBlueMatt
Copy link
Contributor

Your usage docs have a copy paste error... It's a binary flag so you want s/=//

On November 27, 2015 1:53:52 PM EST, Peter Todd notifications@github.com wrote:

There's a good argument for making this default to true - BlockCypher
is rejecting Bitcoin Core txs as "might be doublespent", and whether or
not the tx fee is high enough to satisfy such services isn't reliable
anyway. Defaulting to true also makes it much easier to give people
help on fixing a stuck transaction after the fact, even without
explicit support in Bitcoin Core yet.


Reply to this email directly or view it on GitHub:
#7119 (comment)

@luke-jr
Copy link
Member

luke-jr commented Nov 27, 2015

This is kinda ugly (per-node start instead of per-tx). IMO we shouldn't opt-in by default until Core can actually double-spend them itself.

@petertodd petertodd force-pushed the 2015-11-opt-into-full-rbf-option branch from 4ab6bbc to 7de161e Compare November 28, 2015 09:03
@petertodd
Copy link
Contributor Author

@TheBlueMatt Thanks, fixed.

@luke-jr Yeah, per-tx would be nice, but that'll take a lot more work on UX and RPC calls.

Re: default, remember that we don't try to make any guarantees about the likely hood that a tx will be mined according to what the recipient needs re: double-spending. For instance, a low fee tx is very easy to double-spend, and we'll happily produce them. Similarly we'll happily spend unconfirmed change in many circumstances, which again causes txs to get rejected by zeroconf risk analysis's. Not a particularly predictable UX, so i'm not worried about a default to opt-in, which does have benefits. (also the benefit of ensuring opt-in txs are accepted properly by other wallets!)

@instagibbs
Copy link
Member

The opt-in being standard makes it sound more opt-out from the Core user's perspective. I don't care a ton, but it should to be clear from description.

I didn't even quite get that:

There's a good argument for making this default to true

meant that's how it was until I read the code itself.

@sandakersmann
Copy link
Contributor

NACK. This is not Bitcoin.

@sipa
Copy link
Member

sipa commented Nov 28, 2015

I don't think we should make this default.

@TheBlueMatt
Copy link
Contributor

I agree with Luke, it should be a per-tx option, but this should absolutely be the default. The Bitcoin Core wallet should be setting the example of how to best interact with Bitcoin, which means fill RBF eventually, but normalizing txn with RBF enabled until then.

On November 27, 2015 4:10:31 PM EST, Luke-Jr notifications@github.com wrote:

This is kinda ugly (per-node start instead of per-tx). IMO we shouldn't
opt-in by default until Core can actually double-spend them itself.


Reply to this email directly or view it on GitHub:
#7119 (comment)

@sandakersmann
Copy link
Contributor

0-confirmation transactions are important. If people include a too small fee they can wait until the transaction drops from the mempool.

@rnicoll
Copy link
Contributor

rnicoll commented Nov 29, 2015

Having the option is important, but I'd much rather it be per-transaction, and I'm certainly not convinced RBF-enabled should be default. What's the argument for it as default, that it's impossible to be certain of required transaction fee before relaying?

Maybe the default for fee-less transactions only? Aware that's somewhat complicating things.

@TheBlueMatt
Copy link
Contributor

The best way to optimize fee for users is to always use RBF and shoot for a low fee initially. Certainly this is the way we'd go in the future, the question is whether to default txn to opt into RBF before we've implemented the actual fee-bump-RBF logic in wallet or not.

On November 28, 2015 7:02:27 PM EST, Ross Nicoll notifications@github.com wrote:

Having the option is important, but I'd much rather it be
per-transaction, and I'm certainly not convinced RBF-enabled should be
default. What's the argument for it as default, that it's impossible to
be certain of required transaction fee before relaying?

Maybe the default for fee-less transactions only? Aware that's somewhat
complicating things.


Reply to this email directly or view it on GitHub:
#7119 (comment)

@sandakersmann
Copy link
Contributor

Merchants are users too. RBF is not the way to go in the future.

@maaku
Copy link
Contributor

maaku commented Nov 29, 2015

Concept ACK. Branch doesn't currently merge.

@btcdrak
Copy link
Contributor

btcdrak commented Nov 29, 2015

Needs rebase @petertodd

@surg0r
Copy link

surg0r commented Nov 29, 2015

No.

@gmaxwell
Copy link
Contributor

I'm closing this, it's just going to be a magnet for harassment due to people dishonestly misrepresenting what a pullreq means and I can't see us taking this particular patch as is.

@gmaxwell gmaxwell closed this Nov 29, 2015
@@ -49,6 +50,8 @@ static const CAmount DEFAULT_TRANSACTION_MAXFEE = 0.1 * COIN;
static const CAmount MIN_CHANGE = CENT;
//! -txconfirmtarget default
static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 2;
//! -optintofullrbf default
static const bool DEFAULT_OPT_INTO_FULL_RBF = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this make sense as a separate PR with DEFAULT_OPT_INTO_FULL_RBF = false?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This PR is uncontroversial if this value defaults to false since we're just adding command line configuration options.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.