📝 maflcko opened a pull request: "ci: Rewrite test-each-commit as rust script"
(https://github.com/bitcoin/bitcoin/pull/32680)
This was requested in https://github.com/bitcoin/bitcoin/pull/32573#issuecomment-2897546693, as it is currently not possible to catch this type of error in a Bash linter (https://github.com/bitcoin/bitcoin/pull/32573#pullrequestreview-2857077463).

In theory this could be written in Python, but then we'd get all the type-safety errors, such as https://github.com/bitcoin/bitcoin/pull/29068#issuecomment-1852746032.

So fix all issues by using a simple Rust script compiled with strict warnings:
...

💬 Zeegaths commented on issue "doc: references to unshipped documentation":
(https://github.com/bitcoin/bitcoin/issues/32565#issuecomment-2943120988)
I think adding the online links makes sense. I would like to add them.

💬 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
...