💬 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_r1715130340)
nit "second elements". Also could be helpful that apart from this special case, all multipath are non-internal

💬 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_r1715147527)
(I don't know how frequently we expect each type of usage to be.) Did you consider returning single derivation descriptors in both result arrays, so that the original result can eventually be removed (after deprecation, everyone adds a [0] to their scripts when using single)?

💬 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_r1715114909)
Could also add a comment here on the `j==1`, "first is receiving, second is change" or something

💬 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_r1715151078)
Can `Assume(!multipath_values.empty())` here

💬 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_r1715150336)
Could add some comments for future wallet reviewers, maybe mentioning that they should be consistent with each other (multipath_segment_index.has_value() == !multipath_values.empty() == !seen_multipath.empty())

💬 paplorinc commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#discussion_r1715243406)
Done, thanks!

These files are also edited during the CMake migration, we can finalize the details once that's merged.

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