💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1685786159)
Right. I understand what `-ffile-prefix-map=...` does.

My question was about a change that introduced such non-determinism, given that the source code is the same.

The `-ffile-prefix-map=...` flag is neither required when building `zeromq` with Autotools nor used by any other package in depends.

👍 itornaza approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2190378474)
re ACK b8b3a9f18670ec7bf246a57950cdae7e034a264d

Both of you did an amazing job over this weekend and I learned a lot by just watching this thread! Just to re-confirm from my side as well, the aforementioned `KeyPair()` call removal comes without any security implications as we discussed above https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685779789. However, just for the sake of paranoia I build the commit in debug configuration and run the tests in debug mode using `lldb` and con
...

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685726873)
What is your reasoning for deprecating rather than removing and correcting call-sites to not use `TxidFromString` - is that part of why this PR is a draft? I can't see many other deprecated things in the codebase beyond RPCs.

If you keep it, `TxidFromString` should probably be renamed `TxidFromStringFragile` to follow `SetHexFragile`, but I understand you want to avoid touching **src/qt/** source files as much as possible. Maybe one could at least change the implementation to make it clearer
...

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685798158)
Noticed `ParseHashV(hash_str, ...)` is needlessly called a few lines below instead of using the already verified `hash` value.

🤔 hodlinator reviewed a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2188748227)
Good to see the added functional test conditions for truncated txids!

💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685797580)
(In response to https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684414871):
To see how `FromHex` would look, I implemented a commit on top of this PR that does that transformation: 404c115ea0f53785a640818c2058ef0591c8c6ac

It turned out well enough that I hope you will at least consider it. Could use more `auto` to reduce typing. In my mind it's worth sacrificing some typing and a slightly larger diff to get clarity and RAII.

👍 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)