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

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

Thanks for the links. I don't think I realized this change was made in the other PR, and I think it is not a good change.

I don't see a reason somebody shouldn't be able to run `RANDOM_CTX_SEED=12345 ./test_bitcoin` conveniently from a command line or script without padding the seed with 59 0's. It seems like this makes it pointlessly difficult to specify a random seed and I don't get the motivation behind it.

If w
...

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

> An alternative would be to, as @maflcko initially [suggested](https://github.com/bitcoin/bitcoin/pull/30569/#discussion_r1700712603), just have this be a parameter of `base_blob::FromHex()`, but that feels less elegant to me, still. What do you think?

Yes I think that suggestion would be a lot better! It's hard to imagine this function being used for anything other than as a preprocessor to `FromHex`. And I think it
...

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714528877)
> > The number is not just rejected when it is too long but also if it has whitespace prefixes or suffixes or trailing non-hex characters. These would have been ignored previously.
>
> Is that so? It seems to me that `IsHexNumber()` would have returned false for all of those cases, which is why [802374b](https://github.com/bitcoin/bitcoin/commit/802374b4355bd1dec7a88bba6287c55f935699fe) is a refactor commit?

Oh, sorry I forgot about the previous call. So this commit message is perfectly ac
...

💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1714681251)
> Thanks for the links. I don't think I realized this change was made in the other PR, and I think it is not a good change.
>
> I don't see a reason somebody shouldn't be able to run `RANDOM_CTX_SEED=12345 ./test_bitcoin` conveniently from a command line or script without padding the seed with 59 0's. It seems like this makes it pointlessly difficult to specify a random seed and I don't get the motivation behind it.

No strong opinion, but my thinking was that the seed would at most be copy
...

💬 maflcko commented on pull request "test: update satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2285377167)
review ACK ec317bc44b0cb089419d809b5fef38ecb9423051

💬 maflcko commented on pull request "doc: add missing "testnet4" network string in RPC/init help texts":
(https://github.com/bitcoin/bitcoin/pull/30642#discussion_r1714711575)
Would it make sense to have a `#define LIST_CHAIN_NAMES "main, test, testnet4, signet, regtest"` in `src/chainparamsbase.h`? Otherwise, the lines of code will have to be touched again when testnet3 is removed.

```suggestion
{RPCResult::Type::STR, "chain", "current network name (" LIST_CHAIN_NAMES ")"},
```