💬 moonsettler commented on pull request "Reenable OP_CAT":
(https://github.com/bitcoin/bitcoin/pull/29247#issuecomment-1945807463)
Some of us have been playing with the idea of neutered CAT: instead of the MAX_SCRIPT_ELEMENT_SIZE (520 bytes) the maximum output size would be 80 bytes.

**Rationale:**
It explicitly disables open ended second order effects by removing the ability to assemble a SIGHASH or CTV template on the stack for more detailed introspection than intended. If such introspection behavior is desired in the future it can be explicitly enabled by specialized and more efficient opcodes.

Combined with CSFS
...

💬 maflcko commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1490795645)
I think the trace level was always disabled, unless explicitly enabled? But I haven't double checked this.

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

💬 vasild commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1490766626)
This code is correct, but is doing linear search in the list of networks. For values of up to 6 that is irrelevant, but if it is changed from `vector` to `unordered_set` then lookup is also shorter to write, in addition to being `O(1)`:

```suggestion
if (Assume(it != mapInfo.end()) && networks->contains(it->second.GetNetwork())) break;
```

🤔 vasild reviewed a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436#pullrequestreview-1882354205)
Approach ACK 2096aeabec288bd2852593f0c965a2a9d22ad0c8

Some non-blockers below (feel free to ignore them) plus one `return` vs `continue` that probably needs to be addressed.

💬 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?