💬 rkrux commented on pull request "descriptor: handle listdescriptors(private=true) for descriptors having partial keys":
(https://github.com/bitcoin/bitcoin/pull/32186#issuecomment-2850731098)
@Eunovo I have not gotten around to trying the other approach yet as I was working on few other PRs. Please feel free to give it a shot if you want else I will give it another try some time later.

Closing this PR either way.

💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2073295814)
Since we use the same CMake function to embed the manifest for all executables, I believe checking just one of them in CI is sufficient.

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2850745523)
Can you please show the exact args you were using? Are you using `-addnode=local-network`? Is it possible that behaves like a `reindex-chainstate` regarding periodic flushes (see https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2848103940)?

This is currently only triggered with actual IBD with real peers.

Enabling regular flushes for reindex(-chainstate) (and maybe even for a fixed nodes, not sure) is done separately in this PR: https://github.com/bitcoin/bitcoin/pull/32414
Can
...

💬 hebasto commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2850768999)
> The same applies in CMake. A project should not modify compile flags, irrespective of where the flags are defined. What a project may do, is provide a defensive error/warning when problematic flags are detected:
>
> ```cmake
> if(CMAKE_CXX_FLAGS MATCHES "-O3")
> message(FATAL_ERROR "Building this project with -O3 is not supported.")
> endif()
> ```

This results in behaviour where every configuration step using `-DCMAKE_BUILD_TYPE=Release` with the default flags ends in an error, w
...

📝 Sjors opened a pull request: "miner: don't needlessly append dummy OP_0 to bip34"
(https://github.com/bitcoin/bitcoin/pull/32420)
Our miner code adds OP_0 to the coinbase scriptSig in order to avoid triggering the bad-cb-length consensus error on test networks.

This commit, like blocktools.py, limits that workaround to blocks 1 through 16 where it's actually needed (OP1 through OP_16).

Previously the coinbase transaction generated by our miner code was not used downstream, because the getblocktemplate RPC excludes it.

Since the Mining IPC interface was introduced in #30200 we do expose this dummy coinbase transact
...

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2073326725)
FWIW: It looks like `bitcoin_test.exe`, which is shipped as part of the distribution, does not get a manifest.

💬 maflcko commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#discussion_r2073326908)
nit: Would be good to limit this to regtest, assuming this is the only test network that "needs" it. Otherwise this can't be tested outside of mainnet?

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2850809184)
i've added a check for the application manifest to the symbol checker here: https://github.com/laanwj/bitcoin/tree/2025-05-manifest-check, feel free to cherry-pick.

💬 Sjors commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#discussion_r2073348359)
I'll try to narrow it down to regtest.

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2073354232)
This just mimics the error handling when the file cannot be opened:

https://github.com/bitcoin/bitcoin/blob/53ccb75f0c47e1e48504cb392d7589548a8d25a2/src/node/blockstorage.cpp#L945-L949

👍 TheCharlatan approved a pull request: "miner: timelock the coinbase to the mined block's height"
(https://github.com/bitcoin/bitcoin/pull/32155#pullrequestreview-2814660224)
Re-ACK a58cb3b1c12c8cb75a87375c50f94c4605bb805d

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2850863863)
`fe2d1955f6...2c1a8ff0c0`: address suggestions

> AutoFile::write_buffer should also mark the file as dirty

Done.

> the current fclose returns a fake error code...

Replied to the discussion thread at https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2048792563

💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2073366652)
Thanks! Added.

💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2850870900)
> i've added a check for the application manifest to the symbol checker here: https://github.com/laanwj/bitcoin/tree/2025-05-manifest-check, feel free to cherry-pick.
>
> (not too important yet now as we're just adding some metadata, but when we add critical things in there like "use UTF-8 codepage" as in #32380, it's really important for them to be in the release too)

Thanks! Cherry-picked.

💬 hebasto commented on pull request "build: replace header checks with `__has_include`":
(https://github.com/bitcoin/bitcoin/pull/32405#issuecomment-2850873641)
My Guix build:
```
aarch64
66d71c866bd111ffe65bc03b9e1653a95eb678f0b04451759c56af868bfc03d5 guix-build-e1f543823b30/output/aarch64-linux-gnu/SHA256SUMS.part
1a4130d801620a63d86c3069b1fbca39ebc963e610101451d3f48b1c191ca4b3 guix-build-e1f543823b30/output/aarch64-linux-gnu/bitcoin-e1f543823b30-aarch64-linux-gnu-debug.tar.gz
745adbc7767344a8cd0ebe1e7592239614d89f949558c9b6a2ae58f7b2602a32 guix-build-e1f543823b30/output/aarch64-linux-gnu/bitcoin-e1f543823b30-aarch64-linux-gnu.tar.gz
cb69d205
...

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2073373828)
The interface of `AutoFile::fclose()` is not changed by this PR and is not the purpose of this PR to change it. Thus I will leave it as it is.

I think that the interface of the current `AutoFile::fclose()` is just fine. It mimics 1:1 the libc [fclose(3)](https://www.man7.org/linux/man-pages/man3/fclose.3.html) which is in line with the majority of the other system functions which:

> return the value 0 if successful; otherwise the value -1 is returned and the global variable errno is set to
...

👍 hebasto approved a pull request: "build: replace header checks with `__has_include`"
(https://github.com/bitcoin/bitcoin/pull/32405#pullrequestreview-2814684426)
ACK e1f543823b300b28c9edaf5d1a3e1e9badde471b, I have reviewed the code and it looks OK.

💬 purpleKarrot commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2850897602)
> This results in behaviour where every configuration step using `-DCMAKE_BUILD_TYPE=Release` with the default flags ends in an error, which is a terrible UX.

Then don't use `-DCMAKE_BUILD_TYPE=Release`. You get better UX with less typing.

For local development, just use the default build type. CI and package builds are done with scripts that allow enough ways to specify compile flags.

Distro packagers want to build all packages with the same compile flags. They will patch out all proje
...

👍 rkrux approved a pull request: "scripted-diff: adapt script error constant names in feature_taproot.py"
(https://github.com/bitcoin/bitcoin/pull/32415#pullrequestreview-2814701995)
tACK b5f580c580257d28d295cae3f787b55eb1863f16

Having gone through `feature_taproot.py` file previously that can take a while to understand fully, this seems like a reasonable change to make it easier to follow the file by making the error constant names congruent with the ones in the cpp code.

+1 on using the scripted diff so that the reviewers need not manually verify each change in the file.

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2073385113)
Done