💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685806155)
nit:
```suggestion
return kp.IsValid() && kp.SignSchnorr(hash, sig, aux);
```

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685807082)
One last attempt from my part, but it's not a blocker, would appreciate if you would consider it anyway: in the previous example we've solved this basically by changing the return aggregation (into `ret`) with assertions instead, since we knew that in reality we can't have short-circuiting here. It could make sense to do the same here (if that's the reasoning) and leave a simple return here as well.

⚠️ vostrnad opened an issue: "Faster way to get block with prevouts in JSON-RPC"
(https://github.com/bitcoin/bitcoin/issues/30495)
I often need to process the whole blockchain (or a large part of it) using an external script/program, for which I need blocks with prevout information included. However, the only current way to get that is `getblock <hash> 3`, which includes a lot of potentially unnecessary data and is quite slow, mainly (based on my experiments) because of `UniValue` overhead and descriptor inferring.

I benchmarked current master, retrieving 1000 blocks sequentially starting at block 840000, with different
...

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686039008)
The build logs may be too verbose then, but I'll reconsider it in a follow-up, when more work has landed.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686041566)
> isValid

For new code, that'd need to be snake_case. Other than that, I think it is clear that a mutable method called `Set...(...)` mutates, so there is no need to change the code here. Also, the dev guide doesn't mention it.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686043181)
This shouldn't make a difference in the happy path. Optimizing for the non-happy path seems useless, as no user should be using it (outside of a few test cases).

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686045338)
It does not, because trailing non-hex was always skipped.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686049440)
Should probably be done in https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2241616078, or in a separate pull reqeust. (The method already exists, and if the tests do not exists, one can add them today on master).

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686051299)
No, it is not a factory method, because it does not return an object. I kept the previous name, but I am happy to change it, if there is a reason.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686054773)
> TryParseHex

This pull request doesn't modify (or mention) this helper, so I think it is unrelated. Also, the test already exist in:

```
src/test/util_tests.cpp: result = ParseHex("12 34 56 78");
src/test/util_tests.cpp: result = ParseHex(" 89 34 56 78");
src/test/util_tests.cpp: result = ParseHex(" Ff aA ");
src/test/util_tests.cpp: result = ParseHex("");
src/test/util_tests.cpp: BOOST_CHECK_EQUAL(ParseHex("AAF F").size(), 0);
src/test/util_tests.cpp:
...

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686055997)
With "fragile" I mean that it breaks code that uses it and leads to bugs. See for example the bug that was fixed in this pull request.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686056295)
Good point. I'll just remove this comment.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686056463)
(Same as above)

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686057241)
I am not touching this line of code, so it seems unrelated to refactor it.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686057721)
Happy to review a pull request doing this, but I won't be doing it myself.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686060040)
I am not changing the endian behavior in this instance, so maybe a separate pull can improve the documentation?

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686061163)
See https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2241616078

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686061254)
See https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2241616078

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686065219)
> What is your reasoning for deprecating rather than removing and correcting call-sites

I don't think fixing all bugs in all places in a single pull request makes sense. Often, context specific fixes are needed, so separate pull requests can be used.

> is that part of why this PR is a draft?

It is draft, because it includes a merge commit of other commits.



> If you keep it, `TxidFromString` should probably be renamed `TxidFromStringFragile` to follow `SetHexFragile`

Sure, but
...

💬 maflcko commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1686075826)
No, because the fuzz engine will think that the trimming code is then relevant to the fuzz target. Thus, it will treat coverage feedback in the trimming code as useful and bloat the fuzz input set. Skipping it should be effective to avoid that, apart from https://github.com/bitcoin/bitcoin/pull/27552 which should be a "complete" fix. But this can be done later.