💬 furszy commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1715277603)
> What compiler? I'm not seeing this error, and it seems neither did CI.

clang 14.0.3 (clang-1403.0.22.14.1).

💬 hodlinator commented on pull request "refactor: Add consteval ArrayFromBytes()":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715287560)
Went with `ArrayFromBytes` for all in the latest tip d6d7a0b5221935518d2797aec7abc5c9632cbf68.

Avoided adding a public `CScript::operator<<(std::span)` and went for a private method + public `std::array` overload.

Have a variant with consteval validation in VecFromHex-branch, but didn't feel it was urgent to use in any of the cases, so currently dropped from this PR:
https://github.com/hodlinator/bitcoin/blob/2024-07/uint256S_consteval_VecFromHex/src/util/strencodings.h#L95-L110

👋 hodlinator's pull request is ready for review: "refactor: Add consteval ArrayFromBytes()"
(https://github.com/bitcoin/bitcoin/pull/30377)

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715292834)
I expect single path to still be most used and I thought that returning an array for that would be confusing.

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2286256207)
@pythcoiner @mjdietzx Can you please re-review?

💬 glozow commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1715303747)
Makes sense

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2286346598)
`2db437fbd1...df3a22f87a`: pet clang-tidy and combine the last two commits (that implement the `INV+GETDATA`) because otherwise the test-each-commit CI task fails.

👍 BrandonOdiwuor approved a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235675454)
ACK 701530045553f2b9671a3fffea301bf4dc954514

👍 vasild approved a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2235704287)
ACK 9bed303b9cc28e13b12af067b34e6058cbccf639

👍 ryanofsky approved a pull request: "logging: use bitset for categories"
(https://github.com/bitcoin/bitcoin/pull/26697#pullrequestreview-2235717036)
Code review ACK d2067124aae098980de5c4a810753486491faea7. Since last review, just renamed array member to m_bits, switched from 32 to 64 bit ints and added comment about any and none methods being not atomic.

👍 danielabrozzoni approved a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642#pullrequestreview-2235717662)
ACK 701530045553f2b9671a3fffea301bf4dc954514

🤔 furszy reviewed a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2235734674)
Worked further on your [8d0b5c1](https://github.com/bitcoin-core/gui/pull/824/commits/8d0b5c1526c7abfe8048db15dd3df8c898a2fd35) to eliminate the need for the extra 'Yes, encrypted' button in the migration dialog, which I found quite unfriendly. Could replace it directly for https://github.com/furszy/bitcoin-core/commit/e8d147eeaacac7d434aad8dbbc5022aadf7d112a. It detects the wallet encryption status automatically regardless of its loaded/unloaded state.

Other than this, code is looking good.

🤔 Johnnygatvol reviewed a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2235739639)
I need to know where my money is going

👍 ryanofsky approved a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2235733223)
Code review ACK 9bed303b9cc28e13b12af067b34e6058cbccf639. I do think #26697 is a nicer alternative to this PR because it makes code more safe and readable. But this approach is also fine.

💬 ryanofsky commented on pull request "log: expand BCLog::LogFlags (categories) to 64 bits":
(https://github.com/bitcoin/bitcoin/pull/26619#discussion_r1715401167)
In commit "log: expand BCLog::LogFlags (categories) to 64 bits" (9bed303b9cc28e13b12af067b34e6058cbccf639)

Instead of hardcoding uint64_t throughout this PR, it might make sense to add `using CategoryMask = uint64_t;` and use `CategoryMask` as a type name describing purpose of the type rather than how many bits it presently contains.

👍 ryanofsky approved a pull request: "fix: increase consistency of rpcauth parsing"
(https://github.com/bitcoin/bitcoin/pull/30401#pullrequestreview-2235751865)
Code review ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586. Since last review commit message was just tweaked to clarify previous behavior.

I hope this will get reviewed and merged. It is now just a one line fix!

💬 andrewtoth commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2286433209)
> > I wonder if there is a clang-tidy warning that can be used here instead.

> I have installed Sonar locally, reproduced the warning first, it doesn't appear anymore afer the change.

I think what is being suggested here is to configure clang-tidy to produce this warning in our own CI instead of Sonar, which can then be shown to be fixed by this fix. I think that would be a more valuable change.

> It seems to me that the Coin class already has an explicit move constructor that moves the
...

💬 sipa commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2286446542)
> How can we be sure that this happens?

A class has an implicitly-declared move constructor whenever:
* There are no user-declared copy constructors.
* There are no user-declared copy assignment operators.
* There are no user-declared move constructors.
* There is no user-declared destructor

(from https://en.cppreference.com/w/cpp/language/move_constructor)

Personally, I find these rules too inscrutable to judge from a quick glance at a class, so if I want a class that is guaranteed to have a
...

⚠️ theStack opened an issue: "doc: deduplicate list of chain/network strings in RPC/parameter help texts"
(https://github.com/bitcoin/bitcoin/issues/30645)
### Motivation

Many command line parameter and RPC help texts currently contain the list of chain/network names hardcoded ("main, test, testnet4, signet, regtest"), which is error-prone as it can easily happen to miss an instance if the list ever changes again.

### Possible solution

For better maintainability, the list should be deduplicated, either with a constant string or a function creating that string like `ListChainTypes`. See the suggestions https://github.com/bitcoin/bitcoin/pull/306
...

💬 theStack commented on pull request "doc: add missing "testnet4" network string in RPC/init help texts":
(https://github.com/bitcoin/bitcoin/pull/30642#issuecomment-2286468371)
Thanks for the reviews! Deduplicating the chain list seems to be a good idea, I opened a good first issue for that: https://github.com/bitcoin/bitcoin/issues/30645