💬 vasild commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1490696343)
What is the integer overflow?

This passes on master @ 7143d4388407ab3d12005e55a02d5e8f334e4dc9:

```
$ echo "c2VuZHJhd3RyYW5zYWN0aW9uXAEAAAAAHgBiu7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u7u
...

💬 maflcko commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1490718104)
Yes, this is a side-effect of the changes here, due to using `self.Arg()`.

💬 maflcko commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1490725542)
> What is the integer overflow?

You will have to correctly copy-paste the full base64 input. Dropping the second half will result in a different fuzz input, which will trigger a different code path.


To double check, if you correctly copy-paste the full input, you should get:

```
$ sha1sum /tmp/b
6ef1eadaa3a1ad54365358b1ceb677bd1632396c /tmp/b
$ sha256sum /tmp/b
fa82e44660796a5196cfb05483cae34356e9a2c55aab79d59161c887d5f23b37 /tmp/b
$ FUZZ=rpc UBSAN_OPTIONS="suppressions=$(pwd)/
...

💬 maflcko commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1490735076)
Thanks, I pushed something.

👍 hebasto approved a pull request: "doc: Update translation process guide"
(https://github.com/bitcoin/bitcoin/pull/29414#pullrequestreview-1882368115)
ACK 3b0ec06d6228d965e9cf9121c5dd300da2a930ea.

💬 Sjors commented on pull request "Move log messages: tx enqueue to mempool, allocation to blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27277#discussion_r1490774411)
Indeed this should be `LogDebug()` in order to not change behavior.

The downgrade to trace should be in a different commit. Will look into this again.

> This makes intermittent test failures harder to debug, since trace is disabled.

I'll look into what level to use. Lots of things have changed over the past few rebases.

📝 Sjors converted_to_draft a pull request: "Move log messages: tx enqueue to mempool, allocation to blockstorage"
(https://github.com/bitcoin/bitcoin/pull/27277)
When running with `-debug=validation` the log is filled with:

```
2023-03-19T12:46:01Z [validation] Enqueuing TransactionAddedToMempool: txid=37c8e1ef87d75a67fbaf44e116018fbc70d9c4ef0e27e2ae56861395101f9b8e wtxid=24e2d76012f9cde451d085ead4feab136feddae6cb55d37221a81aac6c0eaf42
2023-03-19T12:46:01Z [validation] TransactionAddedToMempool: txid=37c8e1ef87d75a67fbaf44e116018fbc70d9c4ef0e27e2ae56861395101f9b8e wtxid=24e2d76012f9cde451d085ead4feab136feddae6cb55d37221a81aac6c0eaf42
```

These l
...

🤔 BrandonOdiwuor reviewed a pull request: "Update rpcauth.py"
(https://github.com/bitcoin/bitcoin/pull/29433#pullrequestreview-1882386097)
Concept ACk

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