💬 maflcko commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2671608757)
For reference, the compiler command (without docker) is:

```
# /usr/bin/clang-20 -DUSE_ASM_X86_64=1 -std=c90 -o secp256k1.c.o -c /bitcoin/src/secp256k1/src/secp256k1.c -fsanitize=address
In file included from /bitcoin/src/secp256k1/src/secp256k1.c:28:
In file included from /bitcoin/src/secp256k1/src/scalar_impl.h:20:
/bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
356 | "movq 32(%%rsi), %%r11\n"
| ^
1 error
...

💬 sipa commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2671620363)
This looks familiar. Can you try if `-fomit-frame-pointer` fixes it?

💬 ryanofsky commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1963647337)
In commit "qt: remove UPnP settings" (844770b05ebc34789dc46d70cd6398089539c915)

It probably would be better to replace this line with:

```c++
if (settings.contains("fUseUPnP")) {
if (node().getPersistentSetting("upnp").isNull()) UpdateRwSetting(node(), "upnp", "", settings.value("fUseUPnP").toBool());
settings.remove("fUseUPnP");
}
```

So the setting would still be migrated and a warning could be logged if it was enabled.

💬 laanwj commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2671638099)
@ryanofsky For this specific setting it isn't too important imo. It would be critical for a disappearing privacy setting, but not for a setting that opens up more network surface.

But i do think it would be nice to warn once before dropoing it from `settings.json`. i mean our goal with this change is to encourage people to enable port forwarding to have more connectable nodes. So a reminder to use NAT-PMP instead instead is good.

💬 hebasto commented on pull request "cmake: Fix `-pthread` flags in summary":
(https://github.com/bitcoin/bitcoin/pull/31724#discussion_r1963652393)
1. It is our practise to check whether flags are supported and required, isn't it?
2. In the Autotools-based build system, we used the [`AX_PTHREAD`](https://www.gnu.org/software/autoconf-archive/ax_pthread.html) macro for that purpose.
3. The [`FindThreads`](https://cmake.org/cmake/help/latest/module/FindThreads.html) module is a CMake's way of achieving the same.
4. The current issue on the master branch is not about the thread-related flags for the toolchain but rather their presentation i
...

💬 ryanofsky commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2671654050)
Relatedly, I think QSetttings migration code could be also be improved: https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1963647337

And I think the warning message could be much more descriptive because it's possible someone enabled this setting a long time ago without a full understanding of what it does, and the message doesn't provide any context. Might suggest a warning like:

- The -upnp setting is enabled, but this setting is no longer supported as of Bitcoin Core 0.29. This mean
...

💬 darosior commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1963665903)
I'm on the fence. I agree it's nicer but also it would 10x the size of the change with harder to review code. Thanks for the commit but i'd rather keep it this way for now, unless/until more reviewers believe it's worth pulling in a full varint implementation.

💬 ryanofsky commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2671661174)
> But i do think it would be nice to warn once before dropping it from `settings.json`. i mean our goal with this change is to encourage people to enable port forwarding to have more connectable nodes. So a reminder to use NAT-PMP instead instead is good.

This also does sound like a good idea, and should not be too hard to implement. though not quite as easy as replacing InitWarning with LogWarning

💬 laanwj commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2671668235)
Another alternative would be to automatically "upgrade" `-upnp` to `-natpmp` . i don't think most users care about what method of automatic port forwarding method is used?

🚀 fanquake merged a pull request: "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree"
(https://github.com/bitcoin/bitcoin/pull/31379)

💬 dergoegge commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2671692633)
> Can you try if -fomit-frame-pointer fixes it?

Still the same issue.

```
root@7069ec4b9a4e:/# clang-19 -DUSE_ASM_X86_64=1 -std=c90 -o secp256k1.c.o -c /bitcoin/src/secp256k1/src/secp256k1.c -fsanitize=address -fomit-frame-pointer
In file included from /bitcoin/src/secp256k1/src/secp256k1.c:28:
In file included from /bitcoin/src/secp256k1/src/scalar_impl.h:20:
/bitcoin/src/secp256k1/src/scalar_4x64_impl.h:356:5: error: inline assembly requires more registers than available
356 | "mov
...

💬 sipa commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2671699393)
I believe the explanation here is just that they're incompatible. The asm code in libsecp256k1 requires many registers; an amount which is generally available in builds. But presumably, asan instrumentation reduces the set of general-purpose available register from use in the instrumentation itself, and as a result the code doesn't compile anymore.

💬 maflcko commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963698596)
Also: https://issues.oss-fuzz.com/issues/397734700:

```
Issue 397734700: bitcoin-core:base58check_encode_decode: Timeout in base58check_encode_decode

💬 theStack commented on pull request "qa: clarify and document assumeutxo tests":
(https://github.com/bitcoin/bitcoin/pull/31907#discussion_r1963712350)
Fair enough, doesn't have to happen in this PR (if at all), happy to ACK without (once https://github.com/bitcoin/bitcoin/pull/31907#issuecomment-2669777720 is fixed). Still, I think it would be nice long-term if varints would be easy to change/create, without having to fiddle around in the C++ codebase to get out the serialization.

💬 marcofleon commented on pull request "test: cover base[32|58|64] with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1963718683)
Looking into this. Was there any reason to change `max_ret_len` from 100 to 1000?

💬 darosior commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r1963724337)
The fuzz mock `hash[31] & 0x80 == 0` is equivalent to the pow check against regtest min difficulty used in the unit test. If one passes the other necessarily does. This mean both will mine the same chain. You can check this as they both arrive to the same block hash at height 200, with the same utxo set hash.

👍 TheCharlatan approved a pull request: "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally"
(https://github.com/bitcoin/bitcoin/pull/31662#pullrequestreview-2630129384)
Re-ACK 2c4b229c906de6250500d3af2b44808e90b9ce0b

💬 maflcko commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2671744163)
You can also set `-O1` instead of `-O0`, but I guess you don't want that?

💬 dergoegge commented on issue "build: x86 ASan build broken "error: inline assembly requires more registers than available"":
(https://github.com/bitcoin/bitcoin/issues/31913#issuecomment-2671763581)
I wonder if this is related to it: https://github.com/llvm/llvm-project/blob/1c4e9863fa1cba6d28be92026e0912808b01780d/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp#L3096-L3107. Currently limited to 32-bit but maybe it's also broken for 64-bit?

> You can also set -O1 instead of -O0, but I guess you don't want that?

`-O1` works but `-O0` does not. Could be a workaround as well.

Interestingly using `afl-clang-{lto,fast}` does not fail to compile `/bitcoin/src/secp256k1/src/secp256k1.c
...

📝 stephenmiracle4 opened a pull request: "i deleted a commented line in invalid_signer.py file"
(https://github.com/bitcoin/bitcoin/pull/31914)
<!--
*** Please remove the following help text before submitting: ***

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

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...