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

📝 instagibbs opened a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072)
First few commits of https://github.com/bitcoin/bitcoin/pull/28984 to set the stage for the package RBF logic.

No behavior changes aside from bailing earlier from failed carve-outs.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#issuecomment-2102717549)
suggested by @glozow , can close if this ends up not being helpful

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595480359)
b86487ccf2e3d43da3d3ce8c3d2e2510677eb9ec

I wonder if `PaysForRBF` and `ImprovesFeerateDiagram` should be swapped in the order of checks?

`ImprovesFeerateDiagram` is a more expensive, is a superset of the rules, and a less intuitive error (I'd have an easier time guessing what went wrong with "pays less fees" vs "does not improve feerate diagram". Especially looking at the functional tests which switched out the error strings due to this check being earlier.