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

💬 theuni commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2098532940)
I think the original bug here illustrates that the enum is not well suited for this. Definitely not with a value of 0 for NO_LIMIT anyway. Got any other ideas? :)

🤔 ryanofsky reviewed a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2042988944)
Code review a885a166cec6d84d08600f12b25d912bd28af80e. This is good, change, but I think it has a few issues pointed out in comments below. At a high level I think it makes sense to decouple ECC initialization from the kernel context struct, but I don't think it makes sense to couple it so tightly with the node context struct.

Would suggest an approach more like the this branch: [pr/ecc](https://github.com/ryanofsky/bitcoin/commits/pr/ecc) with commits:

- c6b4b7c050393b4442f155d40c2a7f586eb
...

💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592362999)
In commit "kernel: Remove key module from kernel library" (a885a166cec6d84d08600f12b25d912bd28af80e)

Is there a reason to suppress the lint warning instead of just deleting the destructor, since the destructor does not do anything? If the destructor is needed for some reason, it might be good to clarify in a comment.