💬 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 ")"},
```

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

💬 maflcko commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1714713004)
> - #include <cassert> // lines 9-9

This is wrong. It is used.

Removed the other.

💬 maflcko commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2285409839)
> Sonar

They are mostly just a clang-tidy wrapper, so I wonder if there is a clang-tidy warning that can be used here instead.

💬 paplorinc commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2285442194)
> I wonder if there is a clang-tidy warning

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

> Don't we also need to add move constructor to Coin?

It seems to me that the `Coin` class already has an explicit move constructor that moves the `CTxOut` member - and the rest are trivially movable (i.e. bit-copy of `unsigned int` and `uint32_t`).
```C++
Coin(CTxOut&& outIn, int nHeightIn, bool fCoinBaseIn) : out(std::move(outIn)),
...

👋 paplorinc's pull request is ready for review: "coins: Add move operations to CCoinsCacheEntry"
(https://github.com/bitcoin/bitcoin/pull/30643)