💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#discussion_r1963559614)
> What makes the secp subtree special enough to get its own header?

For other subtrees, it has not yet been decided whether their own build scripts will be used.

🤔 marcofleon reviewed a pull request: "fuzz: add targets for PCP and NAT-PMP port mapping requests"
(https://github.com/bitcoin/bitcoin/pull/31676#pullrequestreview-2629915256)
ACK c73b59d47f1ec6fff1ad9155181c2285a5ef5cf4

I'm not seeing any slow units with the corpuses I have and when I fuzz with afl++ stability looks very good (98-99%). It could be that I have to fuzz longer for it to appear. I think the new fuzz tests are good to go for now. If issues (timeout, etc.) come up, we can address them in a followup?

Also, left one comment below to improve `FuzzedSock` a bit but it's not a blocker.

💬 marcofleon commented on pull request "fuzz: add targets for PCP and NAT-PMP port mapping requests":
(https://github.com/bitcoin/bitcoin/pull/31676#discussion_r1963601055)
To make this more accurate, we could have it advance by the full timeout only when the event fails to occur. And then consume from 0 to `timeout` for when the event succeeds. Same for `WaitMany` below.

💬 ryanofsky commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2671595439)
I'm not sure there is a strong justification for the `-upnp` option to trigger an interactive warning now, if it was always just a best-effort setting that wouldn't previously fail if it couldn't register with the upnp server., or the server wouldn't provide a port or external ip. Since anyone relying on this would need external monitoring or testing to know if it was working anyway, just using LogWarning here seems like it should be ok.

But if I'm underestimating the importance of having an in
...

💬 rkrux commented on pull request "descriptor: check whitespace in keys within fragments":
(https://github.com/bitcoin/bitcoin/pull/31603#issuecomment-2671597288)
> Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment

On master, I am not able to successfully import a descriptor or get the descriptor info in case a space is passed in the desc. Is this statement incorrect or is there any other flow I don't know about?

<details>
<summary>
`getdescriptorinfo` and `importdescriptors` with both public key and private keys on master
</summary>

```
➜ src git:(master)
...

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