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

💬 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_r2185313756)
I think there are a number of issues with this part of the test:
- the docstring states locations 2 and 3 are exempt, but I don't see why they would be
- it tests that the location can log **up to** the ratelimit amount, which is true in any case
- it tests that the filesize increases, which would also happen if ratelimiting were to have happened, so it doesn't really catch anything

I think the `logging_filesize_rate_limit` test can be improved and cleaned up in a follow-up (to e.g. addres
...

💬 hodlinator commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2185335801)
Oof, that's nasty.

💬 hodlinator commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2185355403)
If we know for sure, why are we running `check_clean_start()`? IMO we should do one of:
* Trust that we fully restored the state and remove `check_clean_start()` here.
* Run `check_clean_start(startup_args)` to verify that the index being tested was restored.

💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3036332191)
I opened #32876 and rebased this PR on top of it. That allowed me to drop 1486f05c8eded02d72690bf34d983a03e4ad748b + efc34a514b59cc1bab6e4d0f80c197481561c429.

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2185423052)
For this test, I need the `minimumchainwork` to be higher than the best_header chainwork. The `height` of the best_header here is `3714`, so the `minimumchainwork` needs to be for a chain considerably higher than `3714` blocks. Antoine had provided the exact chain work for `32256` blocks in this [stack exchange answer](https://bitcoin.stackexchange.com/questions/26869/what-is-chainwork#66874), so I used that.

Note that the `minimumchainwork` used doesn't affect the duration of the test, it
...