🤔 ryanofsky reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2230780077)
Code review 855784d3a0026414159acc42fceeb271f8a28133, mild NACK.

I like the new test, and ideas behind this change, but I think some parts of this are not well documented, and are breaking things that work without a clear reason.

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1711982050)
In commit "test: Only accept a fully valid RANDOM_CTX_SEED" (855784d3a0026414159acc42fceeb271f8a28133)

I don't understand the motivation for being so strict here. It seems better to call TrySanitizeHexNumber and keep accepting small numbers. The seed just needs to be any number, and I don't see why numbers that have 64 digits are better than other numbers, or why somebody manually setting the environment variable manually should need to pad the number and make it has 64 digits.

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1711957751)
In commit "util: refactor: change IsHexNumber to TrySanitizeHexNumber" (802374b4355bd1dec7a88bba6287c55f935699fe)

This is a somewhat odd function doing a combination of arbitrary things:

- Trimming 0x prefix
- Returning an error the string is too long
- Padding with 0 if the string is too short

And not doing other things that you might expect a sanitize function to do like trimming whitespace or normalizing the case.

I think instead of saying this "sanitizes" the string and pretend
...

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1711911714)
In commit "test: unittest chainstatemanager_args" (1c909b2ccfaee902637b28f890db8dbe168ea926)

I don't think this should be a standalone function when it is only called one place. IMO it would be better to drop it and move the code into the set_opts lambda, which is the only place using it. Getting rid of this function would also avoid unnecessary complexity of converting a char* array to a std::string array and back to a char* array again, and it could avoid introducing an inconsistently named
...

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1711973562)
In commit "node: use uint256::FromHex for -minimumchainwork parsing" (3c794b537fe8fe000d3545704afc17d1d63bdb5f)

This is also now going to reject assumevalid numbers that begin with `0x` which isn't mentioned in the commit message, and also reject numbers with whitespace prefixes and suffixes and trailing non-hex characters.

I think it would be better to just use TrySanitizeHexNumber here to avoid breaking things that work without a clear reason. I can see an argument for rejecting extra wh
...

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1711985406)
In commit "util: refactor: change IsHexNumber to TrySanitizeHexNumber" (802374b4355bd1dec7a88bba6287c55f935699fe)

> std::nullopt returned if input is longer than result_size

Implementation seems to return null when string is empty, not just when string is too long.

💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1711992941)
In commit "node: use uint256::FromHex for -minimumchainwork parsing" (3c794b537fe8fe000d3545704afc17d1d63bdb5f)

> This makes parsing more strict, by explicitly returning an error
> when the input is longer than 64 hex characters.

Commit message is incomplete. 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.

💬 achow101 commented on pull request "wallet: Fix listwalletdir listing of migrated default wallets and generated backup files":
(https://github.com/bitcoin/bitcoin/pull/30265#discussion_r1712075011)
Removed

👍 hodlinator approved a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2230973734)
Untested re-ACK fa69fba751079ee6042daceb88bc8f8ad3f3de96

`git range-diff master fabcd0c fa69fba` reveals pure whitespace change.

💬 achow101 commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2278659999)
ACK 00618e8745192d209c23e3ae873c077e58168957

💬 sipa commented on pull request "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin":
(https://github.com/bitcoin/bitcoin/pull/30326#issuecomment-2278660421)
utACK 204ca67bba263018374fe86d7a6867362d09536f

✅ achow101 closed an issue: "implicit-integer-sign-change in ActivateSnapshot"
(https://github.com/bitcoin/bitcoin/issues/30514)

🚀 achow101 merged a pull request: "assumeutxo: Drop block height from metadata"
(https://github.com/bitcoin/bitcoin/pull/30598)

💬 achow101 commented on pull request "contrib/signet/miner updates":
(https://github.com/bitcoin/bitcoin/pull/28417#issuecomment-2278716451)
ACK fb6d51eb25a2bb69a3ecdfc4796a88d4d1aacc65

💬 sipa commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2278726067)
It would be good to know what impact it has on IBD, as it's necessarily going to make things slower (more syncs means more disk writes, and more memory/cpu to remember UTXOs which must be marked spent on disk), for the benefit of reducing losses in case of crashes. The question is how much time & I/O it adds.

Regarding doing it for every block (during steady state), the trade-off there is different. On the pro side there is a reduction of latency spikes instead of reducing crash losses (as th
...

💬 maflcko commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2278758371)
Did you get a chance to see if you can reproduce the crash, if you put all data and swap on a single root SSD?

💬 achow101 commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2278782453)
ACK 49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb

🚀 achow101 merged a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519)

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2278793149)
> It would be good to know what impact it has on IBD, as it's necessarily going to make things slower

Note that it is only necessarily slower when running with very high dbcache settings. For default settings the cache fills up and is flushed far more often than every hour, so this will have no effect.

💬 tdb3 commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#issuecomment-2278800814)
Rebased. Addressed comments from @naiyoma and @ryanofsky.

💬 tdb3 commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#discussion_r1712258890)
Added log statements. Thanks.