💬 vasild commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490770478)
This should work as well (untested):

```cpp
(void)addrman.Select(/*new_only=*/false, {NET_I2P});
```

Same in `src/test/addrman_tests.cpp`.

💬 vasild commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490757436)
This should be `continue;`? If addrman has 0 addresses from some of the requested networks, it can still return an address from another requested network.

```suggestion
if (it == m_network_counts.end()) {
continue;
}
```

💬 vasild commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490780583)
This could be shorter (untested):

```cpp
std::tie(addr, addr_last_try) = preferred_net.has_value()
? addrman.Select(false, {*preferred_net})
: addrman.Select(false, g_reachable_nets.All());
```

💬 vasild commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490807334)
If you extend the `ReachableNets` class like this:

```diff
--- i/src/netbase.h
+++ w/src/netbase.h
@@ -102,12 +102,18 @@ public:
[[nodiscard]] bool Contains(const CNetAddr& addr) const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
AssertLockNotHeld(m_mutex);
return Contains(addr.GetNetwork());
}

+ [[nodiscard]] std::unordered_set<Network> All() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
+ {
+ LOCK(m_mutex);
+ return m_reachable;
+
...

💬 vasild commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490794321)
This could end up passing an empty container (`vector` or `unordered_set` if you decide to change it). Maybe this:

```cpp
if (nets.empty()) {
(void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool());
} else {
(void)const_addr_man.Select(fuzzed_data_provider.ConsumeBool(), nets);
}

🤔 hebasto reviewed a pull request: "Prevent weird focus rect on inital sync"
(https://github.com/bitcoin-core/gui/pull/795#pullrequestreview-1882465496)
> ... keeping the focus on the "Hide" button while the ModalOverlay is visible.

That looks like a better commit message / PR description.

💬 hebasto commented on pull request "Prevent weird focus rect on inital sync":
(https://github.com/bitcoin-core/gui/pull/795#discussion_r1490826069)
The same.

💬 hebasto commented on pull request "Prevent weird focus rect on inital sync":
(https://github.com/bitcoin-core/gui/pull/795#discussion_r1490825810)
Please [use](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) braces.

💬 maflcko commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1945874746)
I re-checked b20d882fd53c21098eb7858b2dbca84eafd2b344 and re-confirmed `time loadwallet` (https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1943469180)

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1490849107)
@maflcko shouldn't TSan on CI output something useful about why it crashed? I currently only says "error 2": https://cirrus-ci.com/task/5124733717446656?logs=ci#L3531

When running this locally on Ubuntu with clang 16.0.6 I get a `WARNING: ThreadSanitizer: data race` and significantly more details (still a bit cryptic, but hopefully enough to figure out what's happening).

💬 hebasto commented on issue "Bitcoin Core GUI getrawtransaction implementation":
(https://github.com/bitcoin-core/gui/issues/645#issuecomment-1945884609)
@zndtoshi @BrandonOdiwuor

Are there any compelling reasons not using RPC in the "Console" window?

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1490855751)
@vasild any thoughts on how to make mock `Sock`s that can be used to play messages in two directions?

💬 vasild commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490857252)
With this PR all production code passes both arguments. No need anymore for defaults. This compiles:

```diff
- std::pair<CAddress, NodeSeconds> Select(bool new_only = false, std::optional<std::vector<Network>> networks = std::nullopt) const;
+ std::pair<CAddress, NodeSeconds> Select(bool new_only, std::vector<Network> networks) const;
```

Some tests use `Select()` and I adjusted them to do `Select(false, {})` just to check that the compilation will succeed. They can be adjusted to
...

⚠️ maflcko opened an issue: "wallet: pre-HD HDD migratewallet "
(https://github.com/bitcoin/bitcoin/issues/29438)
Opening a brainstorming issue, if someone wants to test `migratewallet` on a pre-HD wallet on a slow storage device (HDD).

Steps to reproduce:

* Create a wallet in a folder. For example: `./releases/bitcoin-0.11.2/bin/bitcoin-qt -datadir=/tmp/rgw -regtest`. Then stop bitcoin-qt.
* Copy the wallet file to a folder residing on the intended storage device and call `migratewallet` with the latest version of Bitcoin Core.

The second step can be done by a functional test (I modified an exist
...

💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945902492)
Done in https://github.com/bitcoin/bitcoin/issues/29438 . Maybe move the discussion there, so that this can move forward and be merged?

💬 jadijadi commented on pull request "Prevent weird focus rect on inital sync":
(https://github.com/bitcoin-core/gui/pull/795#issuecomment-1945903374)
> Why does the issue happen only on certain platforms?

Not sure about other platforms. I had a mac and a KDE machine and happened on both of them. Have not checked on other platforms.

💬 Sjors commented on issue "RFC: Migration from NSUserNotificationCenter to UNUserNotificationCenter on macOS":
(https://github.com/bitcoin-core/gui/issues/112#issuecomment-1945907679)
I think (2) is fine. We already sign the release build, and anyone who can compile from source can run the extra `codesign` command (maybe as part of `make deploy`?).

If (1) just means that they have to use `make deploy` in order to get notifications working, that seems fine.

🤔 hebasto reviewed a pull request: "RFC: build: remove confusing and inconsistent disable-asm option"
(https://github.com/bitcoin/bitcoin/pull/29407#pullrequestreview-1882535266)
Concept ACK.

💬 maflcko commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1490880303)
Ah, I guess the unit tests don't capture the tsan output?

💬 maflcko commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1490884343)
But they should. At least back when I tested https://github.com/bitcoin/bitcoin/pull/27667