💬 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_r1595247894)
This moves the responsibility of avoiding unintentional tx dependencies from the caller to this function instead. Is this the right way to understand this?

💬 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_r1595251873)
For my understanding: I get that having an ephemeral wallet created and destroyed inside this function gets rid of any data sharing related to `MiniWallet` between the calls of `fill_mempool()`. Since the same tag name is being passed and subsequently the same `internal_key` is generated, how does adding the tag name really help here?

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2102384508)
@Sjors
> Is there something you can remove from cpp-subprocess based on your comment? [#29961 (comment)](https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2100521158)

`Popen::poll()` and `Popen::pid()` might go.

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2102395740)
> In either case, external signer is also not special...

At some point, we might want to get rid of the `#ifdef ENABLE_EXTERNAL_SIGNER` all together.

💬 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_r1595273059)
I wouldn't say it disambiguates between separate `fill_mempool` calls, just between `fill_mempool` and the other `MiniWallet`s. I suppose we can add a parameter to modify the tag name further, but we don't have any functional tests where we do this multiple times.

💬 fanquake commented on pull request "build: swap cctools otool for llvm-objdump":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2102411411)
Guix build (aarch64):
```bash
67f4db93b4f985590948dc344306042841ab747b40ad73bf97ba62d77eb7bfd4 guix-build-7f5ac4520d15/output/aarch64-linux-gnu/SHA256SUMS.part
62e019f164d3c8cf8a1a649b2caa500dd85b00a319910feaf97658020b8cd1e3 guix-build-7f5ac4520d15/output/aarch64-linux-gnu/bitcoin-7f5ac4520d15-aarch64-linux-gnu-debug.tar.gz
c2227a058cf91c0bc3eb244cb254c3c6fcecddc21ba619bca8839d9d79bb1961 guix-build-7f5ac4520d15/output/aarch64-linux-gnu/bitcoin-7f5ac4520d15-aarch64-linux-gnu.tar.gz
e794a5
...

💬 instagibbs commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1595286955)
done

🤔 glozow reviewed a pull request: "test: add conflicting topology test case"
(https://github.com/bitcoin/bitcoin/pull/30066#pullrequestreview-2047737230)
ACK fba1565f4b6bafbf2516f03184cf58aa80d9843f

💬 craigraw commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2102475636)
Implemented support in Sparrow in https://github.com/sparrowwallet/sparrow/commit/d420c71673de5823ba1f6c4a0d4411ebd747c7ba, currently following the approach here to use testnet3 when run with `-n testnet` and testnet4 with `-n testnet4`. Wallets are loading successfully using either the mempool.space Electrum public server or directly with Bitcoin Core RPC.

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