💬 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

💬 davidgumberg commented on pull request "functional test: correctly detect nonstd TRUC tx vsize in feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32859#issuecomment-3029569601)
utACK https://github.com/bitcoin/bitcoin/commit/f0524cda3995cf65adab3d0ca8da0dee4e31c79b

💬 achow101 commented on pull request "functional test: correctly detect nonstd TRUC tx vsize in feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32859#issuecomment-3029573887)
ACK f0524cda3995cf65adab3d0ca8da0dee4e31c79b

💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3029587757)
https://github.com/bitcoin/bitcoin/blob/a92e8b10a5fb1ac25484aa17787e27b4a8632f69/src/wallet/rpc/encrypt.cpp#L103-L104

🤦 yup clear as day thanks

🚀 achow101 merged a pull request: "functional test: correctly detect nonstd TRUC tx vsize in feature_taproot"
(https://github.com/bitcoin/bitcoin/pull/32859)

💬 davidgumberg commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3029592937)
Rebased because of spurious CI failure fixed by #32859.

🤔 pablomartin4btc reviewed a pull request: "test: check P2SH sigop count for coinbase tx"
(https://github.com/bitcoin/bitcoin/pull/32850#pullrequestreview-2980935709)
ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3

(coinbase transactions don’t refer to a real previous output and thus cannot be spending P2SH outputs, making the sigop count irrelevant for them).

🤔 marcofleon reviewed a pull request: "fuzz: Make process_message(s) more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-2980950526)
tACK fac673d434b4d32d8c44dcc50a3655ba4a1816de

Running the stability script, there's significantly less coverage differences with this PR. My only question is why does `ResetChainman` set mock time in `process_messages` but not in `process_message`? Didn't see an obvious reason, but I could be missing something.

💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2181163397)
that was actually done on purpose. See https://leimao.github.io/blog/Python-Default-Argument-Mutable-Object/.