🚀 fanquake merged a pull request: "Fix MSVC warning C4273 "inconsistent dll linkage""
(https://github.com/bitcoin/bitcoin/pull/30491)
(https://github.com/bitcoin/bitcoin/pull/30491)
💬 reardencode commented on pull request "policy: enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241158440)
ACK
(https://github.com/bitcoin/bitcoin/pull/30493#issuecomment-2241158440)
ACK
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685446875)
I'm fairly confident this is not a behavior change, but could have missed something.
If you look at `secp256k1_keypair_xonly_pub`, this will only fail if there was an issue creating the `keypair` object, but this is checked above so we would have already short-circuited. The same is true for `secp256k1_keypair_xonly_pubkey_serialize` (only fails if we can't load the `keypair`, which only fails if the `keypair` is malformed).
`secp256k1_keypair_xonly_tweak_add` will fail if `tweak32` is gr
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685446875)
I'm fairly confident this is not a behavior change, but could have missed something.
If you look at `secp256k1_keypair_xonly_pub`, this will only fail if there was an issue creating the `keypair` object, but this is checked above so we would have already short-circuited. The same is true for `secp256k1_keypair_xonly_pubkey_serialize` (only fails if we can't load the `keypair`, which only fails if the `keypair` is malformed).
`secp256k1_keypair_xonly_tweak_add` will fail if `tweak32` is gr
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685450966)
Personally, I don't see any reason to not reuse `ret` and I think it makes the logic easier to follow vs re-initializing the variable. Regarding returning inside the `if (ret)`, as mentioned in https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685446875, `keypair_xonly_pub` will only fail if the `keypair` is malformed, so if we've successfully created the keypair this call should never fail. So if `schnorrsig_verify` fails, we check it on the next line and clear the signature data befor
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685450966)
Personally, I don't see any reason to not reuse `ret` and I think it makes the logic easier to follow vs re-initializing the variable. Regarding returning inside the `if (ret)`, as mentioned in https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685446875, `keypair_xonly_pub` will only fail if the `keypair` is malformed, so if we've successfully created the keypair this call should never fail. So if `schnorrsig_verify` fails, we check it on the next line and clear the signature data befor
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685454366)
Good catch, I think you're right that if `xonly_tweak_add` were to fail (e.g. tweak32 is the negation of the secret key), then we would return without explicitly clearing the keypair variable. That being said, the variable should already be cleared when it goes out of scope, so the explicit `memory_cleanse` is more of a belt and suspenders approach, from what I understand.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685454366)
Good catch, I think you're right that if `xonly_tweak_add` were to fail (e.g. tweak32 is the negation of the secret key), then we would return without explicitly clearing the keypair variable. That being said, the variable should already be cleared when it goes out of scope, so the explicit `memory_cleanse` is more of a belt and suspenders approach, from what I understand.
💬 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).