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

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

Not directly related to this PR, but I think the code comment above is basically out of date. Initial idea for `kernel::Context` was to have it hold a collection of variables that kernel code needed to run, so kernel code could use the context struct instead of global variables, and application code would still be convenient to write.

But this was before Options structs were introduced, and
...

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

I don't think it's a great idea to add these ECC_Start / ECC_Stop calls here, because this prevents writing lighter-weight tests that may need to pass a `NodeContext` argument to RPC or Init functions, but don't need ECC. Also calling ECC_Start in the constructor makes it not possible for different NodeContext instances to be constructed at the same time. So for example you couldn't write a te
...

💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1592548893)
re: https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1591560587

> This is really unfortunate, but I can't think of anything less hack-ish. And at least it's relegated to the test code.

I debugged this, and the reason this is needed is that `node.kernel.reset()` calls during shutdown (in `Shutdown()` and `BasicTestingSetup::~BasicTestingSetup`) are no longer calling `ECC_Stop`. If those places are updated to still shut down ECC like before, this could go away.

💬 Christewart commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1592626258)
How are users of the RPC expected to obtain `cluster_id`?

IIUC, the way to do this currently is to query

```
./bin/bitcoin-cli getrawmempool true
```

and then aggregate the `cluster_id`'s yourself?

👋 TheCharlatan's pull request is ready for review: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252)

💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2098604388)
> 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? :)

Yeah, I was getting an icky feeling while updating the PR for 2 reasons: `NO_LIMIT` of 0 is crappy and also the caller needs to make sure to use the same `CharLimit` for `Decode` and again with `LocateErrors`, which seems foot-gunny.

My ideal solution is `Decode`/`LocateErrors` shouldn't have a character limit at all (or use 10
...