💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685396384)
We shouldn't need zeroing, if we're processing the values exhaustively - but if we do, we could use
```suggestion
SetNull();
```

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685401233)
I find it quite confusing that we're sneaking in a mutation of the copy, while previously we just returned a copy directly (like `TrimStringView`).
We could `const` the parameters if we did the same as before:
```suggestion
return str.substr(prefix.size());
```
i.e.
```C++
[[nodiscard]] inline std::string_view RemovePrefixView(const std::string_view str, const std::string_view prefix)
```

----

Could you please add a test case that checks the integrity of the original:
```
...

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685455046)
shouldn't this remain `SetHex` here?

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685397114)
previously we've only removed leading whitespaces, but `TrimStringView` removed trailing ones as well. Not sure if this changes the overall behavior, though.

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685445530)
This isn't actually fragile (i.e. breaks easily), quite the opposite, it's rather `lenient`, i.e. accepts unvalidated input.
Could we name it as `FromHexUnchecked` or `FromHexLenient` instead?

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685505233)
it's narrowing the scope, what would be the reason for reusing a variable and widening its scope?
If you feel strongly about it, I don't particularly mind either way.

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685506662)
nit: we could move this down, after the first early return, since it's not needed before that

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685507018)
Your version always executes all steps (e.g. computes `XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);` even after `secp256k1_keypair_load` wasn't successful, right? Previously this method wasn't executed, so at best it's a performance difference, at worst we're hoping that each method handles this error properly (which wasn't necessary before, since the first method guarded them).

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685507979)
there's a lot of ugly duplication because of all the `reinterpret_cast` changes. Wouldn't it make sense to extract those and simplify the function and the diff?

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685508729)
right, I thought that `make` would catch that, not sure why it allowed a `memset` on a const (most be C vs C++ boundary, I guess).

🤔 paplorinc requested changes to a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2190030725)
Thanks for the fixes!
I'm still not convinced that the lack of short-circuits didn't introduce any change.
Please prove me wrong, I want to ACK this :)

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685512256)
That is surprising. I applied your patch directly in `bitcoin-core/secp256k1` and did get a warning:

```
src/modules/extrakeys/main_impl.h:266:12: warning: passing argument 1 of ‘memset’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
266 | memset(keypair, 0, sizeof(*keypair));
| ^~~~~~~
In file included from src/ecmult_impl.h:10,
from src/secp256k1.c:30:
/usr/include/string.h:61:28: note: expected ‘void *’ but argumen
...

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685519477)
Right, my point is there is no way to call `CKey::SignSchnorr` such that `_keypair_create` will succeed but `_xonly_pub` or `_serialize` will fail. Annoyingly, this means in the old version we have branches in the code that we cannot test (easy to prove me wrong if you have a test case :wink:). Hence, I stand by the statement this is not a behavior change and we are merely disagreeing over code aesthetics.

I think you make a good point that _future_ changes to libsecp256k1 could change this (
...

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685520450)
Or maybe introduce a casting helper like https://github.com/bitcoin/bitcoin/blob/master/src/span.h#L295

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685520827)
Do you agree that computations are done now, that would have been avoided previously? It might return the same value, but it does different calculations than before.

> it to be a bit smelly when we have if branches in the code we cannot test.

absolutely, but I don't yet see how avoiding short-circuiting helps with the testing - but exploding it into multiple methods certainly did!

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685522043)
Ah, sorry, I misunderstood your suggestion as "extract" (copy) the data out into a variable but you're suggesting saving the pointer as a variable. I think this would simplify the diff a lot, will give it a shot.

The only thing I don't like here is needing to pass it as non-const to two functions and const to the other two, but I think its okay to pass a non-const object to a function with const in the signature?

✅ brunoerg closed a pull request: "fuzz: compare scripts from `Expand` and `ExpandFromCache`"
(https://github.com/bitcoin/bitcoin/pull/28908)

💬 brunoerg commented on pull request "fuzz: compare scripts from `Expand` and `ExpandFromCache`":
(https://github.com/bitcoin/bitcoin/pull/28908#issuecomment-2241267637)
Closing for now.

👍 tdb3 approved a pull request: "rpc: doc: use "output script" terminology consistently in "asm"/"hex" results"
(https://github.com/bitcoin/bitcoin/pull/30408#pullrequestreview-2190051738)
ACK 29eafd5733d77b3e8f3f3ab6cd65c61ac0e8536b
Thanks for adding clarity.
Left a nit.

💬 tdb3 commented on pull request "rpc: doc: use "output script" terminology consistently in "asm"/"hex" results":
(https://github.com/bitcoin/bitcoin/pull/30408#discussion_r1685526349)
nit:
`witness program` or `output script` seems like it might be more accurate than `witness output script`.