💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2102476656)
@craigraw that might be premature, if we decide to do another genesis block.

💬 craigraw commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2102485440)
@Sjors It shouldn't matter - if the network is reset, the existing wallet history will simply disappear when the wallet is reloaded. I will certainly warn of the possibility of a future network reset on release.

💬 rkrux commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1595337005)
> where we do this multiple times

💬 glozow commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1595338181)
`fill_mempool`

👍 stickies-v approved a pull request: "rpc: Remove index-based Arg accessor"
(https://github.com/bitcoin/bitcoin/pull/29997#pullrequestreview-2047778445)
ACK fadb3eb57b4d207a678067b89caa45abf1f93702

Can't think of any situations where index-based is preferable over key-based, so this seems like a nice cleanup.

💬 rkrux commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1595358454)
Oh I see.

💬 kevkevinpal commented on pull request "test: Assumeutxo: ensure failure when importing a snapshot twice":
(https://github.com/bitcoin/bitcoin/pull/29973#issuecomment-2102549327)
tACK [b259b0e](https://github.com/bitcoin/bitcoin/pull/29973/commits/b259b0e8d360726b062c4b0453d1cf5a68e1933f)

made a similar PR not knowing this existed https://github.com/bitcoin/bitcoin/pull/30068

🤔 stickies-v reviewed a pull request: "rpc: Remove index-based Arg accessor"
(https://github.com/bitcoin/bitcoin/pull/29997#pullrequestreview-2047854683)
Oops, left my nits on the wrong PR. Nothing blocking, only if you re-touch.

💬 stickies-v commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1595387427)
nit: this is just the brief, so should be removed/merged imo (+ for MaybeArg)

💬 stickies-v commented on pull request "rpc: Remove index-based Arg accessor":
(https://github.com/bitcoin/bitcoin/pull/29997#discussion_r1595388346)
nit: there's still a remaining reference to `Arg<Type>(i)` in `CheckRequiredOrDefault`:

<details>
<summary>git diff on fadb3eb57b</summary>

```diff
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index f683878054..d4079d9f37 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -673,7 +673,7 @@ static const UniValue* DetailMaybeArg(CheckFn* check, const std::vector<RPCArg>&

static void CheckRequiredOrDefault(const RPCArg& param)
{
- // Must use `Arg<Type>(i)` to get the
...

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1595390078)
Please consider the above a doodle, change it however you'd like. I just wanted to sketch out the concept of a wrapped `keypair`.

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-2102601696)
Thank you for the review @ryanofsky!

a885a166cec6d84d08600f12b25d912bd28af80e -> 7dc7938731233f5f3299618e1e07ff92782a7cfd ([kernelRmKey_2](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_2) -> [kernelRmKey_3](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmKey_2..kernelRmKey_3))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2042988944), cherry-picked th
...

💬 josibake commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1595408180)
Yeah, for sure! But thanks, very helpful doodle. Not sure why I was so apprehensive about passing around a keypair, but seeing you write it out helped it click for me.

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-2102676859)
Rebased 7dc7938731233f5f3299618e1e07ff92782a7cfd -> 65a205166b3c7d082a7528ea5036dff551d7eb6e ([kernelRmKey_3](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_3) -> [kernelRmKey_4](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_4), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmKey_3..kernelRmKey_4))

💬 vasild commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#issuecomment-2102685727)
`91c90d7595...6bfda84b92`: rebase, pick changes from https://github.com/bitcoin/bitcoin/pull/29421 and adjust the newly added test.

👍 ryanofsky approved a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2047986415)
Code review ACK 65a205166b3c7d082a7528ea5036dff551d7eb6e

💬 ryanofsky commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1595470766)
In commit "Refactor: Remove ECC_Start and ECC_Stop from key header" (65a205166b3c7d082a7528ea5036dff551d7eb6e)

It's probably good to remove these declarations, but I think it would be good to move the documentation to the cpp file so it is not lost. Notes like "May not be called twice" "No-op if ECC_Start wasn't called" seem useful to describe how these are intended to work.

Also references to these function in the ECC_Context comment below should be removed if they are private. Maybe repl
...

💬 instagibbs commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1595483759)
and specifically, the conflict is happening between a transaction in the mempool already, and a transaction in the package

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2102707649)
In terms of which IPv6 address to use, my understanding is this:

1. In the olden days the IPv6 address contained part of the MAC address. This ensured uniqueness, but was bad for mobile device privacy; no matter where you went, part of your IP address was constant. Such addresses always (?) contain `0xfffe`. See https://datatracker.ietf.org/doc/html/rfc7707
2. A new standard was introduced that uses a hash of both the MAC address and the prefix, ensuring it's stable if you stay on the same n
...

🤔 brunoerg reviewed a pull request: "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29939#pullrequestreview-2048018941)
utACK dd8fa861939d5b8bdd844ad7cab015d08533a91a

Code lgtm and I do not see any problem with the "tag", and seems good for determinism.