Skip to content

Remove unused feebumper code#24562

Merged
fanquake merged 1 commit intobitcoin:masterfrom
maflcko:2203-remFeeBumpCode-🐇
Mar 23, 2022

Hidden character warning

The head ref may contain hidden characters: "2203-remFeeBumpCode-\ud83d\udc07"
Merged

Remove unused feebumper code#24562
fanquake merged 1 commit intobitcoin:masterfrom
maflcko:2203-remFeeBumpCode-🐇

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Mar 14, 2022 •
edited
Loading

This was accidentally added in commit 0ea47ba. Presumably due to a copy-paste error, as CreateTransaction already takes care of the rbf-signal.

@maflcko
Copy link
Member Author

maflcko commented Mar 14, 2022

Can be tested in the GUI Console with:

bumpfee $txid '{"replaceable": $bool_val}'

promag
promag approved these changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Code review ACK fae5d06

Indeed, input sequence is set in CreateTransactionInternal:

bitcoin/src/wallet/spend.cpp

Lines 805 to 813 in 7b83c7d

// BIP125 defines opt-in RBF as any nSequence < maxint-1, so
// we use the highest possible value in that range (maxint-2)
// to avoid conflicting with other possible uses of nSequence,
// and in the spirit of "smallest possible change from prior
// behavior."
const uint32_t nSequence{coin_control.m_signal_bip125_rbf.value_or(wallet.m_signal_rbf) ? MAX_BIP125_RBF_SEQUENCE : CTxIn::MAX_SEQUENCE_NONFINAL};
for (const auto& coin : selected_coins) {
txNew.vin.push_back(CTxIn(coin.outpoint, CScript(), nSequence));
}

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 22, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #20640 (wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@achow101
Copy link
Member

achow101 commented Mar 23, 2022

ACK fae5d06

@fanquake
Copy link
Member

fanquake commented Mar 23, 2022

cc @instagibbs

@fanquake fanquake merged commit cea230e into bitcoin:master Mar 23, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 24, 2022
@fanquake
Merge bitcoin#24562: Remove unused feebumper code
ddbd96a
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 24, 2022
Revert "Merge bitcoin#24562: Remove unused feebumper code"
8c83ce1 
This reverts commit ddbd96a.
@DrahtBot DrahtBot mentioned this pull request Mar 24, 2022
Merged
@maflcko maflcko deleted the 2203-remFeeBumpCode-🐇 branch March 24, 2022 07:13
@bitcoin bitcoin locked and limited conversation to collaborators Mar 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@promag promag promag approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@maflcko @DrahtBot @achow101 @fanquake @promag