💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685459510)
Hm, I don't think it makes much of a difference for the diff and makes this particular staging commit more confusing by creating the `KeyPair` object just to pull out the data back out into a `secp256k1_keypair` object. The main advantage of the `KeyPair` wrapper class is that we are keeping the keypair in secure memory, which we would be undoing here by copying it out into a `keypair` object.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685459510)
Hm, I don't think it makes much of a difference for the diff and makes this particular staging commit more confusing by creating the `KeyPair` object just to pull out the data back out into a `secp256k1_keypair` object. The main advantage of the `KeyPair` wrapper class is that we are keeping the keypair in secure memory, which we would be undoing here by copying it out into a `keypair` object.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685460212)
`secp256k1_keypair_xonly_tweak_add` modifies the `keypair` object with `tweak32`, so it can't be const. More generally, libsecp256k1 is a dependency of Bitcoin Core, so we shouldn't ever be modifying `secp256k1_` functions in our code base (despite us pulling it in as a subtree).
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685460212)
`secp256k1_keypair_xonly_tweak_add` modifies the `keypair` object with `tweak32`, so it can't be const. More generally, libsecp256k1 is a dependency of Bitcoin Core, so we shouldn't ever be modifying `secp256k1_` functions in our code base (despite us pulling it in as a subtree).
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685462749)
`IsValid()` returns false when `m_keypair` data is a nullptr, which effectively means the key data was never created or has been cleared (see `src/support/allocators/secure.h`)
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685462749)
`IsValid()` returns false when `m_keypair` data is a nullptr, which effectively means the key data was never created or has been cleared (see `src/support/allocators/secure.h`)
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685467250)
> Since keypair is a local variable in this function shouldn't the KeyPair destructor do the secure erase job anyway at the end of the scope through RAII since m_keypair is a secure_unique_ptr
Correct, which should happen regardless of whether or not we explicitly clear the key data with `keypair = KeyPair();`. Having `keypair = KeyPair()` a belt and suspenders by explicitly clearing the variable. This is because if we create a `KeyPair` object with the default constructor, there is no call t
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685467250)
> Since keypair is a local variable in this function shouldn't the KeyPair destructor do the secure erase job anyway at the end of the scope through RAII since m_keypair is a secure_unique_ptr
Correct, which should happen regardless of whether or not we explicitly clear the key data with `keypair = KeyPair();`. Having `keypair = KeyPair()` a belt and suspenders by explicitly clearing the variable. This is because if we create a `KeyPair` object with the default constructor, there is no call t
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2241187170)
Updated https://github.com/bitcoin/bitcoin/commit/5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec -> https://github.com/bitcoin/bitcoin/commit/5d9a6cf6286f9a7f93527ea76b910537d709a860 ([add-apply-taptweak-method-01](https://github.com/josibake/bitcoin/tree/add-apply-taptweak-method-01) -> [apply-taptweak-method-02](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-02) ([compare](https://github.com/josibake/bitcoin/compare/add-apply-taptweak-method-01..josibake:apply-taptweak-method-02))
...
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2241187170)
Updated https://github.com/bitcoin/bitcoin/commit/5e9f4f41de98c2cdcc74e31fe925b2f5cf9f6aec -> https://github.com/bitcoin/bitcoin/commit/5d9a6cf6286f9a7f93527ea76b910537d709a860 ([add-apply-taptweak-method-01](https://github.com/josibake/bitcoin/tree/add-apply-taptweak-method-01) -> [apply-taptweak-method-02](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-02) ([compare](https://github.com/josibake/bitcoin/compare/add-apply-taptweak-method-01..josibake:apply-taptweak-method-02))
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685473421)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685473421)
Fixed.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685473484)
Fixed.
(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.
(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.
(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.
(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.
(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.
(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.
(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.
(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).
(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.
(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
(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) {
```
(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_
...
(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.
(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
(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