🤔 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.
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685702437)
`BOOST_CHECK` has very bad error messages, consider using the `_EQUAL` or in this case `_EQUAL_COLLECTIONS` for better errors:
```suggestion
BOOST_CHECK_EQUAL_COLLECTIONS(std::begin(sig64), std::end(sig64), sig.begin(), sig.end());
```
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685702437)
`BOOST_CHECK` has very bad error messages, consider using the `_EQUAL` or in this case `_EQUAL_COLLECTIONS` for better errors:
```suggestion
BOOST_CHECK_EQUAL_COLLECTIONS(std::begin(sig64), std::end(sig64), sig.begin(), sig.end());
```
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685697345)
nit: I understand this was the code before and that we've talked about this before, but in other places we use short circuiting logic for such code: https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/modules/extrakeys/main_impl.h#L185. Will leave it up to you to decide which is better, seems safer to me not to call verify when we're already in an invalid state.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685697345)
nit: I understand this was the code before and that we've talked about this before, but in other places we use short circuiting logic for such code: https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/modules/extrakeys/main_impl.h#L185. Will leave it up to you to decide which is better, seems safer to me not to call verify when we're already in an invalid state.
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685703954)
resolved
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685703954)
resolved
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685703852)
nice!
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685703852)
nice!
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685704154)
I'm still a bit hesitant here, see my related comment
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685704154)
I'm still a bit hesitant here, see my related comment
💬 paplorinc commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1685732896)
would it make sense to use a trimmed version here instead of completely skipping the call?
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1685732896)
would it make sense to use a trimmed version here instead of completely skipping the call?
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2241616078)
@paplorinc the first 3 commits here come from pending PR #30436, so comments on those changes are probably better to make there.
(https://github.com/bitcoin/bitcoin/pull/30482#issuecomment-2241616078)
@paplorinc the first 3 commits here come from pending PR #30436, so comments on those changes are probably better to make there.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685767669)
It's 64 because Schnorr signatures, unlike ECDSA signatures, are by definition 64 bytes (see https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#description), so we definitely want to keep this assert.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685767669)
It's 64 because Schnorr signatures, unlike ECDSA signatures, are by definition 64 bytes (see https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki#description), so we definitely want to keep this assert.
💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685769335)
Yes, of course, but that's already the case for every caller currently (i.e. not part of a public api, we know all the callers), and we're not really asserting this in other such methods, so it seems redundant to me. If you disagree, just resolve it, not a big deal either way.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685769335)
Yes, of course, but that's already the case for every caller currently (i.e. not part of a public api, we know all the callers), and we're not really asserting this in other such methods, so it seems redundant to me. If you disagree, just resolve it, not a big deal either way.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685772925)
In this case, I think it's better to match the style of the existing test but I've made a note regarding `_EQUAL` and `_EQUAL_COLLECTIONS` going forward.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685772925)
In this case, I think it's better to match the style of the existing test but I've made a note regarding `_EQUAL` and `_EQUAL_COLLECTIONS` going forward.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685779527)
Ah! I was misunderstanding `m_key = CKey()`. Since CKey (and KeyPair) are using secure allocator, the contents will be cleared before deletion (e.g. when the variable goes out of scope). But in the case of `BIP324Cipher::Initialize`, it seems `m_key` is used to initialize the connection, but then needs to be explicitly cleared by setting with an empty CKey (invalid CKey), because the member field will not go out of scope and be cleared.
So in this case, we don't need the `kp = KeyPair();` lin
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685779527)
Ah! I was misunderstanding `m_key = CKey()`. Since CKey (and KeyPair) are using secure allocator, the contents will be cleared before deletion (e.g. when the variable goes out of scope). But in the case of `BIP324Cipher::Initialize`, it seems `m_key` is used to initialize the connection, but then needs to be explicitly cleared by setting with an empty CKey (invalid CKey), because the member field will not go out of scope and be cleared.
So in this case, we don't need the `kp = KeyPair();` lin
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685779789)
My last comment was wrong, @itornaza is correct: the KeyPair destructor is doing the secure erase job and the `kp = KeyPair();` is unnecessary.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685779789)
My last comment was wrong, @itornaza is correct: the KeyPair destructor is doing the secure erase job and the `kp = KeyPair();` is unnecessary.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2241716848)
Update https://github.com/bitcoin/bitcoin/commit/49057fc4f1e73a14f673c934573d727ae0229779 - > https://github.com/bitcoin/bitcoin/commit/b8b3a9f18670ec7bf246a57950cdae7e034a264d([apply-taptweak-method-03](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-03) -> [apply-taptweak-method-04](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-04) ([compare](https://github.com/josibake/bitcoin/compare/apply-taptweak-method-03..josibake:apply-taptweak-method-04))
* Renamed `r
...
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2241716848)
Update https://github.com/bitcoin/bitcoin/commit/49057fc4f1e73a14f673c934573d727ae0229779 - > https://github.com/bitcoin/bitcoin/commit/b8b3a9f18670ec7bf246a57950cdae7e034a264d([apply-taptweak-method-03](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-03) -> [apply-taptweak-method-04](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-04) ([compare](https://github.com/josibake/bitcoin/compare/apply-taptweak-method-03..josibake:apply-taptweak-method-04))
* Renamed `r
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685780351)
Updated.
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685780351)
Updated.
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685780941)
Thanks! I added a modified version of this as a test. I took out the iterations because creating the keys, signing, etc is an expensive operation and I don't think it adds anything to do multiple rounds every time the tests are ran (i.e., if it this were to fail, it will fail for _all_ keys, not a particular key).
This also means the merkle root isn't be switched, but that's fine since the thing we actually want to test here is that we get the same results using two different methods of getti
...
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685780941)
Thanks! I added a modified version of this as a test. I took out the iterations because creating the keys, signing, etc is an expensive operation and I don't think it adds anything to do multiple rounds every time the tests are ran (i.e., if it this were to fail, it will fail for _all_ keys, not a particular key).
This also means the merkle root isn't be switched, but that's fine since the thing we actually want to test here is that we get the same results using two different methods of getti
...
💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685781344)
Leaving as is for the reasons mentioned in https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685528003
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685781344)
Leaving as is for the reasons mentioned in https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685528003
💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1685781788)
I'm curious why this flag became necessary during the migration to CMake to achieve cross-arch reproducibility?
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1685781788)
I'm curious why this flag became necessary during the migration to CMake to achieve cross-arch reproducibility?