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

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

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

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685473725)
Agree, I think `IsValid` makes this more readable, updated.

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

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

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685474212)
Agree, more readable to have this as `bool ret = true;`, updated.

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685475299)
Unless there is a functional disadvantage to having it this way, I prefer this for readability as it makes it more clear (to me, anyways) that we are accumulating the return values from the various secp256k1 function calls in the `ret` variable.

📝 brunoerg opened a pull request: "fuzz: reduce keypool size in scriptpubkeyman target"
(https://github.com/bitcoin/bitcoin/pull/30494)
Fixes #30476

This PR reduces keypool size in scriptpubkeyman fuzz target to avoid spend a lot of time in `TopUp` (which is obviously called by many spkm functions).

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