Partially fixes #32661.
This follows up a point raised in a review of a previous PR here: #34917 (review).
With nodes running fullrbf, wallet doesn't need to opt into RBF. This patch switches the default of wallet rbf to false. Users still have the option to optin RBF via the individual transaction (and PSBT) creation RPCs - for now.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35405.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | polespinasa, xyzconstant |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
https://github.com/bitcoin/bitcoin/actions/runs/26580689146/job/78312508574?pr=35405#step:9:8958
AssertionError: not(4294967293 == 4294967294)
This failing error would be fixed after #35381 is merged, putting this in draft until then.
0 | @@ -0,0 +1,10 @@ 1 | +Wallet 2 | +------------ 3 | +Post deprecation of the `-walletrbf` startup option, the default 4 | +value instead become effective in case the `replaceable` flag is 5 | +not passed in the transaction and PSBT creations RPCs. Now that 6 | +the bitcoin nodes have fullrbf, this default value is switched 7 | +from `true` to `false` because signalling for RBF is not necessary 8 | +for replacing transactions and the wallet doesn't need to opt 9 | +into it by default.
Maybe this section can be merged into the existing Wallet RPC snippet (doc/release-notes-34917.md)?
Otherwise, it may be a bit confusing to have two overlapping sections talking about the same thing.
Yes, done.
127 | @@ -128,7 +128,7 @@ static const bool DEFAULT_WALLET_REJECT_LONG_CHAINS{true}; 128 | //! -txconfirmtarget default 129 | static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 6; 130 | //! -walletrbf default 131 | -static const bool DEFAULT_WALLET_RBF = true; 132 | +static const bool DEFAULT_WALLET_RBF = false;
static const bool DEFAULT_WALLET_RBF{false};
nit: Doesn't matter, because it will be removed anyway, but for new code can use {}
lgtm, maybe the pull description can repeat some of the broader motivating factors?
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/26635795262/job/78495886583</sub>
<sub>LLM reason (✨ experimental): CI failed due to a trailing_whitespace lint error in doc/release-notes-35405.md (trailing space on line 6).</sub>
<details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
maybe the pull description can repeat some of the broader motivating factors?
Yes, updated it.
6 | @@ -7,3 +7,11 @@ key by passing the `-deprecatedrpc=bip125` startup option. Also, 7 | the `-walletrbf` startup option has been marked as deprecated and 8 | will be fully removed in the next release. Using this option emits 9 | a warning in the logs. 10 | + 11 | +Furthermore post this deprecation, the corresponding RBF default
I think the default fallback behavior was always the case and has nothing to do with the deprecation.
Maybe just: Moreover, the default value of -walletrbf has been changed to 1, because signalling for RBF is not ...?
Also, the title should be Wallet RPC and Startup Option and not RPC and Startup Option?
the default value of -walletrbf has been changed to 1
1? or 0 now?
Sorry, of course 0. Only off-by-one
Edit: I wanted to avoid the true, and false mention, because they are not accepted in the config.
Maybe just:
Pushed something like this.
With the nodes running fullrbf, wallet doesn't need to opt into RBF. This patch
switches the default of wallet rbf to false. Users still have the option to
optin RBF via the individual transaction (and PSBT) creation RPCs - for now.
6 | @@ -7,3 +7,8 @@ key by passing the `-deprecatedrpc=bip125` startup option. Also, 7 | the `-walletrbf` startup option has been marked as deprecated and 8 | will be fully removed in the next release. Using this option emits 9 | a warning in the logs. 10 | + 11 | +Moreover, the default value of -walletrbf has been changed to 0 12 | +in PR #35405 because signalling for RBF is not necessary for 13 | +replacing transactions due to nodes being fullrbf and the wallet 14 | +doesn't need to opt into it by default.
doc/release-notes-34917.md ^^^ A trailing newline is required, because git may warn about it missing. Also, it can make diffs verbose and can break git blame after appending lines.
Thus, it is best to add the trailing newline now.
Please add any false positives to the exclude list. ^---- ⚠️ Failure generated from lint check 'trailing_newline' (Check for trailing newline)!
$ git show 19917ca8c19fd7f5c86accbaceffd335915843ea | grep newline
\ No newline at end of file
It is ok as it is now, but for me makes sense to mention first this change and then the deprecation. Here you are saying why it is not necessary to signal RBF, which is a nice introduction on why we are deprecating.
Or maybe you can move the reason to the first paragraph and then mention the RPCs deprecation keys and then the -walletrbf
Makes sense. I made changes related to it but unintentionally ended up doing in PR #35433 here: https://github.com/bitcoin/bitcoin/pull/35433/changes/17d9368760587eae553e7eb7d12a3d0cfb0832fe.
Let's keep this as-is here because it will be updated in the follow-up PR anyway?
Sure, fine with me
ACK 073a4d1d80d7653aa19ebb8d911a6466393c1bbf
tested with the GUI
ACK 073a4d1d80d7653aa19ebb8d911a6466393c1bbf
Looks good to me
I don't see why the option needs to be changed to default to false before it is removed. This has an actual effect on tranasctions and contradicts the end state of every transaction still using MAX_BIP125_RBF_SEQUENCE once the options are gone.
This has an actual effect on tranasctions and contradicts the end state of every transaction...
I don't fully understand this^ comment.
There was a discussion in the previous deprecation PR #34917 around changing this default: #34917 (review). It makes sense to me to stop signalling because of nodes being fullrbf.
There was a discussion in the previous deprecation PR #34917 around changing this default: #34917 (comment). It makes sense to me to stop signalling because of nodes being fullrbf.
#32661 states:
Always use sequence number MAX - 2 when creating transactions.
Furthermore, I think the default that is used should be something that the wider wallet developer community should agree on for a best practice. We want to choose a sequence that reduces the risk of fingerprinting our user's transactions, not just revert it for the sake of reverting. Given that ~78% of current txs signal replacement, it may make sense to continue to use the signaling sequence even when it is meaningless.
Furthermore, I think the default that is used should be something that the wider wallet developer community should agree on for a best practice. We want to choose a sequence that reduces the risk of fingerprinting our user's transactions, not just revert it for the sake of reverting. Given that ~78% of current txs signal replacement, it may make sense to continue to use the signaling sequence even when it is meaningless.
For context, this was also discussed here #34917 (review)
For context, this was also discussed here #34917 (comment)
Maybe it could make sense to include that as motivation in this pull request, so that the motivation is self-contained and reviewers don't need to scroll down 22 comments into a thread and then follow links to find more about it.
(IIRC there was an earlier comment with the same point in an earlier thread years ago, but I think GH search is broken, so trying to find it is tedious)
Also, to extend that motivation, to remove the risk of this fingerprinting vector, either MAX_SEQUENCE_NONFINAL or MAX_BIP125_RBF_SEQUENCE will have to be fully removed from the codebase. Otherwise there is the risk that one function/RPC is picking MAX_BIP125_RBF_SEQUENCE while the other RPC is picking the basically equivalent MAX_SEQUENCE_NONFINAL (after fullrbf). There has already been a similar bug where SEQUENCE_FINAL was incorrectly picked, thus disabling anti-fee-sniping (see 255f7c720f13fbefafbe2114008a7d9812436690).
Generally, I think the number of possible finger-print vectors should be reduced, and the clearest way long-term seems to me to remove the vector that is the most temporary one and the most confusing one.
Furthermore, I think the default that is used should be something that the wider wallet developer community should agree on for a best practice. We want to choose a sequence that reduces the risk of fingerprinting our user's transactions, not just revert it for the sake of reverting. Given that ~78% of current txs signal replacement, it may make sense to continue to use the signaling sequence even when it is meaningless.
I agree that this should be something that ~everyone agrees on eventually. I presume OpTech (and other places) would pick this up after merge, so that the everyone has a chance to consider it.
I understand that the recent ~70+% figure may point to a path that looks easy (get 70% to 100%), but I think wallets that recently upgraded to MAX_BIP125_RBF_SEQUENCE are likely more active and willing to revert (https://github.com/bitcoin/bitcoin/pull/34917#discussion_r3310449446). At least more likely than wallets that still haven't upgraded to MAX_BIP125_RBF_SEQUENCE after more than a decade. So zooming out, hopefully after ~3 years and the latest ~10 years, ~everyone has removed the MAX_BIP125_RBF_SEQUENCE fingerprint.
Moreover, imagine a new developer reading wallet code (or writing a new wallet) in a decade or so, and them having to think about which sequence number to use, where two sequence numbers have the exact same meaning in practise. The risk of them introducing a new fingerprint by accident seems smaller if MAX_BIP125_RBF_SEQUENCE didn't exist, and they wouldn't have to read up on a now-irrelevant BIP.
Maybe it could make sense to include that as motivation in this pull request, so that the motivation is self-contained and reviewers don't need to scroll down 22 comments into a thread and then follow links to find more about it.
Done.
they wouldn't have to read up on a now-irrelevant BIP.
A relevant BIP should be written that documents a recommended sequence number.
but I think wallets that recently upgraded to MAX_BIP125_RBF_SEQUENCE are likely more active and willing to revert
While going forward, software may revert if that is the recommendation, this change raises a fingerprint for those using software that already exists.
In general, I don't think we should move forward with changing the default without first establishing what other wallets are going/likely to do. This seems like the kind of thing that should be discussed on the mailing list or delving first, and ideally, an informational BIP should be written as well.
This seems like the kind of thing that should be discussed on the mailing list or delving first
I've posted on the mailing list (pending approval).
Another option is to "randomly" signal or not signal, until all wallets agree on a specific setting, this would avoid fingerprint Core. Similar to what we do with backdating the locktime to avoid leaking on which block the tx was created..
Another option is to "randomly" signal or not signal
This option did come to my mind earlier but I decided against it because it doesn't help with the other motivation this PR has - which is to clean up the wallet codebase by removing the occurrences of this BIP 125 signalling constant and related variables in coin control, which makes the code longer and harder to reason about imo. In fact, adding the randomness flow might just make the wallet code flow more confusing.
Related to this clean up, a follow-up PR (to this and to #35433) is to clean up the functional tests: #35442.
This seems like the kind of thing that should be discussed on the mailing list or delving first
I've posted on the mailing list (pending approval).
Thx. Assinged the 32.x milestone, because it makes sense to first figure out what the overall goal should be before deprecation happens. So if this pull isn't merged or closed before 32.x, we should probably revert 34917 as well for now. The correct order to me seems to be: (1) Decide whether to use MAX_BIP125_RBF_SEQUENCE or not, then (2) move towards that, then (3) possibly at the same time with (2), "deprecate" the "wrong" sequence number, so that it isn't available in the source code as a fingerprinting vector.
Assinged the 32.x milestone
Thanks, this is helpful.
it makes sense to first figure out what the overall goal should be before deprecation happens. So if this pull isn't merged or closed before 32.x, we should probably revert 34917 as well for now.
Correct me if I misunderstood your comment here but I believe deprecating the walletrbf option (#34917) and the replaceable argument in the wallet RPCs (#35433) are not dependent on this PR. The fingerprinting concern was raised in this PR because of changing the default but the rationale for deprecating still holds. Users still will have the option to signal for RBF by using the deprecatedrpc option but because the signalling is redundant now, it does make sense to deprecate it so that the users know that this signalling is not needed anymore.
sure, they don't depend on each other code-wise. However, conceptually it feels a bit cleaner (and easier for users to follow) to first agree on the end-goal and then move toward that end-goal as reference goal.
Let's recall the goal here is not to just deprecate things, because they can be deprecated. As I see it, the main motivation to deprecate it would be to reduce fingerprinting. (Sure there is some minimal maintenance overhead and confusion to keep the toggle, but outside of fingerprinting I don't see that as a large downside of keeping the toggle). So I think conceptually it makes more sense to first figure out fingerprinting and then do everything else afterwards under that umbrella.
So if this pull isn't merged or closed before 32.x, we should probably revert 34917 as well for now.
Ok, I will raise a revert PR later* if this PR is not merged before the cut-off for v32.
Let's recall the goal here is not to just deprecate things, because they can be deprecated. As I see it, the main motivation to deprecate it would be to reduce fingerprinting. (Sure there is some minimal maintenance overhead and confusion to keep the toggle, but outside of fingerprinting I don't see that as a large downside of keeping the toggle). So I think conceptually it makes more sense to first figure out fingerprinting and then do everything else afterwards under that umbrella.
I have a slightly different view on this.
When I raised the PR, I didn't think that the main motivation was to reduce fingerprinting, although it is a good motivation. My main motivation was to improve user experience because I believe via deprecating it simplifies the overall flow of transaction creation for the user. Generally, I find it better if the user has to deal with fewer stuff while going through a workflow. It's an added advantage that the deprecation improves code readability and maintainability as well.
I've posted on the mailing list (pending approval).
Based on the responses in the ML from wallet developers in the community, I can see there is a preference of MAX-2 as the input sequence number in the transaction, even though it's not necessary.
So if this pull isn't merged or closed before 32.x, we should probably revert 34917 as well for now.
Ok, I will raise a revert PR later* if this PR is not merged before the cut-off for v32.
I don't think I should do this. I feel that the flow of providing the replaceable option to the user should still be deprecated (and removed later). The MAX-2 as the input sequence number can stay in the codebase but I feel it doesn't need to be associated with the concept of the transaction being replaceable. I will rework the draft PR #35442 in order to achieve this. Meanwhile, I look forward to reviews on #35433.