💬 hebasto commented on pull request "build: Fix false positive `CHECK_ATOMIC` test for clang-15":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2053973941)
> The same error happens with clang-18 locally, but why does CI pass?
>
> [fad23a0](https://github.com/bitcoin/bitcoin/commit/fad23a06469607689c4f637bb407c96af4902a27)

Depends on standard C++ library version?

💬 laanwj commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-2053981804)
Concept ACK, i think it's important that signet gets its own launch icon if testnet has, it's more useful for testing than testnet. The other option would be to keep only the mainnet icon. But i think there's value in making windows users aware of these kind of things (so they don't reinvent wheels, or, test on mainnet 😅 ).

💬 fanquake commented on pull request "ci: use Clang 16 for Valgrind":
(https://github.com/bitcoin/bitcoin/pull/29848#discussion_r1564592864)
Added

💬 laanwj commented on issue "depens: bdb build fails on Intel macOS 13.6.6 ":
(https://github.com/bitcoin/bitcoin/issues/29792#issuecomment-2053992273)
Looks like it's resolved? As a rare issue with a dependency that's going away anyhow, i think this can be closed.

💬 laanwj commented on pull request "depends: suggest GNU patch for macOS <= 13":
(https://github.com/bitcoin/bitcoin/pull/29814#issuecomment-2053992693)
i would prefer not to require (nor suggest) any GNU tools on non-GNU operating systems, does this potentially affect any other deps besides bdb?

ACK on the way this is worded, anyhow. But we might just sit this out until bdb is gone.

🤔 maflcko reviewed a pull request: "depends: suggest GNU patch for macOS <= 13"
(https://github.com/bitcoin/bitcoin/pull/29814#pullrequestreview-1999542526)
Not sure. Anyone else on any version of macOS can build fine. So far only a single person ran into this issue, without steps to reproduce, so I am not sure about adjusting the docs.

💬 maflcko commented on pull request "depends: suggest GNU patch for macOS <= 13":
(https://github.com/bitcoin/bitcoin/pull/29814#discussion_r1564613112)
Installing it would be insufficient anyway, because you'd still need to overwrite the global default?

💬 maflcko commented on pull request "ci: use Clang 16 for Valgrind":
(https://github.com/bitcoin/bitcoin/pull/29848#issuecomment-2053999845)
lgtm ACK ad21f2294821d7c436e58a8f199fb555b11a56ad

💬 maflcko commented on pull request "build: Fix false positive `CHECK_ATOMIC` test for clang-15":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2054000372)
> > The same error happens with clang-18 locally, but why does CI pass?
> > [fad23a0](https://github.com/bitcoin/bitcoin/commit/fad23a06469607689c4f637bb407c96af4902a27)
>
> Depends on standard C++ library version?

I can re-try, but my findings didn't indicate that IIRC.

In any case, the title needs to be adjusted to remove the `clang-15`, no?

💬 hebasto commented on pull request "build: Fix false positive `CHECK_ATOMIC` test":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2054000744)
> In any case, the title needs to be adjusted to remove the `clang-15`, no?

Updated.

💬 maflcko commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2054000903)
Is this related to bbe82c116e72ca0638751e063bf564cd1fe5c4d5?

💬 furszy commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2054064278)
> Is this related to [bbe82c1](https://github.com/bitcoin/bitcoin/commit/bbe82c116e72ca0638751e063bf564cd1fe5c4d5)?

No, it is related to 0faafb57f8298547949cbc0044ee9e925ed887ba. This change is partially reverting it and documenting the flow so the regression does not happen again.

💬 laanwj commented on pull request "util: remove unused cpp-subprocess options":
(https://github.com/bitcoin/bitcoin/pull/29865#issuecomment-2054078250)
Concept ACK, but could it be that we could need some of these?
Eg `close_fds` sounds like what's described in #22417

✅ furszy closed a pull request: "Introduce 'getblockfileinfo' RPC command"
(https://github.com/bitcoin/bitcoin/pull/27770)

💬 furszy commented on pull request "Introduce 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-2054101285)
> I think this information is too low-level. In the past we've rejected changes that expose low-level information about our block storage because block files are not an external API, and exposing this kind of info limits our flexibility in regard to changing the block storage model.

Conceptually, I agree. But I don't think that the block storage model flexibility argument is the best for not doing this. We do have procedures tightly coupled to the current block storage model (e.g. the best ch
...

💬 michaelwklein commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2054102603)
Hi @hebasto, @achow101 thanks for moderating the repository. @joinfuel is a project that assigns rewards to GitHub issues, developers simply to connect their GitHub via joinfuel.io to obtain the rewards. Is there someone we can talk to in Developer relations to have these comments approved? The community and the project itself and assign rewards to GitHub issues to promote open source projects.

💬 laanwj commented on pull request "Introduce 'getblockfileinfo' RPC command":
(https://github.com/bitcoin/bitcoin/pull/27770#issuecomment-2054104846)
> Removing this new debug-only RPC command would be the least of our problems if we ever seek to change the storage model.

Sure, that's true. In that sense it's different than say, adding block file/pos to `getblock` info, as it doesn't affect the output of a common RPC.

📝 hebasto opened a pull request: "Reintroduce external signer support for Windows"
(https://github.com/bitcoin/bitcoin/pull/29868)
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/99.

Partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489

After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.hpp`.

💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1564859345)
> ci: remove --enable-external-signer from win64 job

Why this is being reverted? This is already applied globally to the CI (and I'd say pointlessly so).

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1564860366)
Thanks! Dropped.