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

Merged
merged 3 commits into from Sep 13, 2016

Projects

None yet

7 participants

@laanwj
Member
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 changed the title from Add option to opt into full-RBF when sending funds (rebase) to Add option to opt into full-RBF when sending funds (rebase, original by petertodd) Aug 26, 2016
@MarcoFalke MarcoFalke commented on an outdated diff Aug 26, 2016
src/wallet/wallet.cpp
@@ -2355,11 +2356,17 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt
// Fill vin
//
- // Note how the sequence number is set to max()-1 so that the
- // nLockTime set above actually works.
+ // Note how the sequence number is set to non-maxint so that
+ // the nLockTime set above actually works.
+ /
@MarcoFalke
MarcoFalke Aug 26, 2016 Member

I don't think this is valid cpp 😛

@luke-jr
Member
luke-jr commented Aug 27, 2016

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

@dcousens
Contributor
dcousens commented Aug 29, 2016 edited

utACK 05fa823

@petertodd
Member

utACK

@laanwj
Member
laanwj commented Sep 1, 2016

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

Fine with me.

@paveljanik
Contributor

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

@luke-jr
Member
luke-jr commented Sep 1, 2016

utACK b54c367 as-is

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

@laanwj laanwj Rename `-optintofullrbf` option to `-walletrbf`
This makes it clear that this is a wallet option.
86726d8
@laanwj
Member
laanwj commented Sep 13, 2016

Squashed in @luke-jr's patch

@laanwj laanwj merged commit 86726d8 into bitcoin:master Sep 13, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@laanwj laanwj added a commit that referenced this pull request Sep 13, 2016
@laanwj laanwj Merge #8601: Add option to opt into full-RBF when sending funds (reba…
…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)
37ac678
@@ -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;
@rebroad
rebroad Sep 15, 2016 Contributor

why false by default?

@MarcoFalke
Member

Please see the linked pull in the description

On Thu, Sep 15, 2016 at 4:19 AM, R E Broadley notifications@github.com
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
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment