👍 paplorinc approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2190393727)
utACK b8b3a9f18670ec7bf246a57950cdae7e034a264d

Appreciate your perseverance and creativity @josibake, and your tests @itornaza.

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