💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077252899)
Why do we need to use a third-party repository, that runs some Javascript, to configure a Windows Command Prompt?

💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077259998)
Why do we need this check? Regex over a copy-pasted blob of XML is pretty meh. We aren't really checking anything important here either?

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077268171)
Currently, no, but after #32380 it becomes critical because it determines the process code page.

> (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 https://github.com/bitcoin/bitcoin/pull/32380, it's really important for them to be in the release too)

This is a quick and dirty check that the manifest is added and is what we expect, feel free to add detailed XML parsing.

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077273311)
It also already found one bug in this PR: the manifest was not added to `test_bitcoin.exe` .

💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077274476)
I think for now we could just assert `binary.resources_manager.has_manifest`, if we care that this is being added? Could save anything else for if/when it's actually being used?

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077279482)
i don't see the problem with having it right now like this, but okay.

💬 laanwj commented on pull request "lint: Remove string exclusion from locale check":
(https://github.com/bitcoin/bitcoin/pull/32434#issuecomment-2858004369)
> The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().

i wonder if the same is true for the comment exclusion. Would it detect `/* bla */ wrong()`.

💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2077314447)
One thing i did think about is using the input file (`make/windows-app.manifest.in`) as template instead of hardcoding it here, but having the check depend on files scattered throughout the repository is probably not that great, either.

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2077317447)
Can be marked as resolved, thanks! I think we were talking past each other a bit but resolved offline.

⚠️ maflcko opened an issue: "intermittent issue in wallet_listsinceblock.py ( test_framework.test_node.FailedToStartError: [node 3] bitcoind exited with status -6 during initialization.)"
(https://github.com/bitcoin/bitcoin/issues/32435)
-6 seems to indicate an abort. However, the node did not print anything at all to the log, nor to the stderr:

https://github.com/bitcoin/bitcoin/actions/runs/14879952737/job/41785504218?pr=32430#step:7:3527

```
186/266 - wallet_listsinceblock.py failed, Duration: 1 s

stdout:
2025-05-07T09:47:50.041000Z TestFramework (INFO): PRNG seed is: 2493267510557761438
2025-05-07T09:47:50.044000Z TestFramework (INFO): Initializing test directory /Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/t
...

✅ maflcko closed an issue: "intermittent issue in wallet_listsinceblock.py ( test_framework.test_node.FailedToStartError: [node 3] bitcoind exited with status -6 during initialization.)"
(https://github.com/bitcoin/bitcoin/issues/32435)

💬 maflcko commented on issue "intermittent issue in wallet_listsinceblock.py ( test_framework.test_node.FailedToStartError: [node 3] bitcoind exited with status -6 during initialization.)":
(https://github.com/bitcoin/bitcoin/issues/32435#issuecomment-2858030829)
Not sure how to debug this further, so closing for now. However, further input is still very welcome.

💬 TheCharlatan commented on pull request "kernel: Separate UTXO set access from validation functions":
(https://github.com/bitcoin/bitcoin/pull/32317#issuecomment-2858043588)
Re https://github.com/bitcoin/bitcoin/pull/32317#pullrequestreview-2785006540

> nit: The name SpendBlock may not be clear (unless this concept is already known and I'm missing the point).
Maybe BIP30Validation, something like that, would be clearer.


It is doing more than BIP-30 validation, so I don't think that would accurate either. I thought the name was pretty clear for what it does, but I'd be open to other suggestions.

📝 theStack opened a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436)
This small PR gets rid of manual mod-n inversion of the ECDSA signature-s part in unit tests (introduced a long time ago in #5256, triggered by https://github.com/bitcoin-core/secp256k1/pull/69) by using secp256k1 instead. The function wasn't available at that time, but was introduced about three years later, see https://github.com/bitcoin-core/secp256k1/pull/408. Note that as the name suggests, `secp256k1_ec_seckey_negate` is meant to be used for secret keys, but it obviously works in general f
...

💬 Crypt-iQ commented on pull request "fuzz: Remove unused TimeoutExpired catch in fuzz runner":
(https://github.com/bitcoin/bitcoin/pull/32388#issuecomment-2858082209)
crACK fa4804009ceba96926edd7f55ea22442ebdc665d

Just curious, why did adding the `read_file` fallback cause the timeout to be unused? I can't really speak to the Windows side-effect as I don't totally understand it.

👍 rkrux approved a pull request: "docs: Improve `keypoolrefill` RPC docs"
(https://github.com/bitcoin/bitcoin/pull/32429#pullrequestreview-2821296596)
ACK cd00fb85e9e0997120715e68374462cff0dad7eb given the suggestions are addressed.

I checked that by default there are 4 scriptpubkeymans setup on the first run of the wallet inside `SetupDescriptorScriptPubKeyMans`. Also, ack because the lack of this information has caused an issue to be created.

🚀 fanquake merged a pull request: "fuzz: Remove unused TimeoutExpired catch in fuzz runner"
(https://github.com/bitcoin/bitcoin/pull/32388)

📝 fanquake opened a pull request: "crypto: disable ASan for sha256_sse4 with Clang"
(https://github.com/bitcoin/bitcoin/pull/32437)
This also fails to compile when optimisations are being used, see: https://github.com/bitcoin/bitcoin/issues/31913.
So just disable ASan under any optimisation level.

Closes #31913.

💬 rkrux commented on pull request "lint: Remove string exclusion from locale check":
(https://github.com/bitcoin/bitcoin/pull/32434#issuecomment-2858137223)
> > The exclusion isn't needed. In fact, it prevents detection of "bla" + wrong().
>
> i wonder if the same is true for the comment exclusion. Would it detect `/* bla */ wrong()`.

It seems to.

```bash
➜ bitcoin git:(keypoolrefill_docs) ✗ ./test/lint/lint-locale-dependence.py
The locale dependent function std::to_string(...) appears to be used:
src/wallet/rpc/addresses.cpp: "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \
...

🤔 rkrux reviewed a pull request: "lint: Remove string exclusion from locale check"
(https://github.com/bitcoin/bitcoin/pull/32434#pullrequestreview-2821339543)
Maybe can mention the `to_string` function in the docs as well? It's already a part of the linter.

https://github.com/bitcoin/bitcoin/blob/59d3e4ed34eb55cb40928d524cb0bd5e183ed85a/doc/developer-notes.md?plain=1#L1022-L1042