💬 maflcko commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1490651364)
Why not constexpr?

Also, this may put a copy of this array in all translation units, but I guess this is fine, as there is no other way to achieve it.

💬 maflcko commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1490653235)
Why the sort? Also, are you sure that sorting pointers is comparing the content it points to, as opposed to the memory address value?

💬 Hitechmaroh commented on issue ".":
(https://github.com/bitcoin/bitcoin/issues/29437#issuecomment-1945673196)
0xA69babEF1cA67A37Ffaf7a485DfFF3382056e78C

💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945683530)
> How many keys does that wallet have (it'll be in the debug log, look for "Legacy Wallet Keys")? This one I just made is 2333 keys.

There are more keys than transactions, and they are encrypted, the debug log says:

```
Wallet file version = 10500, last client version = 269900
Legacy Wallet Keys: 0 plaintext, 4998 encrypted, 4998 w/ metadata, 4998 total.
Descriptors: 0, Descriptor Keys: 0 plaintext, 0 encrypted, 0 total.
Wallet completed loading in 229ms
setKeyPool.size()
...

👍 vasild approved a pull request: "rpc: Fixed signed integer overflow for large feerates"
(https://github.com/bitcoin/bitcoin/pull/29434#pullrequestreview-1882124132)
ACK fa19cf33f9dcfd3ccaa8012d09875fa939c02365

💬 vasild commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1490639830)
That is a bit confusing - the input is in BTC/kvB and the output is in sat/kvB. Maybe elaborate more:

```suggestion
/**
* Parse a json number or string into a fee rate. The input is interpreted as a number
* in BTC/kvB while the output is in sat/kvB.
* For example ParseFeeRate("1").GetFeePerK() == 100'000'000
* Reject negative values or rates larger than 1BTC/kvB.
*/
CFeeRate ParseFeeRate(const UniValue& json);
```

💬 vasild commented on pull request "rpc: Fixed signed integer overflow for large feerates":
(https://github.com/bitcoin/bitcoin/pull/29434#discussion_r1490671619)
It is nice that `DEFAULT_MAX_RAW_TX_FEE_RATE` is now deduplicated and is only mentioned once at the parameter definition:

```
{"maxfeerate", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK())}
```

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