💬 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

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2073386754)
I see. Done. I also moved the `m_was_written = true` to be just after the `fwrite()` calls.

💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2850907232)
@laanwj

```
[12:43:30.811] contrib/devtools/symbol-check.py:308: error: No overload variant of "int" matches argument type "RESOURCE_TYPES" [call-overload]
[12:43:30.811] contrib/devtools/symbol-check.py:308: note: Possible overload variants:
[12:43:30.811] contrib/devtools/symbol-check.py:308: note: def __new__(cls, str | Buffer | SupportsInt | SupportsIndex | SupportsTrunc = ..., /) -> int
[12:43:30.811] contrib/devtools/symbol-check.py:308: note: def __new__(cls, str | bytes
...

💬 Sjors commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#issuecomment-2850943519)
I limited the exception to regtest. I also found two tests that for some reason implement their own coinbase construction code. I adjusted those for consistently with the first commit.

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2850944121)
`2c1a8ff0c0...61ba3fc494`: fix CI

💬 ajtowns commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#discussion_r2073418791)
Match the comment to the updated code too?

📝 hebasto converted_to_draft a pull request: "cmake: Add application manifests when cross-compiling for Windows"
(https://github.com/bitcoin/bitcoin/pull/32396)
Windows [application manifests ](https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests) provide several benefits—such as enhanced security settings, and the ability to set a process-wide code page (required for https://github.com/bitcoin/bitcoin/pull/32380), as well as granular control over supported Windows versions. Most of these benefits lie beyond the scope of this PR and will be evaluated separately.

On the current master branch @ fc6346dbc8dc3db40aad4079210332b5f8b
...

💬 Sjors commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2850994834)
@l0rinc ah, I did have an `addnode` with a local peer in my config file for this otherwise fresh node. Let me try again without that.

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2073451336)
Without the `int()` this doesn't work at all. Maybe just harcode `24`.

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2851026294)
`write_buffer` was also marked as dirty in latest push.

untested ACK 61ba3fc494f2f5e60b668316c84cecbdc123e5fa

Q: shouldn't `Truncate` and `Commit` also set `m_was_written = true`?

💬 Sjors commented on pull request "miner: don't needlessly append dummy OP_0 to bip34":
(https://github.com/bitcoin/bitcoin/pull/32420#discussion_r2073461584)
Fixed

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2073462754)
Also mind that newer verions of LIEF completely reorganize these constants (it's called `lief.PE.ResourcesManager.TYPE.MANIFEST` now). Had to figure out what it was called for this version then it didn't even work as-is.

💬 TheCharlatan commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851053267)
Concept ACK

I think I'd prefer keeping the option until #32401 is addressed. Clearly there seems to be at least some demand for mining on charitable templates, if this is worth the additional code and maintenance is another discussion.

👍 TheCharlatan approved a pull request: "build: replace header checks with `__has_include`"
(https://github.com/bitcoin/bitcoin/pull/32405#pullrequestreview-2814879581)
ACK e1f543823b300b28c9edaf5d1a3e1e9badde471b

```
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-lin
...

🤔 musaHaruna reviewed a pull request: "mining: rename gbt_force and gbt_force_name"
(https://github.com/bitcoin/bitcoin/pull/32386#pullrequestreview-2814893880)
ACK [5e87c3e](https://github.com/bitcoin/bitcoin/pull/32386/commits/5e87c3ec094d68a7a27dfb7ae665b225ff4dfdb6)
After reading [BIP9](https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate-changes), I think variable rename from gbt_force to gbt_optional_rule makes the intent and purpose clear as it's stated in the specification