💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3051852485)
> A generic -Werror option (without the = part) that simply turns all warnings into errors?

I think so. The downside in this PR is that we have to somewhat (even if semantically) misuse `AUTHOR_WARNING()`, to get the warning into error behaviour. Ideally we could just use `WARNING()`, and `-Werror` would turn those warnings into errors.

💬 Rahamath-unnisa commented on issue "init: torcontrol argument should be validated":
(https://github.com/bitcoin/bitcoin/issues/23589#issuecomment-3051989757)
Hi! I'm new to Bitcoin Core development and would like to work on this issue. Can I take it?

🤔 maflcko reviewed a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-3000242855)
left some style nits, which are trivial to ignore, or follow-up with. Nothing blocking.

review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9 🌬

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+
...

💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194189826)
from the llm linter:

* +LogWarning, LogError, → LogWarning, LogError` [missing backtick before comma makes the inline code span invalid]


However, all of those are implementation details anyway, and it could make sense to remove those. End-users probably wonder what those mean for them, so it could make sense to instead mention the name of the setting that affects them:

"Unconditional logging to disk is now rate limited by giving each source location a quota of 1MiB per hour. Unconditional l
...

💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194354583)
nit in https://github.com/bitcoin/bitcoin/commit/b5d0cc29ac378f360424472f637f6d6af74660a5: Could clarify in the comment "... of bytes *per source location* ...", but just a nit.

💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194330070)
nit in b5d0cc29ac378f360424472f637f6d6af74660a5: Can remove the trailing `\n` (it is added by logging).

Also, could use the Warning log level instead, because rate-limiting should never happen in practise and if it does, it seems like something that should be fixed by adjusting the logging at that location?

💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194249282)
nit: Probably wanted to say "... *to be able* to catch ..." or "to throw any exceptions." Otherwise this reads a bit as if `noexcept(false)` were to catch any thrown exceptions. Could also just remove the comment, but just a nit.

💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194621936)
nit in 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6: Could use the `HasReason` helper, but up to you.

💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194367325)
nit in https://github.com/bitcoin/bitcoin/commit/b5d0cc29ac378f360424472f637f6d6af74660a5: I don't think the `ll` is needed to silence the w-sign-compare warning. I'd go with just `0u`, or if you want to fully specify the type `uint64_t{0}`.

💬 Sjors commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3052127988)
It's still marked as draft though.

💬 fanquake commented on issue "qt: translation related warnings":
(https://github.com/bitcoin/bitcoin/issues/32710#issuecomment-3052248717)
@hebasto If this is an actual issue can it be fixed up? Otherwise, it'd be good if there was a way to avoid this much build log spam. ie:
```bash
CMake Warning at depends/arm64-apple-darwin/native/lib/cmake/Qt6LinguistTools/Qt6LinguistToolsMacros.cmake:418 (message):
Failed to resolve language code for
/root/ci_scratch/src/qt/locale/bitcoin_am.ts. Please update
CFBundleLocalizations in your Info.plist manually.
Call Stack (most recent call first):
depends/arm64-apple-darwin/native/lib/c
...

💬 TheCharlatan commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2194840399)
The way I read it you are currently just checking if the field is set, not if it is set to true. At least providing "return_spending_tx":false in the cli still provides the full transaction.

💬 TheCharlatan commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2194916715)
Yes, I just wasn't sure if this is really useful. There are many other ways something could be misconfigured. We'd also be warning here now on what continues to be just normal working behaviour. Clients should be querying `getindexinfo` first anway in my opinion, but then again, I am also not sure what exactly the precedent is here. Maybe add a note to pass `"mempool_only":true` to silence the warning?

📝 maflcko opened a pull request: "fuzz: Add missing calls to `SetMockTime` for determinism"
(https://github.com/bitcoin/bitcoin/pull/32927)
(Tracking issue https://github.com/bitcoin/bitcoin/issues/29018)

During fuzzing, `AppInitParameterInteraction` may actually disable a previously set mocktime. This is confusing and can also cause non-determinism.

Fix this issue, by

* fixing the erroneous `-mocktime` parsing in `AppInitParameterInteraction`.
* adding the missing `SetMockTime` calls to the affected fuzz init functions.
* adding a `CheckGlobals` to the fuzz init, to prevent this issue in the future.

This can be tested
...

👋 ryanofsky's pull request is ready for review: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345)

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3052619311)
> It's still marked as draft though.

Thanks, fixed!

💬 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_r2195027425)
Added in 4c772cbd83e502a1339e8993d192ea6416ecd45c

💬 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_r2195027732)
Changed in 4c772cbd83e502a1339e8993d192ea6416ecd45c

💬 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-3052667445)
4c772cbd83e502a1339e8993d192ea6416ecd45c changes the release notes for clarity.

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-3052670668)
Rebased eec7c49cf777cdcc53aeb20e31fbd5881c2b008b -> 8512349ccd73c7acd8ccd6d4c2708459bdce3301 ([chainman_flush_destructor_5](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_5) -> [chainman_flush_destructor_6](https://github.com/TheCharlatan/bitcoin/tree/chainman_flush_destructor_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainman_flush_destructor_5..chainman_flush_destructor_6))