💬 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!
(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?
(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)
(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.
(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.
(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`.
(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`.
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685527164)
if you end up using similar casting helper methods, the const part won't matter anymore, right?
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685527164)
if you end up using similar casting helper methods, the const part won't matter anymore, right?
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685528003)
> Do you agree that computations are done now, that would have been avoided previously?
No, this is the part I am disagreeing with. As I explained in the first message, there is no way for `_xonly_pub` or `_serialize` to fail if `keypair_create` succeeds. If `_create`, `_xonly_pub`, and `_serialize` succeed, then `XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);` is computed. After that, `_xonly_tweak_add` will be called. `_xonly_tweak_add` can fai
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685528003)
> Do you agree that computations are done now, that would have been avoided previously?
No, this is the part I am disagreeing with. As I explained in the first message, there is no way for `_xonly_pub` or `_serialize` to fail if `keypair_create` succeeds. If `_create`, `_xonly_pub`, and `_serialize` succeed, then `XOnlyPubKey(pubkey_bytes).ComputeTapTweakHash(merkle_root->IsNull() ? nullptr : merkle_root);` is computed. After that, `_xonly_tweak_add` will be called. `_xonly_tweak_add` can fai
...
👍 tdb3 approved a pull request: "net: log connections failures via SOCKS5 with less severity"
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2190054257)
Code Review ACK f3cfbd65f54b5b6423d786fb8850cece05af603e
(https://github.com/bitcoin/bitcoin/pull/30064#pullrequestreview-2190054257)
Code Review ACK f3cfbd65f54b5b6423d786fb8850cece05af603e
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685530015)
> there is no way for _xonly_pub or _serialize to fail if keypair_create succeeds
I wish it were easy to see that from the code, like before the change.
I'll check it out more thoroughly tomorrow - if you think you can make it more obvious in the code (e.g. if you're sure some of the values can only be true, you could maybe signal that by replacing confusing `ret &= ` code with asserts instead), please push it.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685530015)
> there is no way for _xonly_pub or _serialize to fail if keypair_create succeeds
I wish it were easy to see that from the code, like before the change.
I'll check it out more thoroughly tomorrow - if you think you can make it more obvious in the code (e.g. if you're sure some of the values can only be true, you could maybe signal that by replacing confusing `ret &= ` code with asserts instead), please push it.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2241534584)
Update https://github.com/bitcoin/bitcoin/commit/5d9a6cf6286f9a7f93527ea76b910537d709a860 -> https://github.com/bitcoin/bitcoin/commit/49057fc4f1e73a14f673c934573d727ae0229779 ([apply-taptweak-method-02](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-02) -> [apply-taptweak-method-03](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-03) ([compare](https://github.com/josibake/bitcoin/compare/apply-taptweak-method-02..josibake:apply-taptweak-method-03))
* Use `CKey
...
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2241534584)
Update https://github.com/bitcoin/bitcoin/commit/5d9a6cf6286f9a7f93527ea76b910537d709a860 -> https://github.com/bitcoin/bitcoin/commit/49057fc4f1e73a14f673c934573d727ae0229779 ([apply-taptweak-method-02](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-02) -> [apply-taptweak-method-03](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-03) ([compare](https://github.com/josibake/bitcoin/compare/apply-taptweak-method-02..josibake:apply-taptweak-method-03))
* Use `CKey
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685690725)
Spent some time thinking about this and realised the main issue here is we start with a valid secret key, create a keypair (which generates the public key in the process), and then extract the public key from the keypair and serialise it. Instead, we can just get the public key directly from the secret key via `CKey::GetPubKey`. This makes the original code more clear, eliminates the unnecessary if branches and makes the diff _much_ simpler.
If you look into `GetPubKey` you can see it’s calli
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685690725)
Spent some time thinking about this and realised the main issue here is we start with a valid secret key, create a keypair (which generates the public key in the process), and then extract the public key from the keypair and serialise it. Instead, we can just get the public key directly from the secret key via `CKey::GetPubKey`. This makes the original code more clear, eliminates the unnecessary if branches and makes the diff _much_ simpler.
If you look into `GetPubKey` you can see it’s calli
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685690873)
Resolving since this is no longer relevant.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685690873)
Resolving since this is no longer relevant.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685691416)
Took your suggestion of saving the pointer in a variable. I’m not sure about a cast helper, mainly because I don’t want to put `secp256k1_keypair` into a header file, so seems better to just have people use reinterpret cast wherever KeyPair is being used.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685691416)
Took your suggestion of saving the pointer in a variable. I’m not sure about a cast helper, mainly because I don’t want to put `secp256k1_keypair` into a header file, so seems better to just have people use reinterpret cast wherever KeyPair is being used.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685691767)
Removed all of the instances of initialisation the bool outside of a function call.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685691767)
Removed all of the instances of initialisation the bool outside of a function call.
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685696117)
this is SO MUCH BETTER! \\:D/ (it even has the asserts that we were talking about)
I didn't trust, but verified, of course, if you think it makes sense, please add this test to the commit (you can add me as co-author if you want as `l0rinc <pap.lorinc@gmail.com>`):
```C++
BOOST_AUTO_TEST_CASE(key_sign_schnorr_tweak_test)
{
secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
for (int i = 0; i < 1000; ++i) {
CKey key;
ke
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685696117)
this is SO MUCH BETTER! \\:D/ (it even has the asserts that we were talking about)
I didn't trust, but verified, of course, if you think it makes sense, please add this test to the commit (you can add me as co-author if you want as `l0rinc <pap.lorinc@gmail.com>`):
```C++
BOOST_AUTO_TEST_CASE(key_sign_schnorr_tweak_test)
{
secp256k1_context* secp256k1_context_sign = secp256k1_context_create(SECP256K1_CONTEXT_SIGN);
for (int i = 0; i < 1000; ++i) {
CKey key;
ke
...
🤔 paplorinc reviewed a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2190286464)
Nice, left a few nits and a clarification
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2190286464)
Nice, left a few nits and a clarification
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685703075)
this is the only remaining part that I'm uncomfortable with, even the IDE is complaining:
<img src="https://github.com/user-attachments/assets/52bf7173-a562-4dde-9f2c-9834b3a5d5da">
which I'm not getting for `BIP324Cipher::Initialize`, since it's assigning a field, not a local variable.
Is there a more intuitive way to do this? Do we have tests to check that this value is cleared?
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685703075)
this is the only remaining part that I'm uncomfortable with, even the IDE is complaining:
<img src="https://github.com/user-attachments/assets/52bf7173-a562-4dde-9f2c-9834b3a5d5da">
which I'm not getting for `BIP324Cipher::Initialize`, since it's assigning a field, not a local variable.
Is there a more intuitive way to do this? Do we have tests to check that this value is cleared?
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685699288)
nit: in other cases it makes sense to call it `ret`, since it's the return value, here it's just a validity check - if you think it makes sense, [consider renaming](https://github.com/bitcoin/bitcoin/blob/master/src/key.cpp#L338) to avoid the surprise
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685699288)
nit: in other cases it makes sense to call it `ret`, since it's the return value, here it's just a validity check - if you think it makes sense, [consider renaming](https://github.com/bitcoin/bitcoin/blob/master/src/key.cpp#L338) to avoid the surprise
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685701466)
nit: it's 64 because of https://github.com/bitcoin/bitcoin/blob/master/src/script/sign.cpp#L86, right?
It's not really called from other places, the assert seems like noise here.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685701466)
nit: it's 64 because of https://github.com/bitcoin/bitcoin/blob/master/src/script/sign.cpp#L86, right?
It's not really called from other places, the assert seems like noise here.