Skip to content

Add option to opt into full-RBF when sending funds (rebase, original by petertodd) #8601

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 3 commits into from
Sep 13, 2016
Merged

Add option to opt into full-RBF when sending funds (rebase, original by petertodd) #8601

laanwj merged 3 commits into from
Sep 13, 2016

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Aug 26, 2016

Rebases #7132
Also adds the comment for #7132 (comment)

Original pull has plenty of ACKs. After all this time we still have no way to opt in to RBF in the wallet, let's not try to bikeshed it to death this time.

@laanwj laanwj mentioned this pull request Aug 26, 2016
Closed
@laanwj laanwj changed the title Add option to opt into full-RBF when sending funds (rebase) Add option to opt into full-RBF when sending funds (rebase, original by petertodd) Aug 26, 2016
maflcko
maflcko reviewed Aug 26, 2016
View reviewed changes
src/wallet/wallet.cpp Outdated
Copy link
Member

@maflcko maflcko Aug 26, 2016

Choose a reason for hiding this comment

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

I don't think this is valid cpp 😛

@laanwj laanwj force-pushed the 2016_08_full_rbf_option branch from 0c87520 to 05fa823 Compare August 26, 2016 10:57
@luke-jr
Copy link
Member

luke-jr commented Aug 27, 2016

I thought the commandline option was to be renamed to -walletrbf?

@dcousens
Copy link
Contributor

dcousens commented Aug 29, 2016 •
edited
Loading

utACK 05fa823

@petertodd
Copy link
Contributor

petertodd commented Aug 31, 2016

utACK

@laanwj
Copy link
Member Author

laanwj commented Sep 1, 2016

I thought the commandline option was to be renamed to -walletrbf?

Fine with me.

@paveljanik
Copy link
Contributor

paveljanik commented Sep 1, 2016

Why not rename the DEFAULT_.. as well to match the option name?

@luke-jr
Copy link
Member

luke-jr commented Sep 1, 2016

utACK b54c36703c4023114b05305b2edf15db88746fef as-is

http://codepad.org/fhQWSuAk could be included to address @paveljanik's suggestion

@laanwj
Rename -optintofullrbf option to -walletrbf
86726d8 
This makes it clear that this is a wallet option.
@laanwj laanwj force-pushed the 2016_08_full_rbf_option branch from b54c367 to 86726d8 Compare September 13, 2016 09:32
@laanwj
Copy link
Member Author

laanwj commented Sep 13, 2016

Squashed in @luke-jr's patch

@laanwj laanwj merged commit 86726d8 into bitcoin:master Sep 13, 2016
laanwj added a commit that referenced this pull request Sep 13, 2016
@laanwj
Merge #8601: Add option to opt into full-RBF when sending funds (reba…
37ac678 
…se, original by petertodd)

86726d8 Rename `-optintofullrbf` option to `-walletrbf` (Wladimir J. van der Laan)
05fa823 wallet: Add BIP125 comment for MAXINT-1/-2 behavior (Wladimir J. van der Laan)
152f45b Add option to opt into full-RBF when sending funds (Peter Todd)
rebroad
rebroad reviewed Sep 15, 2016
View reviewed changes
src/wallet/wallet.h
//! -txconfirmtarget default
static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 2;
//! -walletrbf default
static const bool DEFAULT_WALLET_RBF = false;
Copy link
Contributor

@rebroad rebroad Sep 15, 2016

Choose a reason for hiding this comment

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

why false by default?

@maflcko
Copy link
Member

maflcko commented Sep 15, 2016

Please see the linked pull in the description

On Thu, Sep 15, 2016 at 4:19 AM, R E Broadley [email protected]
wrote:

@rebroad commented on this pull request.

In src/wallet/wallet.h
#8601 (review):

@@ -53,6 +54,8 @@ static const bool DEFAULT_SPEND_ZEROCONF_CHANGE = true;
static const bool DEFAULT_SEND_FREE_TRANSACTIONS = false;
//! -txconfirmtarget default
static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 2;
+//! -walletrbf default
+static const bool DEFAULT_WALLET_RBF = false;

why false by default?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#8601 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AGGmv74fpRaN3BoDZKy7UTc-EN_RqpRnks5qqKs7gaJpZM4Jt8HS
.

@petertodd petertodd mentioned this pull request Sep 21, 2016
Closed
@maflcko maflcko mentioned this pull request Nov 28, 2016
Closed
@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.

Reviewers

1 more reviewer

@rebroad rebroad rebroad left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@laanwj @luke-jr @dcousens @petertodd @paveljanik @maflcko @rebroad