💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1906326392)
> According to my `range-diff` nothing changed. reACK [18ad1b9](https://github.com/bitcoin/bitcoin/commit/18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d)
for context, this was the reason for the rebase: https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1906227242
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1906326392)
> According to my `range-diff` nothing changed. reACK [18ad1b9](https://github.com/bitcoin/bitcoin/commit/18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d)
for context, this was the reason for the rebase: https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1906227242
💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463486871)
I liked more the previous version which called `fread(3)` here. It was simple stupid. This `>>` is now hard to follow, especially given that it depends on `T`. For `vector` it ends up calling `AutoFile::detail_fread()`. It does not check whether `ferror(3)` occurred.
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463486871)
I liked more the previous version which called `fread(3)` here. It was simple stupid. This `>>` is now hard to follow, especially given that it depends on `T`. For `vector` it ends up calling `AutoFile::detail_fread()`. It does not check whether `ferror(3)` occurred.
💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463498321)
Same as above, the [dumb](https://en.wikipedia.org/wiki/KISS_principle) version was easier to follow. The new one does not check whether `fclose(3)` failed, but it should. I think that is a serious deficiency in `AutoFile` itself.
`fwrite(3)` may succeed, but if a subsequent `fclose(3)` fails we should consider the data did not make it safely to disk and that the file is corrupted (`fclose(3)` writes any buffered data to disk using `fflush(3)`, so a failure at `fclose(3)` is as bad as failure
...
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463498321)
Same as above, the [dumb](https://en.wikipedia.org/wiki/KISS_principle) version was easier to follow. The new one does not check whether `fclose(3)` failed, but it should. I think that is a serious deficiency in `AutoFile` itself.
`fwrite(3)` may succeed, but if a subsequent `fclose(3)` fails we should consider the data did not make it safely to disk and that the file is corrupted (`fclose(3)` writes any buffered data to disk using `fflush(3)`, so a failure at `fclose(3)` is as bad as failure
...
💬 ryanofsky commented on pull request "ci: Rename tasks (previous releases, macOS cross)":
(https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1906382136)
Would it maybe make sense to have a wiki page with a reverse chronological list of known CI failures? I think it would be useful to have a simple place to check if there is a known issue when a CI job is failing, but I'm not sure if it would create a maintenance burden.
(https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1906382136)
Would it maybe make sense to have a wiki page with a reverse chronological list of known CI failures? I think it would be useful to have a simple place to check if there is a known issue when a CI job is failing, but I'm not sure if it would create a maintenance burden.
🤔 vasild reviewed a pull request: "CKey: add Serialize and Unserialize"
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1839192623)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1839192623)
Concept ACK
💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1463523098)
Is it not worth saving `CKey::fCompressed` to disk as well and fully support ser/unser of any `CKey`?
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1463523098)
Is it not worth saving `CKey::fCompressed` to disk as well and fully support ser/unser of any `CKey`?
💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1463517690)
Use `secure_allocator` for the vector because it also ensures the data is not swapped out to disk. Then you do not need the `memory_cleanse()` call at the end:
```suggestion
std::vector<unsigned char, secure_allocator<unsigned char>> key_data(32);
s >> MakeWritableByteSpan(key_data);
this->Set(key_data.begin(), key_data.end(), true);
```
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1463517690)
Use `secure_allocator` for the vector because it also ensures the data is not swapped out to disk. Then you do not need the `memory_cleanse()` call at the end:
```suggestion
std::vector<unsigned char, secure_allocator<unsigned char>> key_data(32);
s >> MakeWritableByteSpan(key_data);
this->Set(key_data.begin(), key_data.end(), true);
```
💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1463531442)
Wait, I forgot that this is inside a `CKey` method and we have access to the private `keydata` member. Should be possible to write directly to it, without an intermediate vector. Something like:
```cpp
MakeKeyData();
s >> MakeSpanFromCKeykeydataomgwhatisthis(keydata->begin(), keydata->end());
```
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1463531442)
Wait, I forgot that this is inside a `CKey` method and we have access to the private `keydata` member. Should be possible to write directly to it, without an intermediate vector. Something like:
```cpp
MakeKeyData();
s >> MakeSpanFromCKeykeydataomgwhatisthis(keydata->begin(), keydata->end());
```
🤔 sr-gi reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1829978188)
Approach ACK
I think this looks good overall, I left some potential improvements/questions but feel free to disregard them given this is just for testing
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1829978188)
Approach ACK
I think this looks good overall, I left some potential improvements/questions but feel free to disregard them given this is just for testing
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457677043)
I wonder whether it may be worth defining a `Peer` class for this and move the peer-building logic of `initialize_v2_transport` here. I don't think this would make a huge difference, but it would make accessing the elements of peer cleaner for the callers. e.g. `self.peer.send_L` instead of `self.peer["send_L"]`
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457677043)
I wonder whether it may be worth defining a `Peer` class for this and move the peer-building logic of `initialize_v2_transport` here. I don't think this would make a huge difference, but it would make accessing the elements of peer cleaner for the callers. e.g. `self.peer.send_L` instead of `self.peer["send_L"]`
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457621863)
Why is 32 here being passed in every tuple if it really is used as a constant?
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457621863)
Why is 32 here being passed in every tuple if it really is used as a constant?
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457652467)
Why is `garbage_terminators` being actively deleted here instead of letting the deletion be managed by the interpreter once we go out of context (and `peer` is deleted as a whole)?
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457652467)
Why is `garbage_terminators` being actively deleted here instead of letting the deletion be managed by the interpreter once we go out of context (and `peer` is deleted as a whole)?
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462241588)
Sorry, I meant right after peer2, referring to grouping the connections in the test by either inbound or outbound.
peer1 does v2 **from** v2
peer2 does v2 **from** v1
peer3 does v2 **to** v1
[...] the rest are all x **to** y
and then peer7 goes back to peer1 does v1 **from** v2
Certainly not a big deal though
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462241588)
Sorry, I meant right after peer2, referring to grouping the connections in the test by either inbound or outbound.
peer1 does v2 **from** v2
peer2 does v2 **from** v1
peer3 does v2 **to** v1
[...] the rest are all x **to** y
and then peer7 goes back to peer1 does v1 **from** v2
Certainly not a big deal though
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457654033)
I initially thought this had to do with:
> \# To achieve forward secrecy we must wipe the key material used to initialize the ciphers
But turns out we are not doing it for the key material.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457654033)
I initially thought this had to do with:
> \# To achieve forward secrecy we must wipe the key material used to initialize the ciphers
But turns out we are not doing it for the key material.
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1463495866)
I wonder if it'd be worth also checking that `len(self.recvbuf) > 15` to avoid calling `v2_handshake` when we potentially don't have enough data to decide whether this a v1 or v2 connection. This potentially will call `v2_handshake` 16 times, and process processing byte arrays of length from 0..16.
In practice, I don't think this matters, given the logic never triggers: this happens when the python node is the responder, and the Core node will never send a half-baked `magic_bytes + version` (
...
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1463495866)
I wonder if it'd be worth also checking that `len(self.recvbuf) > 15` to avoid calling `v2_handshake` when we potentially don't have enough data to decide whether this a v1 or v2 connection. This potentially will call `v2_handshake` 16 times, and process processing byte arrays of length from 0..16.
In practice, I don't think this matters, given the logic never triggers: this happens when the python node is the responder, and the Core node will never send a half-baked `magic_bytes + version` (
...
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457665569)
We could even get rid of this whole re-assignment and just set it in the latter if/else:
```
self.peer['send_garbage_terminator'] = peer['garbage_terminators'][:16]
self.peer['recv_garbage_terminator'] = peer['garbage_terminators'][16:]
```
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457665569)
We could even get rid of this whole re-assignment and just set it in the latter if/else:
```
self.peer['send_garbage_terminator'] = peer['garbage_terminators'][:16]
self.peer['recv_garbage_terminator'] = peer['garbage_terminators'][16:]
```
💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1463533897)
Yes! This reads much clearer now :)
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1463533897)
Yes! This reads much clearer now :)
💬 maflcko commented on pull request "ci: Rename tasks (previous releases, macOS cross)":
(https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1906456732)
> Would it maybe make sense to have a wiki page with a reverse chronological list of known CI failures?
I think it would be easier to have each as an issue, which can be reverse chronologically sorted, or in any other way. There is a `Tests` label, but a new label can be added, if a dedicated "category" is needed. See, https://github.com/bitcoin/bitcoin/issues?q=is%3Aissue+is%3Aopen+label%3ATests
Using the issue tracker would also make it easy to search?
(https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1906456732)
> Would it maybe make sense to have a wiki page with a reverse chronological list of known CI failures?
I think it would be easier to have each as an issue, which can be reverse chronologically sorted, or in any other way. There is a `Tests` label, but a new label can be added, if a dedicated "category" is needed. See, https://github.com/bitcoin/bitcoin/issues?q=is%3Aissue+is%3Aopen+label%3ATests
Using the issue tracker would also make it easy to search?
💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463592487)
> For vector it ends up calling AutoFile::detail_fread(). It does not check whether ferror(3) occurred.
It does. If the return value of `detail_fread` is not `output.size()`, `operator>>` will fail.
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463592487)
> For vector it ends up calling AutoFile::detail_fread(). It does not check whether ferror(3) occurred.
It does. If the return value of `detail_fread` is not `output.size()`, `operator>>` will fail.
💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1463600444)
i took the [text from the BIP](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#cite_note-handshake_progress_19) here. On the python side, [`data_received()` asyncio function](https://docs.python.org/3/library/asyncio-protocol.html#asyncio.Protocol.data_received) only gets called when a bytes object is received.
(thinking about the c++ side)
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1463600444)
i took the [text from the BIP](https://github.com/bitcoin/bips/blob/master/bip-0324.mediawiki#cite_note-handshake_progress_19) here. On the python side, [`data_received()` asyncio function](https://docs.python.org/3/library/asyncio-protocol.html#asyncio.Protocol.data_received) only gets called when a bytes object is received.
(thinking about the c++ side)