💬 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?
🤔 hebasto reviewed a pull request: "depends: build zeromq with CMake"
(https://github.com/bitcoin/bitcoin/pull/29723#pullrequestreview-2190373199)
CMake compiles 7 fewer source files compared to Autotools. It skips::
- `gssapi_client.cpp`
- `gssapi_mechanism_base.cpp`
- `gssapi_server.cpp`
- `vmci_address.cpp`
- `vmci_connecter.cpp`
- `vmci.cpp`
- `vmci_listener.cpp`
(https://github.com/bitcoin/bitcoin/pull/29723#pullrequestreview-2190373199)
CMake compiles 7 fewer source files compared to Autotools. It skips::
- `gssapi_client.cpp`
- `gssapi_mechanism_base.cpp`
- `gssapi_server.cpp`
- `vmci_address.cpp`
- `vmci_connecter.cpp`
- `vmci.cpp`
- `vmci_listener.cpp`
💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1685783759)
To remove the non-deterministic file prefixes being added to the library.
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1685783759)
To remove the non-deterministic file prefixes being added to the library.
💬 jbesraa commented on issue "`scantxoutset` should output the block hash instead of block height":
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2241724750)
I am interested in taking a look at this issue if nobody else is working on it already
(https://github.com/bitcoin/bitcoin/issues/30478#issuecomment-2241724750)
I am interested in taking a look at this issue if nobody else is working on it already
💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1685786159)
Right. I understand what `-ffile-prefix-map=...` does.
My question was about a change that introduced such non-determinism, given that the source code is the same.
The `-ffile-prefix-map=...` flag is neither required when building `zeromq` with Autotools nor used by any other package in depends.
(https://github.com/bitcoin/bitcoin/pull/29723#discussion_r1685786159)
Right. I understand what `-ffile-prefix-map=...` does.
My question was about a change that introduced such non-determinism, given that the source code is the same.
The `-ffile-prefix-map=...` flag is neither required when building `zeromq` with Autotools nor used by any other package in depends.
👍 itornaza approved a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2190378474)
re ACK b8b3a9f18670ec7bf246a57950cdae7e034a264d
Both of you did an amazing job over this weekend and I learned a lot by just watching this thread! Just to re-confirm from my side as well, the aforementioned `KeyPair()` call removal comes without any security implications as we discussed above https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685779789. However, just for the sake of paranoia I build the commit in debug configuration and run the tests in debug mode using `lldb` and con
...
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2190378474)
re ACK b8b3a9f18670ec7bf246a57950cdae7e034a264d
Both of you did an amazing job over this weekend and I learned a lot by just watching this thread! Just to re-confirm from my side as well, the aforementioned `KeyPair()` call removal comes without any security implications as we discussed above https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1685779789. However, just for the sake of paranoia I build the commit in debug configuration and run the tests in debug mode using `lldb` and con
...
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685726873)
What is your reasoning for deprecating rather than removing and correcting call-sites to not use `TxidFromString` - is that part of why this PR is a draft? I can't see many other deprecated things in the codebase beyond RPCs.
If you keep it, `TxidFromString` should probably be renamed `TxidFromStringFragile` to follow `SetHexFragile`, but I understand you want to avoid touching **src/qt/** source files as much as possible. Maybe one could at least change the implementation to make it clearer
...
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685726873)
What is your reasoning for deprecating rather than removing and correcting call-sites to not use `TxidFromString` - is that part of why this PR is a draft? I can't see many other deprecated things in the codebase beyond RPCs.
If you keep it, `TxidFromString` should probably be renamed `TxidFromStringFragile` to follow `SetHexFragile`, but I understand you want to avoid touching **src/qt/** source files as much as possible. Maybe one could at least change the implementation to make it clearer
...
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685798158)
Noticed `ParseHashV(hash_str, ...)` is needlessly called a few lines below instead of using the already verified `hash` value.
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685798158)
Noticed `ParseHashV(hash_str, ...)` is needlessly called a few lines below instead of using the already verified `hash` value.
🤔 hodlinator reviewed a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2188748227)
Good to see the added functional test conditions for truncated txids!
(https://github.com/bitcoin/bitcoin/pull/30482#pullrequestreview-2188748227)
Good to see the added functional test conditions for truncated txids!
💬 hodlinator commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685797580)
(In response to https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684414871):
To see how `FromHex` would look, I implemented a commit on top of this PR that does that transformation: 404c115ea0f53785a640818c2058ef0591c8c6ac
It turned out well enough that I hope you will at least consider it. Could use more `auto` to reduce typing. In my mind it's worth sacrificing some typing and a slightly larger diff to get clarity and RAII.
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1685797580)
(In response to https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684414871):
To see how `FromHex` would look, I implemented a commit on top of this PR that does that transformation: 404c115ea0f53785a640818c2058ef0591c8c6ac
It turned out well enough that I hope you will at least consider it. Could use more `auto` to reduce typing. In my mind it's worth sacrificing some typing and a slightly larger diff to get clarity and RAII.