ResendWalletTransactions() only executes every 12-36h (24h average). Triggering it every second is excessive, once per minute should be plenty.
The goal of this PR is to reduce the amount of (unnecessary) schedule executions by ~60x without meaningfully altering transaction rebroadcast logic/assumptions which would require more significant review.
150 | @@ -151,7 +151,7 @@ void StartWallets(WalletContext& context, CScheduler& scheduler) 151 | if (context.args->GetBoolArg("-flushwallet", DEFAULT_FLUSHWALLET)) { 152 | scheduler.scheduleEvery([&context] { MaybeCompactWalletDB(context); }, std::chrono::milliseconds{500}); 153 | } 154 | - scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, std::chrono::milliseconds{1000}); 155 | + scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, std::chrono::milliseconds{1000 * 60});
scheduler.scheduleEvery([&context] { MaybeResendWalletTxs(context); }, 1min});
oh nice, thanks! pushed
wallet_resendwallettransactions.py is failing. You need to change the mockscheduler calls to work with the new timer.
ACK 7f4e1a7760d3f4c60a41bb6a1fca3a457e8ad297 Seems like a reasonable change.
utACK 7f4e1a77 Shouldn't be a problem to rise it even more while the next resend is kept at 12-36 hours from the last one.
ResendWalletTransactions() only executes every 12-36h (24h average).
Triggering it every second is excessive, once per minute should be
plenty.
Force pushed to incorporate #25929 and (hopefully) fix the failing Windows builds - otherwise no changes in this latest force push.
wallet_resendwallettransactions.py is failing. You need to change the mockscheduler calls to work with the new timer.
Thanks, fixed from 78f864d -> 7f4e1a7
Shouldn't be a problem to rise it even more while the next resend is kept at 12-36 hours from the last one.
Probably, but I think every minute is a frequency everyone can agree with without bikeshedding so I'd rather not make the change too big.
ResendWalletTransactions() only executes every 12-36h (24h average). Triggering it every second is excessive, once per minute should be plenty.
This was done to improve privacy in https://github.com/bitcoin/bitcoin/pull/18038
This was done to improve privacy in #18038
Is this a comment regarding an issue you have with this PR privacy-wise, or just to share background information? I don't see how this PR conflicts with the privacy improvements of #18038 since it doesn't do anything to rebroadcast transactions faster. As such I assume you intended the latter, but if not would you care to elaborate, please?
I replied to the PR description and not sure about the goal of this PR by changing MaybeResendWalletTxs in StartWallets() to 1 minute if rebroadcast will happen once per day.
The goal of this PR is to reduce the amount of (unnecessary) schedule executions by ~60x without meaningfully altering transaction rebroadcast logic/assumptions which would require more significant review. There should be no privacy implications in this PR. If you, like furszy suggested earlier too, would be open to an even slower execution schedule, I'm not against that. Beyond a per-minute frequency I just don't think it matters enough to spend a lot of time discussing that.
Edit: added some of this to PR description.
The goal of this PR is to reduce the amount of (unnecessary) schedule executions by ~60x without meaningfully altering transaction rebroadcast logic/assumptions which would require more significant review. There should be no privacy implications in this PR. If you, like furszy suggested earlier too, would be open to an even slower execution schedule, I'm not against that. Beyond a per-minute frequency I just don't think it matters enough to spend a lot of time discussing that.
Thanks. I understand the pull request now. Yes 1 minute is better than 1 second and could be changed to anything between 1 hour to 6 hours as well.
Concept ACK
ACK 5ef8c2c9fc4ebce6cbfea6a55a89a0ab7ee98a1a
ACK https://github.com/bitcoin/bitcoin/pull/25922/commits/5ef8c2c9fc4ebce6cbfea6a55a89a0ab7ee98a1a
nit: Since we have setmocktime and mockscheduler RPC, it would have been better if the test tried triggering this after 1 minute on several occasions and see how things work. Example: 1 minute, 15 minutes, 60 minutes, 4 hours etc.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--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.