💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726880784)
> (In the follow-up you can also adjust the error message to explain the new max-length check.

Done.

> Will use .value() either in next force-push

Resolved by using `FromUserHex()` instead.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726879444)
> I think I would probably add base_blob::FromUserHex() function wrapping base_blob::FromHex().

Thanks for the suggestion, I've adopted this approach.

🤔 stickies-v reviewed a pull request: "node: reduce unsafe uint256S usage"
(https://github.com/bitcoin/bitcoin/pull/30569#pullrequestreview-2254228380)
Force-pushed (twice) to change the approach to address @achow101's, @ryanofsky's and @hodlinator's concerns about being too strict about user hex input for `-assumevalid` and `RANDOM_CTX_SEED`. Thanks again for sharing your concerns about breaking changes (and the detailed review), in hindsight I think this is indeed the better approach.

Specifically, these last two force-pushes:
- introduces a `uint256::FromUserHex()` to allow for lenient user input parsing and
does away with the `TrySanit
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726878931)
Thank you for summarizing your concerns @ryanofsky. I believe the latest force-push addresses all of them.

> It's good this change is making it an error to include trailing non-hex characters instead of silently ignoring them.
It's good this change is making it an error to specify hex strings that are too long.
It's probably ok this change makes it an error to include leading and trailing whitespace, instead of trimming it. Maybe that could be safer and prevent configuration bugs.

Releva
...

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726868348)
Resolved by removing `TrySanitizeHexNumber()` altogether (and empty strings are still allowed in `uint256::FromUserHex`, as per current behaviour on `master`)

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726858671)
Thanks, I've added these two test cases!

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726863840)
Oops, it wasn't, thanks for spotting. Reverted in latest force-push.

💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726864665)
Taken both suggestions in latest force-push, thanks.

💬 marcofleon commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726881676)
`i2p.cpp` is missing here

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726889959)
Thanks!

I don't consider this a blocker, so it will be fixed in a follow-up.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2304454120)
My Guix build:
```
x86_64
c4e0333106b259d1ab11066677fc32fba8dd380afc2f9c334d9eff2dccccba39 guix-build-41051290ab3b/output/aarch64-linux-gnu/SHA256SUMS.part
bfce0ebd120bd45576586691c46b47e66bb0031f7540772a924d4cb1c2602576 guix-build-41051290ab3b/output/aarch64-linux-gnu/bitcoin-41051290ab3b-aarch64-linux-gnu-debug.tar.gz
615aae82e503fc07ee1053649b3a45ad2f0dd9c02d5a85394f8ba33b3bd5be88 guix-build-41051290ab3b/output/aarch64-linux-gnu/bitcoin-41051290ab3b-aarch64-linux-gnu.tar.gz
2a8ef06d7
...

💬 Sjors commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#discussion_r1726894640)
Thanks, will fix if I need to retouch.

💬 vasild commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726899916)
Is there a list of known things to be addressed in a followup? It would help us not to forget things and also should avoid people reporting duplicates.

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2304465368)
I like it a lot more now, thanks for considering the feedbacks.

The only part that still bugs me a bit is the naming of `FromUserHex` -> the "user" is on a different abstraction level, I'd prefer mentioning the behavior change compared to the sibling method (i.e. it's more forgiving/leninent for the inputs), rather than the motivation for adding the method (the users sometimes prefer different formats).

ACK 81554aac80bf2270db977c110c37acc7e8034194

💬 l0rinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1726880388)
nit, if you touch again, consider:
```suggestion
std::cerr << RANDOM_CTX_SEED << " must consist of up to " << uint256::size() * 2 << " hex digits (\"0x\" prefix allowed), it was set to: '" << num << "'.\n";
```

💬 l0rinc commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726907251)
we're adding some follow-ups as PRs to https://github.com/hebasto/bitcoin/pulls

💬 Sjors commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2304492506)
Getting the same guix hashes as @hebasto and x86_64.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1726922698)
> we're adding some follow-ups as PRs to https://github.com/hebasto/bitcoin/pulls

Right. This, and https://github.com/bitcoin/bitcoin/issues?q=label%3A%22Needs+CMake+port%22+is%3Aclosed.

💬 l0rinc commented on pull request "Fix maybe-uninitialized warning in IsSpentKey":
(https://github.com/bitcoin/bitcoin/pull/30691#issuecomment-2304495811)
utACK 17707db939cb09a16c002d226152e71fce9289f2

👍 BrandonOdiwuor approved a pull request: "Fix maybe-uninitialized warning in IsSpentKey"
(https://github.com/bitcoin/bitcoin/pull/30691#pullrequestreview-2254358116)
Code Review ACK 17707db939cb09a16c002d226152e71fce9289f2