💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743824670)
Ah yes that would be better, will update this in next force-push, thanks!

💬 maflcko commented on pull request "test: add check that too large txs aren't put into orphanage":
(https://github.com/bitcoin/bitcoin/pull/30784#issuecomment-2329110687)
review ACK 66d13c870284327abc89d36c0b5cc5f58e96f570

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2329122240)
ACK da4377dc2ad0f495ebb97722d6cc2a95850363fa

I didn't mind the previous version either, but maybe this preserves more of what's not strictly related to the PR.
I think it's an improvement as-is, but I will gladly reack if you decide to include the other reviews as well.

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743824684)
Nit1: consider narrowing the scope of expected:
Nit2: usually we compare the actual against the expected (switched the order):
```suggestion
if (auto expected_len{uint256::size() * 2}; strHex.length() != expected_len) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, expected_len, strHex.length(), strHex));
}
```

No strong feelings either way, feel free to ignore if you don't like it

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743835345)
> is not trivially clear to me that the UintToArith256 tests can be removed

+1 for restoring them in that case

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743832221)
You mean something like:
```C++
uint256 ParseHashV(const UniValue& v, std::string_view name)
{
const std::string& strHex(v.get_str());
if (auto rv{uint256::FromHex(strHex)}){
return *rv;
} else if (auto expected_len{uint256::size() * 2}; strHex.length() != expected_len) {
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s must be of length %d (not %d, for '%s')", name, expected_len, strHex.length(), strHex));
} else {
throw JSONRPCError(RPC_
...

💬 fanquake commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1743843129)
From https://github.com/bitcoin/bitcoin/actions/runs/10665138943/job/29557888413#step:8:1:
```bash
Preset CMake variables:
BUILD_GUI="ON"
VCPKG_TARGET_TRIPLET="x64-windows-static"
WITH_NATPMP="OFF"
WITH_QRENCODE="OFF"
```
Can you explain why some options are passed on the command line, and why some are put into `CMakePresets.json`; what determines where each option belongs? I'd think it'd be better to have a single source of truth for the CI config.

Also, I see [compared to 28
...

💬 fjahr commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743843312)
Yeah, much simpler, thanks, done!

💬 fjahr commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743843443)
done

💬 fjahr commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743843548)
done

💬 fanquake commented on pull request "ci: Add missed configuration options to "Win64 native" job":
(https://github.com/bitcoin/bitcoin/pull/30755#discussion_r1743850784)
For example, I can imagine someone changes `CMakePresets.json`, without realising that also partially controls the CI, and then we (silently) lose Qt compilation in this build.

💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743855915)
Forgot the CBlockIndex `*` -> `&` ? :sweat_smile:

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2329149553)
@sdaftuar awesome data! Encouraging to see the timings move along with what @sipa was asserting :+1:

👍 maflcko approved a pull request: "rpc: dumptxoutset height parameter follow-ups (29553)"
(https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2280325494)
re-ACK 769dc84852e6f14876d595a7d628cc02fa074c75 🎃

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 769dc84852e6f14876d5
...

💬 maflcko commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743863145)
nit in the last commit: Shouldn't this be a LogWarning, so that the user knows that the log is related to an RPC error that happened? Also, it could include the name of the RPC that triggered the log.

🤔 l0rinc reviewed a pull request: "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage"
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2280307916)
> good point thanks, perhaps that would be good to clarify in a docstring?

Do you think it's important in this case? We're not using the `random_hex_string` as input here at all, only the output.
If you do, please give me a diff an I'll apply it.

💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1743859614)
Git blame without proper history is useless anyway, I always needed a bisect to see the real author, so I don't really see the advantage of optimizing for that.
But mirroring the header file's structure does simplify the code a bit.
Let me know if you feel strongly about it and I'll revert, but I would consider that a step back.

💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1743852827)
If we think this takes a lot of time, we can optimize it by removing cases that we think are unlikely to result in revealing failures.
We can also do it in separate steps, if we see that it's stable for months, remove the excess.
Maybe @maflcko can help us out here.

💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1743862579)
nice, if I retouch I'll apply it!

💬 fjahr commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1743870042)
Ah, yeah, I overlooked that. Fixed.