💬 josibake commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#discussion_r1590763287)
See https://github.com/bitcoin/bitcoin/blob/246e2a24c5460504c799fbc6312ef075f0ca4e72/src/test/data/bip352_send_and_receive_vectors.json#L383 for an example of how the old method (a.hash < b.hash, a.vout < b.vout) will yield a different ordering than comparing outpoints lexicographically.

📝 josibake opened a pull request: "crypto: add `NUMS_H` const"
(https://github.com/bitcoin/bitcoin/pull/30048)
Broken out from #28122

---

[BIP341](https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#constructing-and-spending-taproot-outputs) defines a NUMS point `H` as *H = lift_x(0x50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0)* which is [constructed](https://github.com/ElementsProject/secp256k1-zkp/blob/11af7015de624b010424273be3d91f117f172c82/src/modules/rangeproof/main_impl.h#L16) by taking the hash of the standard uncompressed encoding of the [secp256k1](https:/
...

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1590788176)
opened in https://github.com/bitcoin/bitcoin/pull/30046

💬 maflcko commented on pull request "build, bench, msvc: Add missing benchmarks":
(https://github.com/bitcoin/bitcoin/pull/29773#issuecomment-2095609119)
An alternative would be to update the generation script?

📝 hebasto opened a pull request: "build, test, doc: Temporarily remove Android-related stuff"
(https://github.com/bitcoin/bitcoin/pull/30049)
Previously, our Android builds were geared towards generating APKs, which relied on Qt. However, after migrating to C++20, compiling for Android became unfeasible due to Qt 5.15's compatibility limitations with NDK only up to r25, which includes an outdated embedded libc++.

All removed stuff will be reinstated after migrating the build system to CMake and upgrading Qt to version 6.x."

This PR makes possible a clean migration to the CMake-based build system as it removes code, which is not
...

📝 hebasto converted_to_draft a pull request: "build, test, doc: Temporarily remove Android-related stuff"
(https://github.com/bitcoin/bitcoin/pull/30049)
Previously, our Android builds were geared towards generating APKs, which relied on Qt. However, after migrating to C++20, compiling for Android became unfeasible due to Qt 5.15's compatibility limitations with NDK only up to r25, which includes an outdated embedded libc++.

All removed stuff will be reinstated after migrating the build system to CMake and upgrading Qt to version 6.x."

This PR makes possible a clean migration to the CMake-based build system as it removes code, which is not
...

👋 hebasto's pull request is ready for review: "build, test, doc: Temporarily remove Android-related stuff"
(https://github.com/bitcoin/bitcoin/pull/30049)

💬 josibake commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#discussion_r1590805470)
Although, to your point, `a.hash < b.hash` is doing a lexicographic comparison:

https://github.com/bitcoin/bitcoin/blob/e623d8a05ad4e2b81e19c4f10b22e2ce4db35b8d/src/uint256.h#L54

So maybe it would be more efficient to just serialize `a.vout, b.vout` as little-endian here, rather than serializing the whole `COutPoint`?

💬 hebasto commented on pull request "build, bench, msvc: Add missing benchmarks":
(https://github.com/bitcoin/bitcoin/pull/29773#issuecomment-2095647236)
> An alternative would be to update the generation script?

Yes. However, in the light of coming migration to CMake, it makes no substantial difference, right?

👋 maflcko's pull request is ready for review: "build: Require libc++-16 or later"
(https://github.com/bitcoin/bitcoin/pull/29077)

💬 maflcko commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#discussion_r1590811175)
This is now at v11

💬 fanquake commented on pull request "build, test, doc: Temporarily remove Android-related stuff":
(https://github.com/bitcoin/bitcoin/pull/30049#discussion_r1590816532)
Need to drop applying this as well

💬 hebasto commented on pull request "build, test, doc: Temporarily remove Android-related stuff":
(https://github.com/bitcoin/bitcoin/pull/30049#discussion_r1590818277)
Thanks! Dropped.

💬 maflcko commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#discussion_r1590821935)
Thanks, clarified that "libstdc++ (any version) requires at least clang-16"

📝 josibake opened a pull request: "refactor, wallet: get serialized size of `CRecipient`s directly"
(https://github.com/bitcoin/bitcoin/pull/30050)
Broken out from #28201

---

In order to estimate fees properly, we need to know what the final serialized transaction size will be. This PR refactors `CreateTransactionInternal` to:

* Get the serialized size directly from the `CRecipient`: this sets us up in a future PR to calculate the serialized size of silent payment `CTxDestinations` (see https://github.com/bitcoin/bitcoin/pull/28201/commits/797e21c8c1bf393e668eeef8bb7ee9cae1e5e41e)
* Use the new `GetSerializeSizeForRecipient` to m
...

📝 maflcko converted_to_draft a pull request: "build: Require libc++-16 or later"
(https://github.com/bitcoin/bitcoin/pull/29077)
I am not familiar with macOS, but given that https://github.com/bitcoin/bitcoin/pull/28622 prescribes a minimum of libc++-16 for macOS, doing the same on Linux could make sense?

This is needed for stuff:

* `std::ranges::equal`, see https://github.com/bitcoin/bitcoin/pull/29071
* `std::source_location`, see https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1851949647

I don't think anyone is using `-stdlib=libc++` on Linux, except for the CI and OSS-Fuzz?

The minimum required
...

💬 maflcko commented on pull request "build, bench, msvc: Add missing benchmarks":
(https://github.com/bitcoin/bitcoin/pull/29773#issuecomment-2095738135)
lgtm ACK 31a15f0aff79d2b34a9640909b9e6fb39a647b60

📝 josibake opened a pull request: "crypto, refactor: add method for applying the taptweak"
(https://github.com/bitcoin/bitcoin/pull/30051)
The wallet returns an untweaked internal key for taproot outputs. If the output commits to a tree of scripts, this key needs to be tweaked with the merkle root. Even if the output does not commit to a tree of scripts, BIP341/342 recommend commiting to a hash of the public key.

Previously, this logic for applying the taptweak was implemented in the `CKey::SignSchnorr` method.

This PR moves this logic to its own method, `CKey::ApplyTapTweak` and refactors CKey::SignSchnorr` to use this new m
...

💬 Sjors commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#issuecomment-2095752757)
Don't forget to add a source comment that BIP352 depends on this exact ordering.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2095761769)
Concept ACK