💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945528839)
> I'm any case, I didn't think the performance issues should be a blocker for this PR. It's clear that migration works, it may just take a while.
>
> Missing this release _will_ push back the timeline another release cycle, if not more.

Yes, I agree. Sorry for not being clear.

Since it appears that at least one other person could reproduce a longer migrate time (30 minutes), why not mention that the user should take care to disable the client rpc timeout? Something along the lines of:
...

💬 maflcko commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1945535057)
Ok, I didn't see that you already did that. :sweat_smile:

lgtm ACK f1684bb88a878eb3f54e945db39ea69b14256eef

⚠️ Hitechmaroh opened an issue: "35c1089b1cf59c00a526ebd8b233494d500c71b75208bcd1bc469b665a36214e"
(https://github.com/bitcoin/bitcoin/issues/29437)
0x6Bb39276bA115B89F97433BF12307CcBA2d575Dc

✅ hebasto closed an issue: "35c1089b1cf59c00a526ebd8b233494d500c71b75208bcd1bc469b665a36214e"
(https://github.com/bitcoin/bitcoin/issues/29437)

💬 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_r1490642807)
```suggestion
for (const auto& msg : g_all_net_message_types) {
```

nit: Add missing `{` while touching?

💬 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