💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685780351)
Updated.

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685780941)
Thanks! I added a modified version of this as a test. I took out the iterations because creating the keys, signing, etc is an expensive operation and I don't think it adds anything to do multiple rounds every time the tests are ran (i.e., if it this were to fail, it will fail for _all_ keys, not a particular key).

This also means the merkle root isn't be switched, but that's fine since the thing we actually want to test here is that we get the same results using two different methods of getti
...

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685781344)
Leaving as is for the reasons mentioned in https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685528003

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1685781788)
I'm curious why this flag became necessary during the migration to CMake to achieve cross-arch reproducibility?

🤔 hebasto reviewed a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723#pullrequestreview-2190373199)
CMake compiles 7 fewer source files compared to Autotools. It skips::
- `gssapi_client.cpp`
- `gssapi_mechanism_base.cpp`
- `gssapi_server.cpp`
- `vmci_address.cpp`
- `vmci_connecter.cpp`
- `vmci.cpp`
- `vmci_listener.cpp`

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1685783759)
To remove the non-deterministic file prefixes being added to the library.

💬 jbesraa commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2241724750)
I am interested in taking a look at this issue if nobody else is working on it already

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