🤔 rkrux reviewed a pull request: "test: assert can't activate snapshot based chainstate more than once"
(https://github.com/bitcoin/bitcoin/pull/30068#pullrequestreview-2047568227)
Isn't this same as this PR?
https://github.com/bitcoin/bitcoin/pull/29973

💬 hebasto commented on pull request "build, test, doc: Temporarily remove Android-related stuff":
(https://github.com/bitcoin/bitcoin/pull/30049#issuecomment-2102320655)
> > I guess the idea is that there's basically nothing worth salvaging from our current Android build procedure?
>
> Not sure about that. i think the idea is that the current stuff is untestable in practice, which meant the CMake transition is blocked on it. E.g. porting it as-is wouldn't result in anything usable nor testable.

Exactly.

> i've already cautioned not to go to wild with this and throw away android compatibility in the code, but removing the user facing build system support
...

✅ fanquake closed a pull request: "test: assert can't activate snapshot based chainstate more than once"
(https://github.com/bitcoin/bitcoin/pull/30068)

💬 laanwj commented on pull request "Add option dbfilesize to control LevelDB target ("max") file size":
(https://github.com/bitcoin/bitcoin/pull/30059#issuecomment-2102331899)
i'm broadly on board with making this an option (seperately from changing the default in #30039). Assuming there are legit reasons for varying this based on the kind of server, operating system, amount of RAM, kind of disk, etc, and there in't a single sweet spot.

🤔 Sjors reviewed a pull request: "net: Replace libnatpmp with built-in PCP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2047432934)
Tested on Ubuntu 24.04 I'm getting in bound via IPv4 and IPv6.

When a GUI user previously had NAT-PMP selected there would be a `"natpmp": true` field in `settings.json`.

We need to rename this to `"tcp"` in order for the box to remain checked.

Maybe @ryanofsky has an idea how to do this elegantly?

A little hack that works is to put the following at the end of `ReadSettings()` in `settings.cpp`:

```cpp
// Migrate settings:
if (values.contains("natpmp")) {
auto
...

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1595145827)
73037f27fc21765414c298b171dfdeee130c549b: maybe add: "PCP is the successor to NAT-PMP.", in case someone who didn't read the release notes is confused why that option disappeared.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1595141704)
73037f27fc21765414c298b171dfdeee130c549b: needs to be `true`, but you can actually drop this line.

👍 rkrux approved a pull request: "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29939#pullrequestreview-2047614749)
tACK [dd8fa86](https://github.com/bitcoin/bitcoin/pull/29939/commits/dd8fa861939d5b8bdd844ad7cab015d08533a91a)

Make successful, all functional tests pass.

Thanks for splitting the PR into multiple commits, made reviewing easy. I've asked couple questions for my understanding.

💬 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