💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743729122)
question in 948bb07715f9ef1d90b91dfbea4f487f755498f5: Is there a reason why the tests are dropped? I read the commit message and I understand that the comment says "String constructors", but I think it meant to say `UintToArith256 testing from a string`, which is not a test-only function.

This is my fault, because I forgot to update the comment in commit facf629ce8ff1d1f6d254dde4e89b5018f8df60e.

I also understand that there is a `BOOST_AUTO_TEST_CASE(conversion)` test. However, I think it
...

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743786908)
Sorry for misunderstanding, but I think an error message of

`"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f')"` is unclear whether the error is that there are non-hex digits or whether there are missing or extraneous characters. And it is not trivial to see from looking at it.

A proposed error message of `"txid must be string of 64 hex digits (not '1d999634f709c98f242f2e553bf0fdac7deb6f972c44c8c1b4d576d564acc7f' of len $num)"` wh
...

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743805522)
I tend to agree that the length is superfluous (we're usually not *that* user friendly with our error messages) and in case of error the user would likely copy-paste again which would likely solve the error.
I'm fine with both, just don't revert anything @stickies-v :)

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743810469)
Some of these were reverted since it seemed to us that it's testing prefixes and whitespaces primarily, see https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740942391

💬 maflcko commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1743817035)
I agree the prefix and whitespace testing is useless and the prefix and whitespace can simply be removed in the test cases, but it is not trivially clear to me that the `UintToArith256` tests can be removed.

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