💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450510684)
nice, taken

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450514108)
Keeping as is because I prefer the peace of mind of knowing `tester_wallet` has a mature coinbase.

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450512137)
dropped the extra args

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450511111)
dropped the args

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450510108)
done

💬 maflcko commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1450520489)
```suggestion
bool node_v2transport = connman.GetLocalServices() & NODE_P2P_V2;
```

nit, if you re-touch

💬 glozow commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1889361221)
CI failure is unrelated, #29234

💬 maflcko commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#discussion_r1450528234)
```suggestion
n0.unloadwallet('w') # Required, because on Windows only one process can access a file, when it is written
```

nit: Seems better to run the same test on all platforms, than to run different branches on different platforms, and thus make it harder to debug test failures?

💬 maflcko commented on issue "ci: failure in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1889369430)
Could this be a bug in the wallet? Why would it call `remove_all` on the wallet file of a completely different node?

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450532808)
> Keeping as is because I prefer the peace of mind of knowing `tester_wallet` has a mature coinbase.

You shouldn't be concerned about that. Thats how the test framework behaves when `setup_clean_chain` is not set. See [code](https://github.com/bitcoin/bitcoin/blob/3ba8de1b704d590fa4e1975620bd21d830d11666/test/functional/test_framework/test_framework.py#L381). If the wallet wouldn't be having mature coins, the test would be always failing due a lack of funds.

👍 stickies-v approved a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1818404985)
re-ACK 9db0d42509e0e223b45238fd8087fc98ef32c091

💬 sipa commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1450536603)
Done. I needed to re-touch anyway because apparently `nool` is not a C++ type.

👍 ismaelsadeeq approved a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1818400509)
ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c

It will be nice to also log how many transactions were dumped not just unbroadcast set
q
```diff
diff --git a/src/kernel/mempool_persist.cpp b/src/kernel/mempool_persist.cpp
index cae4b04bf65..404f23b21a9 100644
--- a/src/kernel/mempool_persist.cpp
+++ b/src/kernel/mempool_persist.cpp
@@ -194,7 +194,6 @@ bool DumpMempool(const CTxMemPool& pool, const fs::path& dump_path, FopenFn mock

file << mapDeltas;

- LogPrintf("Wr
...

💬 ismaelsadeeq commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#discussion_r1450533993)
nit
```suggestion
LogInfo("Progress loading mempool transactions from disk: %d%% completed (tried %u txs, %u txs remaining)\n",
```

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450547499)
Deleted this line altogether, there's no reason to generate 1 either.

💬 sipa commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1889415495)
It appears to me that whitelisting of outbound connections really only applies to manual ones: if you have reason to specifically trust another peer or otherwise give them elevated privileges, you probably want to also try actually connecting to them.

If so, it seems more natural to me to specify outbound net permissions wherever that manual connection is instructed (so inside `-addnode`, -connect=`, or as argument to the `addnode` RPC?

👍 dergoegge approved a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1818441827)
Code review ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1889422328)
> See commit https://github.com/bitcoin/bitcoin/commit/f8273512a50d9ed03bc48803634fb8780b9023c7 (feel free to pull it).

Sorry I hadn't seen this. Thanks, taken with a couple of edits.

📝 fjahr opened a pull request: "doc: Add missing backtick in developer notes logging section"
(https://github.com/bitcoin/bitcoin/pull/29241)
Newly added logging section is missing a single backtick.

💬 maflcko commented on pull request "test: unbreak: exclude windows from wallet_assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/29238#issuecomment-1889432589)
```
test/functional/wallet_assumeutxo.py:14:1: F401 'platform' imported but unused