-walletbroadcast was a boolean 0 or 1 meaning either "do not broadcast transactions or "initial broadcast plus periodic re-broadcast (every 12-36 hours)".
Add a new possible value initialonly which would allow the user to configure the wallet to do the initial broadcast only and not to do the periodic rebroadcasts.
No new configuration options and no change in default behavior.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | achow101 |
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-->
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed.
<sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/36546473252/job/109333967547</sub>
<sub>LLM reason (✨ experimental): CI failed because the lint “doc” check detected undocumented command-line arguments: -walletbroadcast and -walletrebroadcast (AssertionError).</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>
133 | @@ -134,6 +134,7 @@ inline constexpr unsigned int DEFAULT_TX_CONFIRM_TARGET = 6; 134 | //! -walletrbf default 135 | inline constexpr bool DEFAULT_WALLET_RBF = true; 136 | inline constexpr bool DEFAULT_WALLETBROADCAST = true; 137 | +inline constexpr bool DEFAULT_WALLETREBROADCAST{false};
Why default to false? I feel like the responsibility for privacy should be left to users desiring privacy. The majority of them probably just want convenience. The initial -walletbroadcast did not require any setup from the user. For this, what if the user forgets to set up the -walletrebroadcast option?. IMO, I think rebroadcasting outweighs privacy so defaulting to true would be convenient. Just a thought though, I might be missing something.
No strong opinion. I was influenced by #36330 which would completely disable wallet re-broadcast with no way to enable it, if privatebroadcast=1. So, this PR, even with the default to "off" is softer than that because it makes it possible to enable the wallet rebroadcast if needed.
Also, my thinking, ignoring #36330 is that the wallet re-broadcast does more harm than good. It is a privacy nightmare and only provides some service in rare cases. But this is just my guess. For sure, there are users that use it and for them it is not "rare".
This is why I think it makes sense to have the wallet re-broadcast configurable in the first place. I am fine with a default to "on" (as in master without this PR).
Modified the PR to not change the default behavior.
Can we safely disable wallet re-broadcasting? If so, why do we need re-broadcasting at all?
Even if we choose this approach, I think extending -walletbroadcast with new modes would be cleaner than introducing another parameter with a very similar name.
Can we safely disable wallet re-broadcasting?
I think yes. Most users don't need it, but are nevertheless exposed to the privacy leaks it brings even if they never use it.
If so, why do we need re-broadcasting at all?
It serves this scenario:
Up to your judgement if this is rare or not, but is good to have wallet rebroadcast in such cases.
Even if we choose this approach, I think extending
-walletbroadcastwith new modes would be cleaner than introducing another parameter with a very similar name.
Hmm, sounds like something worth considering. Currently -walletbroadcast is a boolean. What would you suggest? Something like this maybe:
0 no initial, no rebroadcast1 initial and rebroadcastinitialonly initial, no rebroadcast?
The new
-walletrebroadcastis introduced with a default tofalse, which is a change in the default behavior.
Concept NACK
Disabling rebroadcasts by default has bad effects for the wallet.
`-walletbroadcast` was a boolean `0` or `1` meaning either "do not
broadcast transactions or "initial broadcast plus periodic
re-broadcast (every 12-36 hours)".
Add a new possible value `initialonly` which would allow the user to
configure the wallet to do the initial broadcast only and not to do the
periodic rebroadcasts.
No new configuration options and no change in default behavior.
57cbe86d86733d95d116c84a880c70c15dc99562...155eb27489f4fdbf9570dfb064e36fa12f37f399: don't introduce a new option, instead extend the allowed values of -walletbroadcast.
Concept NACK Disabling rebroadcasts by default has bad effects for the wallet.
Changed the PR. Now the default behavior is unmodified.
Now this PR allows -walletbroadcast=initialonly (in addition to the existent 0 and 1) to instruct it to only do the initial broadcasts and skip the periodic rebroadcasts.