💬 maflcko commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2185030297)
same here. if truncation failed and is logged as error, i don't think it matters to log another error whether or not closing has failed as well.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3035641377)

>
> What command did you use to run locally?

I first checked by adding two log statements in `rpc_psbt.py` the first log to check the psbt before signing and the second log after `walletprocesspsbt` where CI is reporting an error. And then manually parsing the result with the help of `decodepsbt` to check whether the PSBT after`walletprocesspsbt` contains non-witness UTXO, but it doesn't in my case, so the test passed successfully.

I also ran all the functional test cases with `—exte
...

💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3035710020)
you'll have to use `--usecli` to run the tests, otherwise the cli is not used

👍 l0rinc approved a pull request: "cmake: Move internal binaries from bin/ to libexec/"
(https://github.com/bitcoin/bitcoin/pull/31679#pullrequestreview-2986225579)
Concept ACK, tested on Mac and Linux, it works as expected.

```bash
rm -rfd build install && \
cmake -B build -G Ninja \
-DCMAKE_BUILD_TYPE=RelWithDebInfo \
-DCMAKE_INSTALL_PREFIX="$PWD/install" \
-DBUILD_TESTS=ON \
-DBUILD_BENCH=ON \
-DBUILD_FUZZ_BINARY=ON \
-DBUILD_UTIL_CHAINSTATE=ON \
-DBUILD_GUI=ON \
-DBUILD_GUI_TESTS=ON && \
ninja -C build install && \
tree -L 3 install/{bin,libexec}
```

Checked the install command on Mac and Linux, it works as expected.

```bash
rm -rfd b
...

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184813932)
nit: we might want to realign the comments after the change

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184948130)
Q: slightly unrelated, but if testing should be available for the installed version, how come fuzzing isn't? Seems it should be mentioned with a 🛠 at least...

💬 l0rinc commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#discussion_r2184924462)
I understand the reason for wanting to clean up the install layout, but was wondering why the build layout doesn't mirror the production layout, given that `reducing clutter in bin` might apply to the development environment as well. I think it's best if devs get used to the final layout and don't have to think about the exact environment when deciding if https://github.com/bitcoin/bitcoin/blob/fa900bb2dce8ef3ee11d5980f008995d66877155/contrib/devtools/deterministic-unittest-coverage/src/main.rs#
...

💬 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`?