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

⚠️ brunoerg opened an issue: "test: break `feature_block` into subtests?"
(https://github.com/bitcoin/bitcoin/issues/32877)
According to [corecheck](https://corecheck.dev/tests), `feature_block` is the slowest functional test and it's taking about 70 seconds to run. `feature_block` is a huge functional test that tests many things - large re-orgs, block with invalid transactions, disabled opcodes, sigop counts, etc. These things are not necessary coupled and could be broken into subtests and then we could run some of them instead of the whole test (see https://github.com/bitcoin/bitcoin/pull/30991) when necessary.

T
...

💬 furszy commented on pull request "test: fix feature_init.py intermittencies":
(https://github.com/bitcoin/bitcoin/pull/32835#discussion_r2185424642)
> If we know for sure, why are we running check_clean_start()?

I assume we are doing it to verify nothing else broke after deleting + restoring some data. But you have a fair point.
Pushed.

💬 Eunovo commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3036343118)
Rebased https://github.com/bitcoin/bitcoin/pull/31615/commits/11a9d55e84af2d6ff138b99310720667cd59dcc3 to https://github.com/bitcoin/bitcoin/pull/31615/commits/e260e23d88cd6b2aaa03d9d45dae8b174f2cfe1f

- moved `feature_reindex.py` test to "Tests less than 2m" section in `test_runner.py`