💬 brunoerg commented on issue "scriptpubkeyman fuzz target TopUp is slow":
(https://github.com/bitcoin/bitcoin/issues/30476#issuecomment-2241191443)
> Instead of calling `TopUp` every time we call `AddDescriptorKey`, maybe it could be done just one time per iteration. I can see it.
>
> Also, there are other functions, `GetNewDestination`, `MarkUnusedAddresses` and `AddWalletDescriptor` that internally call `TopUp()`.

Forget, reducing keypool size should be enough. Just opened #30494.

🤔 paplorinc requested changes to a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2189937467)
Nice cleanup, please see my questions and observations

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685387889)
When a method both mutates and returns, [we often extract it](https://github.com/bitcoin/bitcoin/blob/master/src/coins.cpp#L254), to clarify that it's not just a simple pure check.
```suggestion
bool isValid = hash.SetHex(hashStr);
if (!isValid) {
```

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685385755)
We might as well use the `[[deprecated]]` attribute instead:
```suggestion
[[deprecated("Missing length-check and hex-check, please use the safer SetHex, or FromUint256")]]
```

Resulting in compilation warnings for the remaining usages:

> bitcoin/src/qt/coincontroldialog.cpp:345:25: warning: 'TxidFromString' is deprecated: Missing length-check and hex-check, please use the safer SetHex, or FromUint256 [-Wdeprecated-declarations]
COutPoint outpt(TxidFromString(item->data(COLUMN_
...

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685389203)
What's the reason for inverting the clauses, previously if `!output` we short-circuited the check, now we're attempting to set the hex value even when output is empty.

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685393669)
+1, trims the same whitespace values as https://github.com/bitcoin/bitcoin/blob/master/src/util/strencodings.h#L169 before

💬 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