💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181022825)
Now uses a parameter, but I have kept the comment. I think it is clear as is? I think a follow-up could address this?

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181023549)
I haven't implemented this, could this be left for a follow-up?

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181024709)
I've modified the comment so it doesn't repeat the phrase "rate limit" twice.

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181026104)
Modified the commit message and have checked that `MemUsage` returns an accurate accounting of the memory used. Thanks for this comment, it encouraged me to verify this.

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181027949)
I have not run benchmarks, though this `insert` is pre-existing so will resolve. It would be nicer if there was a better way to format the string other than the insert-at-beginning logic in `FormatLogStrInPlace`.

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181028316)
Modified the commit message 👍

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181029394)
I agree this is confusing and threw me. The different base part is pre-existing so will resolve. I think another PR could eliminate the different bases unless there's a compelling "historical" reason to keep both?

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181029843)
Latest change address this.

💬 achow101 commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3029409482)
> which is that timers are mapped by name and can be removed by name if, for example, the user calls the RPC again with a different timeout value.

This behavior would already be changing in this PR. Deleting the `HTTPRPCTimer` does not de-schedule the task and it will still execute later.

> Current behavior I believe is to only honor the most recent walletpassphrase argument.

This behavior would still be maintained even if the task runs multiple times because there is an explicit check
...

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181030545)
I doubt anyone will review it if we push it separately - but I think we could make these tests less awkward. Not a blocker from my side, just a preference

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181030565)
Changed, I think tracking bytes is better.

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181030928)
Did not implement as this gave me a compilation error?

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181031427)
Agree phrasing was confusing, have changed.

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181033230)
Can you show us the raw measurements?

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2181049800)
I agree with you that the tests are awkward, but I do want to limit churn as this PR has been through a few iterations. I would definitely review a follow-up if you wanted to make the PR. This could also be a good candidate for https://www.bitcoin-followups.info/

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3029455047)
> * I feel like it doesn't make sense to store backups of a wallet _inside_ the wallet being backed up. The approach I suggested of just backing up all wallets as `{prefix}_{timestamp}.legacy.bak` files in the top-level wallets directory seems like it should make the backup files easier to find, identify, and clean up.

> That seems reasonable to me.

Agreed, I've added https://github.com/bitcoin/bitcoin/pull/32273/commits/75df71398dc4feefaa6f6ee611e41c5daa8ebc2b which moves the backup from
...

💬 achow101 commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `unused(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-3029461513)
> we don't seem to be rotating `unused(KEY)` descriptors when encrypting the wallet,

We only rotate the automatically generated descriptors, and `unused(KEY)` is not one of those.

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#issuecomment-3029466719)
The latest push 34fc54d5476af464407c8de64c58897a35dce5cb should address the most pressing comments. IMO there is still a bit of cleanup to be done in the tests, but I would like to leave that for a follow-up. I'm going to test this branch more, so I've updated the branch mentioned in OP that can trigger the rate limiting logic: https://github.com/Crypt-iQ/bitcoin/tree/log_ratelimiting_05192025_rpc

Windows CI failure is unrelated and is addressed by https://github.com/bitcoin/bitcoin/pull/3285
...

🤔 skylinesales reviewed a pull request: "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet"
(https://github.com/bitcoin/bitcoin/pull/31423#pullrequestreview-2980797818)
@achow101 

💬 skylinesales commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#discussion_r2181075955)
src/qt/walletcontroller.cpp