💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592095735)
Seemed unnecessary? Representing this as a byte vector allows us to create an `XOnlyPublicKey` directly without an extra function call. Is there a reason to prefer having this as a string literal?

💬 hebasto commented on pull request "build, test, doc: Temporarily remove Android-related stuff":
(https://github.com/bitcoin/bitcoin/pull/30049#issuecomment-2097841736)
> I guess the idea is that there's basically nothing worth salvaging from our current Android build procedure?

The commit history keeps everything we might need when restoring Android builds.

💬 maflcko commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592108480)
Just a style-nit, as it makes it easier to grep and compare the string literal. The function call could be moved to compile time, but I haven't implemented it yet. Calling it once at runtime should be fine also, for now?

Just a nit in any case.

💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2097902146)
> This function feels kinda weird. As it's named `ApplyTapTweak`, I would expect it to modify `*this`. It also takes in a `uint256* merkle_root` but doesn't check for null, a reference would make more sense.
>
> Perhaps this would make more sense as a static utility function that takes input/output keys?

Utility function might make more sense here: could do a utility function with `secp256k1_keypairs` as in/out args:

```
int compute_taptweak(secp256k1_keypair in, unsigned char merkle_r
...

💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1592155826)
Ah cool, my slight aversion to `ParseHex` was the call at runtime for a static const, but if we are going to move that to compile time and the string literal makes it easier to compare (e.g. with the value in BIP341, which is also a string literal), then happy to change it.

💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#issuecomment-2097928134)
Added a comment per @ajtowns 's feedback and reverted to using `ParseHex` on a string literal per @maflcko 's feedback.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2097968387)
> That said, it is a little rough to review as
> It needs to be compared to the letter of the spec, with the assumption that @laanwj is evil or has mis-implemented (I doubt that :)

If you're more comfortable comparing it against another implementation there's:
- A kinda haphazard windows implementation here: https://github.com/moonlight-stream/GS-IPv6-Forwarder/blob/master/GSv6Fwd/pcp.cpp
- Miniupnpd's pcp server implementation https://github.com/miniupnp/miniupnp/blob/master/miniupnpd/pc
...

💬 craigraw commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1592183994)
Typo here - should be `Use the testnet4 chain.`

👍 rkrux approved a pull request: "test: added test coverage to loadtxoutset could not open file"
(https://github.com/bitcoin/bitcoin/pull/30053#pullrequestreview-2042752271)
ACK [ee67bba](https://github.com/bitcoin/bitcoin/pull/30053/commits/ee67bba76cca2355541f99bb731f58479981b29e)

Make successful, unit and functional tests successful.
Short and sweet!

💬 rkrux commented on pull request "test: added test coverage to loadtxoutset could not open file":
(https://github.com/bitcoin/bitcoin/pull/30053#discussion_r1592215194)
Got to learn about the functionality of the slash operator in case of Pathlib.Path!
https://docs.python.org/3/library/pathlib.html#operators

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1592341198)
Good catch. I don't think I would open a new PR that just removing an unnecessary #include without having a larger goal, but I wouldn't object to reviewing one. Maybe the include fixes could be part of #29252, too.

📝 TheCharlatan converted_to_draft a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252)
The key module's functionality is not used by the kernel library, but currently kernel users are still required to initialize the key module's `secp256k1_context_sign` global as part of the `kernel::Context` through `ECC_Start`. So move the `ECC_Start` call to the `NodeContext` ctor instead to completely remove the key module from the kernel library.

The gui tests currently keep multiple `NodeContext` objects in memory, so call `ECC_Stop` manually to avoid triggering an assertion on `ECC_Star
...

💬 Christewart commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1592480884)
Isn't this trivially true from this: https://github.com/bitcoin/bitcoin/pull/28676/files#diff-7dcb5aa190f52e5da3aca406127995f65dab3ee9d33ab68130cb3460d2298c7eR498

💬 instagibbs commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#issuecomment-2098415750)
ACK https://github.com/bitcoin/bitcoin/pull/29292/commits/78e52f663f3e3ac86260b913dad777fd7218f210

only the suggested changes, verified via `git range-diff master 311f523 78e52f6`

💬 naiyoma commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2098422207)
I've attempted to run the tests locally, but encountered an error:
Restarting node(`self.restart_node(2, extra_args=['-reindex-chainstate=1', *self.extra_args[2]])`
) fails with this error:

```
test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status -6 during initialization. ./validation.h:608 CoinsDB: Assertion `m_coins_views' failed.
************************
```
Feel free to ignore this, as it's not exclusive to the tests you've added; it also occurs in othe
...

💬 maflcko commented on issue "Apple Clang 14.0 lacks support for `std::is_eq`":
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2098424144)
Anything left to be done here?

✅ maflcko closed an issue: "Apple Clang 14.0 lacks support for `std::is_eq`"
(https://github.com/bitcoin/bitcoin/issues/29918)

💬 maflcko commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2098434187)
Is this waiting for someone's review, or are two ACKs sufficient?

👍 rkrux approved a pull request: "test: Assumeutxo: snapshots with less work should not be loaded"
(https://github.com/bitcoin/bitcoin/pull/29428#pullrequestreview-2043225133)
crACK, utACK [2f1b1eee](https://github.com/bitcoin/bitcoin/pull/29428/commits/2f1b1eee8b8a8f546fb5975ac529c4032e46f069)

Thanks for adding the test, it's short and to the point. However, I've not been able to build the branch because it fails on my system (MAC M1) with the following error. I understand it is because this branch doesn't contain the latest changes of the master branch, notably this one: https://github.com/bitcoin/bitcoin/pull/29081.

```
util/time.cpp:54:9: error: use of unde
...

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2098527381)
`ComputeTapTweak` is more like what I had in mind, yes.