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