💬 fanquake commented on pull request "build: replace header checks with `__has_include`":
(https://github.com/bitcoin/bitcoin/pull/32405#issuecomment-2850645193)
> The logic should be: If the platform is BAR, then include <foo.h>

Not sure I agree here; as this would give us less-generic code, and the need to maintain platform mappings.

💬 Sjors commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2850660569)
To clarify, is this PR supposed to write to disk _during_ IBD? My own testing with a very large -dbcache doesn't show that, since the chainstate directory remains 16 kb for at least a few hours.

🤔 TheCharlatan reviewed a pull request: "build: replace header checks with `__has_include`"
(https://github.com/bitcoin/bitcoin/pull/32405#pullrequestreview-2814491794)
Approach ACK e1f543823b300b28c9edaf5d1a3e1e9badde471b , pending guix build.

> You don't include a header because it exists, but because of the symbols it declares. The logic should be: If the platform is BAR, then include <foo.h> to use function foo and fail when it it does not exist. If not on platform BAR, then don't include <foo.h>.

There is already some additional gating in the first instance (randomenv.cpp), which actually checks for the relevant gadgets (HAVE_SYSCTL). A similar gatin
...

💬 Eunovo commented on pull request "descriptor: handle listdescriptors(private=true) for descriptors having partial keys":
(https://github.com/bitcoin/bitcoin/pull/32186#issuecomment-2850676440)
@rkrux still working on this?

👍 laanwj approved a pull request: "subprocess: Backport upstream changes"
(https://github.com/bitcoin/bitcoin/pull/32358#pullrequestreview-2814499302)
Code review re-ACK cd95c9d6a7ec08cca0f9c98328c759be586720f8
Reviewed:
```
git range-diff d62c2d82e14d27307d8790fd9d921b740b784668..7e80030a952a06101d5755032ebb1ff15823815e 5b8046a6e893b7fad5a93631e6d1e70db31878af..cd95c9d6a7ec08cca0f9c98328c759be586720f8
```
- 2fd3f2fec67a3bb62378c286fbf9667e6fb3cc3b added
- bb9ffea53fb021580f069c431aee02f547039831 added
- df7a52241f7dd7e995e5971abf67c293fc667144 → 174bd43f2e46a0ccc6f5ad486bb587c72c1241c3 was missing a `subprocess_close`

🚀 hebasto merged a pull request: "subprocess: Backport upstream changes"
(https://github.com/bitcoin/bitcoin/pull/32358)

👋 hebasto's pull request is ready for review: "Reintroduce external signer support for Windows"
(https://github.com/bitcoin/bitcoin/pull/29868)

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2850723656)
Rebased and undrafted.

📝 rkrux opened a pull request: "psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows"
(https://github.com/bitcoin/bitcoin/pull/32419)
The unserialization flows of the PSBT types work based on few underlying assumptions of functions from `serialize.h` & `stream.h` that takes some to understand when read the first time.

Add few comments that highlight these assumptions hopefully making it easier to grasp. Also, mention key/value format types as per BIP 174.

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-rel
...

✅ rkrux closed a pull request: "descriptor: handle listdescriptors(private=true) for descriptors having partial keys"
(https://github.com/bitcoin/bitcoin/pull/32186)

💬 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