💬 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
(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).
(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?
(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).
(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 :)
(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
...
(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 (
...
(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
(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
💬 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
...