💬 maflcko commented on pull request "ci: Avoid && dropping errors":
(https://github.com/bitcoin/bitcoin/pull/32573#issuecomment-2943202412)
Ok, done in https://github.com/bitcoin/bitcoin/pull/32680

🚀 fanquake merged a pull request: "fuzz: Add target for coins database"
(https://github.com/bitcoin/bitcoin/pull/32602)

💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2128265048)
I think it's fine, but indeed should be in the description.

🚀 fanquake merged a pull request: "doc: update tor docs to use bitcoind binary from path"
(https://github.com/bitcoin/bitcoin/pull/32679)

💬 TheCharlatan commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2128294281)
If so, it should also get a note that its command line interface is not expected to be stable.

💬 m3dwards commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2128307329)
@hodlinator thanks for the confirmation.

The other factor worth considering is that as written, this function will not work on Windows. I'm inclined to still leave it out.

🚀 fanquake merged a pull request: "depends: drop `ltcg` for Windows Qt"
(https://github.com/bitcoin/bitcoin/pull/32496)

💬 maflcko commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2128319385)
If it is kept, it shouldn't be the default, like it is now. Also, it should document that it doesn't work on Windows nor on cmake.

💬 hodlinator commented on pull request "blocks: force hash validations on disk read":
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2128262134)
From PR description:

> added hash assertions in ProcessGetBlockData and ProcessMessage to validate that the block read from disk matches the expected hash;

Stared at `BlockManager::AddToBlockIndex()` but I fail to see how the hash in the keys of the `BlockManager::m_block_index` map would be different from the hash in the `CBlockIndex::phashBlock` (stored in the values of `BlockManager::m_block_index`). Would require cosmic rays hitting RAM (or faulty RAM) rather than disk from what I can
...

💬 hodlinator commented on pull request "blocks: force hash validations on disk read":
(https://github.com/bitcoin/bitcoin/pull/32638#discussion_r2128271631)
nit: Would be clearer if the PR description changed to something like
```diff
- Why is the hash still optional
+ Why is the hash still optional (but no longer has a default value)
```

Happy this PR removes the default value!

🤔 hodlinator reviewed a pull request: "blocks: force hash validations on disk read"
(https://github.com/bitcoin/bitcoin/pull/32638#pullrequestreview-2899350845)
Concept ACK efbe0e86810ccbe828472935eb221c2ddf295bf3

💬 fanquake commented on issue "test: `feature_fee_estimation` failure after duplicate coinbase tx weight reservation fix [AssertionError: Estimated fee (0.00923427) out of range]":
(https://github.com/bitcoin/bitcoin/issues/32461#issuecomment-2943410152)
https://github.com/bitcoin/bitcoin/runs/43526788535

💬 fanquake commented on issue "test: `feature_fee_estimation` failure after duplicate coinbase tx weight reservation fix [AssertionError: Estimated fee (0.00923427) out of range]":
(https://github.com/bitcoin/bitcoin/issues/32461#issuecomment-2943411436)
https://github.com/bitcoin/bitcoin/runs/43526123006

💬 hodlinator commented on pull request "test: enabling wallet migration functional test on windows":
(https://github.com/bitcoin/bitcoin/pull/32219#discussion_r2128390637)
Commit message for ff34214267d0526a59d4ffebdfa9b194f73a5ff0 should at least be changed to something like:

```diff
- Using the get_previous_releases.py script to build from source would have
- broken with the move to cmake. As there were no complaints, it is
- assumed nobody uses this functionality.
+ Using the get_previous_releases.py script to build from source only works for
+ releases prior to v29 due to removal of Autotools (in favor of CMake). It also
+ does not support building on
...

⚠️ fanquake opened an issue: "ci: libFuzzer disabled leak detection after every mutation"
(https://github.com/bitcoin/bitcoin/issues/32681)
https://cirrus-ci.com/task/4646509657980928?logs=ci#L91
```bash
[05:44:30.094] INFO: libFuzzer disabled leak detection after every mutation.
[05:44:30.094] Most likely the target function accumulates allocated
[05:44:30.094] memory in a global state w/o actually leaking it.
[05:44:30.094] You may try running this binary with -trace_malloc=[12] to get a trace of mallocs and frees.
[05:44:30.094] If LeakSanitizer is enabled in this process it will still
[05:44:30.094]
...

💬 maflcko commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2943516249)
> But makes `rpc_help.py` fail. (I still don't know why)

The cli tool needs to "convert" non-string args. However, if the overload accepts a string and a non-string, it is unclear how to proceed. Maybe similar to `hash_or_height`?

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2128279631)
in bec74626867abefbfc55da5aaaf8a0c030663f3a:

nit: comparing ints is more performant than comparing strings, and is more differentiating than the filename, so would do the line comparison first

```cpp
return lhs.line() == rhs.line() && strcmp(lhs.file_name(), rhs.file_name()) == 0 ;
```

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2128389554)
`NeedsRateLimiting` as well as the underlying maps imo belong to `LogRateLimiter` rather than `Logger`. I also don't think `should_ratelimit` is a meaningful parameter, the function should just not be called if that's `false`. I also don't understand why `logging_function` is a separate parameter - can't we just get that from `source_loc`?

Suggested diff that incorporates all of those suggestions:

<details>
<summary>git diff on 5535df69a2</summary>

```diff
diff --git a/src/logging.cpp
...

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2128403151)
It seems like this startup option is still in the latest force-push (5914a2ee6c915585b7213217047ad1644b873376), but from this discussion it doesn't seem like there are good arguments to keep it? I'm not sure if @l0rinc still needs it for IBD bench reasons, but imo that's way too much of a niche to keep it as a startup option?

💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2128390673)
nit: string_view to avoid unnecessary string construction

```suggestion
.Write(std::hash<std::string_view>{}(s.file_name()))
```