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

tiny nit:
looks redundant to call `add_wallet_options(legacy=False)` and then call `skip_if_no_sqlite()`.
If the wallet is enabled and `sqlite` is not supported, then the only option is a legacy wallet, which is blocked by the `add_wallet_options` call.

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450417390)
Unless you specify a clean chain (which this test does not), the test will have an existing matured. Which means, you could modify this for

```suggestion
tester_wallet = MiniWallet(tester_node)
self.generate(tester_wallet, 1)
```

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450361941)
tiny nit: this could be `sync_mempools` instead.

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450366154)
In https://github.com/bitcoin/bitcoin/commit/c39e82926a3740642f8f1bb72c6b10b67a6dc724:

same as above, the `descriptors` flag isn't needed. Also, if you set `disable_private_keys=True`, there is no need to set `blank=True`.

💬 ryanofsky commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#discussion_r1450456098)
I think in general it is safe to edit the file, and main thing worth pointing out is when changes might be ignored or overwritten. It's possible adding more information could be useful, but I don't think there's anything clearly missing. The commit message also seems ok and wouldn't really be an appropriate place to caution users since they are unlikely to see it (in the file itself or in documentation would be better).

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450457328)
I initially left the same comment about `disable_private_keys` but think it is helpful to contrast the difference with further down the test where we set `disable_private_keys=True`. Likewise, I think keeping `descriptors=True` is helpful self-documentation given the contrast with the test in `wallet_import_rescan.py`. No strong opinion on either, though.

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

💬 dergoegge commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#discussion_r1450462010)
nit:

```suggestion
const int percentage_done{(int)(100.0 * txns_tried / total_txns_to_load)};
```

💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1450469043)
No opinion on this thread, but I wanted to mention that it is not always possible to easily define a logging category right now. For example, log statements in the flatfile module may fit in the "blockstorage" or "indexes" category, depending on context. Thus, either the log category would have to be passed down the call stack before logging, or the log-string would have to be passed up the call stack before logging.

Also, some modules, such as `args`, have only a few log statements, so creat
...

💬 glozow commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#discussion_r1450470423)
I thought we didn't like c-style casts?

💬 maflcko commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#discussion_r1450471581)
nitting the nit:

```diff
-const int percentage_done{(int)(100.0 * txns_tried / total_txns_to_load)};
+const int percentage_done(100.0 * txns_tried / total_txns_to_load);

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1889248825)
@dergoegge @glozow I just updated the PR description.

cc: @sr-gi (asked same in IRC)

💬 glozow commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#discussion_r1450477325)
@maflcko knows best, changed.

👍 furszy approved a pull request: "wallet: Fix migration of blank wallets"
(https://github.com/bitcoin/bitcoin/pull/28976#pullrequestreview-1818312971)
reACK c11c404. CI failure is unrelated.

💬 glozow commented on issue "ci: failure in `wallet_assumeutxo.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/29234#issuecomment-1889316636)
https://github.com/bitcoin/bitcoin/actions/runs/7503027363/job/20426899651

💬 sipa commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1450511206)
Unsure how relevant that is, but I've changed it to drop the 3rd argument whenever it matches the node `-v2transport` setting (I think).

💬 sipa commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1450511652)
Nice, done.

📝 GoodDaisy opened a pull request: "Fix typos"
(https://github.com/bitcoin-core/gui/pull/787)
fix: typo in doc/Doxyfile.in
fix: typo in test/functional/wallet_backup.py

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1450515730)
> Why is the approach to add "in" and "out" instead of just having specified permissions apply to both? Are there instances where you want to specify different permissions for the same address depending on the connection direction?

For the same address especifically, idk. But since we can use `-whitelist` to give permissions for "all" nodes (e.g. using `0.0.0.0`), it could be worth to specify the direction.

💬 glozow commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450509572)
Seems pretty common to specify both. They are technically different; one says no legacy and the other says need descriptors. I know that's effectively the same thing and we don't have 3 types, but I think it's harmless to keep this.