💬 furszy commented on pull request "wallet: fix blank legacy detection":
(https://github.com/bitcoin/bitcoin/pull/30621#discussion_r1714380532)
done

💬 andrewtoth commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2284929509)
Don't we also need to add move constructor to `Coin`? Its prevector is the only thing that really benefits from being moved I believe.

🤔 stickies-v reviewed a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2232822586)
Approach ACK

💬 stickies-v commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1713639323)
nit: no more need for `#include <algorithm>`

<details>
<summary>iwyu</summary>

```
span.h should add these lines:
#include <utility> // for declval, forward

span.h should remove these lines:
- #include <algorithm> // lines 8-8
- #include <cassert> // lines 9-9

The full include-list for span.h:
#include <cstddef> // for size_t, byte, nullptr_t
#include <span> // for span
#include <type_traits> // for remove_pointer, is_convertible, enable_if, enable_if_t,
...

👍 AngusP approved a pull request: "test: update satoshi_round function"
(https://github.com/bitcoin/bitcoin/pull/29566#pullrequestreview-2234072206)
ACK ec317bc44b0cb089419d809b5fef38ecb9423051

🤔 pablomartin4btc reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2234121687)
tACK 67b1e236334f38ec4e4d2251dbdfb790f20ed88b

Built with `GUI` on macOS 14.4 following the instructions in `/doc/build-osx.md` (part of this PR) and run `./build/qt/bitcoin-qt` successfully.

A few observations:

- Regarding the configuration: in the documentation says:
"_If `berkeley-db@4` is installed, then legacy wallet support will be built_."
But I have to use `-DWITH_BDB=ON` in order to get that functionality.
- After I compile with `cmake` I get this warning:
`ld: warning
...

🤔 tdb3 reviewed a pull request: "wallet: fix blank legacy detection"
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2234159075)
Approach ACK

Nice simplifications.

Retested (https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2231909317) with same result (migration allowed).

Saw some `may be used uninitialized` warnings when building, e.g.
```
inlined from ‘bool wallet::CWallet::IsSpentKey(const CScript&) const’ at wallet/wallet.cpp:1046:37:
./prevector.h:175:37: warning: ‘*(const prevector<28, unsigned char, unsigned int, int>*)((char*)&<unnamed> + offsetof(const std::CTxDestination, std::varian
...

👍 ryanofsky approved a pull request: "logging: use bitset for categories"
(https://github.com/bitcoin/bitcoin/pull/26697#pullrequestreview-2234186346)
Code review ACK d8c637a0ad704c30dc128d0c66215d5ac220f85d

I wouldn't have a problem just switching the code to use uint64_t but I think this approach is a nicer and more type safe. I like that this gets rid of all the bitwise operations and (1 << 28) cruft and stops hardcoding uint32_t various places. The atomic bitset class seems pretty simple to me and has a nice interface.

💬 ryanofsky commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#discussion_r1714473985)
This is a good catch and I think it's worth commenting on, but I don't think it is a bug. Would suggest adding a comment like: "Checks if any bits are set. Note: Bits are checked independently, so result reflects the state of all bits, but not at a single instant in time."

💬 ryanofsky commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#discussion_r1714475980)
In commit "logging: use bitset class for categories" (d8c637a0ad704c30dc128d0c66215d5ac220f85d)

Would be nice to rename `bits` member to `m_bits` for clarity I think

👍 ryanofsky approved a pull request: "fix: increase consistency of rpcauth parsing"
(https://github.com/bitcoin/bitcoin/pull/30401#pullrequestreview-2234212279)
Code review ACK f4b0b69a3e7df58430cb887028b13265834d8c7e.

This PR is now just a one line fix accompanied by various tests.

💬 ryanofsky commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#discussion_r1714491084)
In commit "fix: increase consistency of rpcauth parsing" (f4b0b69a3e7df58430cb887028b13265834d8c7e)

I think it would be nice if commit message said explicitly that previous behavior was to sometimes ignore empty `-rpcauth=` settings, and other times treat them as errors, and now they are consistently treated as errors.

Could also mention the details, but not important

> Previous behavior was nonsensical:
>
> - If an empty -rpcauth="" argument was specified as the last command line ar
...

💬 LarryRuane commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#issuecomment-2285108664)
@ryanofsky - I took your suggestions (`bits` to `m_bits` and added the comment), and I noticed something else: By defining `LogCategoryMask` as comprising `uint64_t` instead of `uint32_t` base elements, the [non-atomic timing window problem](https://github.com/bitcoin/bitcoin/pull/26697#discussion_r1713192718) won't happen until we exceed 64 logging categories, because the loop in `any()` will iterate only once. That loop doesn't look at individual bits, it looks at one entire base element (now
...

💬 tdb3 commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#discussion_r1714507567)
Thanks. Amended the commit message to mention that the previous rpcauth behavior was inconsistent. Updated the PR description to list detailed behavior.

🤔 tdb3 reviewed a pull request: "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate"
(https://github.com/bitcoin/bitcoin/pull/30636#pullrequestreview-2234279782)
Concept ACK

Great test additions.
Light code review and manual test. I concur with the nits from @fjahr.

💬 achow101 commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2285198261)
ACK 1610643c8b37a9f674b236cfa79abf8f8aaf1410

If a few more people can review this in the next day or so, I believe we can have this in for 28.0.

🤔 ryanofsky reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2234255969)
Code review 855784d3a0026414159acc42fceeb271f8a28133 again (unchanged since last time). I think motivation here is good and this PR could be merged if my concerns are not shared, but IMO this change is a net negative in current state without some compatibility improvements.

re: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712563888

> Because my understanding is that `0x` is used for hex numbers, and a block hash is not a number.

I do think block hashes are numbers, and it'
...

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714518388)
re: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712438144

> With that said, would you still prefer not having a separate function, or e.g. templating it (prematurely) and moving it into `test/util/`?

Thanks for pointing out that it's a pretty generic function. Either way seems fine, since it does seem like something that could be reused.

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714521300)
re: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712441877

> "Desired" to me sounds like the result string may not actually be of the requested size, when it is actually guaranteed. I'm not sure that would be an improvement?

Makes sense, I think "Requested" would be better than "Desired" in that case, since the function can still fail and return nullopt.

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714546457)
re: https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1712432448

Thanks for the response, but I think there are two things that could be improved here:

- I think it would be better to call TrySanitizeHexNumber so this continues to accept block hashes with optional 0x prefixes and without leading 0's. This behavior worked previously and seems safe, so I don't see a reason to break it. If we do want to break, it would be nice to state some reasoning for doing that in the commit messa
...