🤔 TheCharlatan reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2814380926)
Approach ACK

I've been thinking about this approach over the approach @darosior suggested in #30983 again, and think this would indeed be cleaner. While providing multiprocess binaries over a known interface (e.g. just invoking the current binaries) could be beneficial in the initial roll out, I also like how this binary could potentially be the point where everything is packaged together if we ever do choose to go the full process + repository separation route. I think combining multiprocess
...

💬 TheCharlatan commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2073194474)
I think it would be good to check that the file actually exists here. If in future releases more commands are added, and the user applies a command for it on a binary directory with the old version, the error is a bit terse. E.g. removing bitcoind will currently result in:
```
./bitcoin daemon --signet
Error: execvp failed to execute '/home/drgrid/bitcoin/build_dev_mode_clang/bin/bitcoind': No such file or directory
Try './bitcoin --help' for more information.
```
We already print "Unrecog
...

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2073215058)
Should it be checking all the manifests or is just checking bitcoind.exe's enough?

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2850592767)
We might also want to add a check for correct manifests to the symbols / security checks in the guix build. No idea if LIEF makes this easy or not.

💬 purpleKarrot commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2850642681)
The best approach to respect user-provided flags is to treat **all** `CMAKE_...` variables as **read-only**.
The only place where those variables may be set, is *outside the project* (ie: env vars, toolchain files, initial cache, cli arguments).

Consider you have a function in C. Inside that function, you have to perform a division by one of the arguments. Since you cannot divide by zero, you treat the value `0` *as if* the user provided the value `2` instead. Would you consider that a valid
...

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