💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184938620)
I haven't done these steps manually yet , just want to double check if these are still accurate - or if we need to scan the `libexec` dir as well now:
* https://github.com/bitcoin/bitcoin/blob/31d325464d0cf2d06888e0c543ae26a944f2ec6b/contrib/macdeploy/detached-sig-create.sh#L47
* https://github.com/bitcoin/bitcoin/blob/e181bda061ca63021511be6e286fdf6a5818df49/contrib/guix/libexec/codesign.sh#L113

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185068952)
```suggestion
This table describes the files installed by Bitcoin Core across different platforms.
```

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185089423)
`bitcoin-wallet` should still be in `bin` as far as I can tell, not `libexec` (nit: + double space):
```suggestion
`bitcoin-node` are also now installed in `libexec/`
```

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185083314)
nit: if you're already editing this file consider fixing line 86:
> ... double-spends due to *the* lack of synchronization

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184944417)
nit: not everyone views the rendered view, we might want to align the source view as well:
```suggestion
| **Path** | **Description** |
|------------------------------------------------------------|-----------------------------------------------------------------------------|
| [README.md](README.md) or [readme.txt](README_windows.txt) | Project information and instructions
...

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185072353)
nit: there's a similar one in the PR description:
> The table below shows the install location and availability **of** each binary after this change

👍 ismaelsadeeq approved a pull request: "cluster mempool: add TxGraph reorg functionality"
(https://github.com/bitcoin/bitcoin/pull/31553#pullrequestreview-2983011274)
reACK 1632fc104be8f171f59a023800b2f9f20d0a3cff 🛰️

💬 ismaelsadeeq commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r2182682297)
We start being oversized at i = 24 iteration.

💬 maflcko commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185230519)
not sure what your question/suggestion is, but it should be possible to replace outdir/libexec with `"${BASE_BUILD_DIR}"/bin/` here. Either should be fine and equivalent.

💬 maflcko commented on pull request "ci: Catch tests corrupting the source directory":
(https://github.com/bitcoin/bitcoin/pull/32874#issuecomment-3036105185)
so on linux the check doesn't work and on macos it passed: https://github.com/bitcoin/bitcoin/actions/runs/16073560546/job/45363434537?pr=32874#step:7:2812 :(

💬 m3dwards commented on pull request "ci: update pwsh to use custom shell that fails-fast":
(https://github.com/bitcoin/bitcoin/pull/32672#issuecomment-3036105592)
@hebasto is there any reason we need Powershell at all? Can we just use bash (or another language) everywhere?

📝 Sjors opened a pull request: "refactor: use PSBTOptions for filling and signing"
(https://github.com/bitcoin/bitcoin/pull/32876)
Replace the `sign`, `finalize` , `bip32derivs` and `sighash_type` arguments that are passed to `FillPSBT()` and `SignPSBTInput()` with a `PSBTOptions` struct.

```cpp
truct PSBTOptions {
bool sign{true};
bool finalize{true};
bool bip32_derivs{true};
std::optional<int> sighash_type{std::nullopt};
};
```

This makes it easier to add additional options later without large code churn, such as `avoid_script_path` proposed in #32857. It also makes the use of default boolean
...

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2185280684)
Sure, but why do we have to keep both layouts, why isn't `"${BASE_BUILD_DIR}"/libexec/test_bitcoin` the equivalent of `"${BASE_OUTDIR}"/libexec/test_bitcoin`?

🤔 l0rinc reviewed a pull request: "refactor: Header sync optimisations & simplifications"
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-2986979853)
I'd like to review this - can you tell me how to measure the effect of the "optimizations" using either the microbenchmarks or a reindex or anything that shows the magnitude of change?
Would be helpful if you could publish your measurements if you have any.

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2185233124)
There's a significant overlap with https://github.com/bitcoin/bitcoin/pull/32579/files#diff-46faf106e779941e78de61d061d7817f9dd7b1f12a4157e6991417c7f0a7e9d3L95 - would be great if we could avoid a complete rewrite after one of them is merged

💬 l0rinc commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2185221868)
nit: please reformat the block

💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2185308799)
Reintroducing this block + your patch will only do something if we also reintroduce the custom warning handling (`configure_warnings` doesn't come from CMake). The behavior of master is that the only warnings that could appear here are `no python`, `tried to turn off ccache but couldn't`, `PIE doesn't work`, (and no other CMake warnings/output).

> Also, could enable Werror in "name": "dev-mode", preset?

Will add.

💬 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_r2182919153)
nit: making this a simple `struct LogRateLimiter::Stats` would clean things up a bit more.

Diff that does that + updates naming to reflect the phasing out of `LogLocationCounter` naming and updates documentation to not mention concepts it's not aware of (e.g. `Stats` does not know anything about time windows).

<details>
<summary>git diff on 6a7147358c</summary>

```diff
diff --git a/src/logging.cpp b/src/logging.cpp
index a090803652..7dc81928f2 100644
--- a/src/logging.cpp
+++ b/src
...

👍 stickies-v approved a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2983245277)
ACK 6a7147358c9d6e3883dcdbbee9fb2c1cb4baf5ff

Keeping nits to a minimum because I've already been quite loud on this PR and I want to make sure we get the improvements merged soon. Left a few nits/test comments that can be done in a follow-up (although I'd be happy to quickly re-ACK here).

💬 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_r2185218652)
review note: if a single line contains two log statements, they will be sharing the same stats. I can't find any current such instances in the code, and the only realistic scenario in which I could see something like that happen is:

```cpp
warn ? LogWarning(msg) : LogInfo(msg)
```

which would logically be equivalent to the same log location (e.g. using `LogPrintLevel`) anyway.

This behaviour can be verified with:

<details>
<summary>git diff on 6a7147358c</summary>

```diff
diff
...