💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268245361)
I think the calls to `ftell` in `WriteBlockToDisk` and `UndoWriteToDisk` can be removed since the file position is known before the call and the number of bytes written is also known in the success case (in the failure case the writes will throw), so it should just be possible to increment the initial file position used in `OpenBlockFile` / `OpenUndoFile` ?

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1268252521)
I am still not convinced about `auto` and brace-initialization.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax mentions:
> Use = only when you are sure that there can be no narrowing conversions
> For built-in arithmetic types, use = only with auto

There will not be narrowing conversions with `auto x = expression;`

One of the examples down there uses `auto p = ...`.

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268252629)
I guess it depends on the data written. My benchmark was for 1MB written. With 33 bytes written, `ftell` is the dominant:


```
ROUTINE ====================== AutoFile::write in src/streams.cpp
2 28 Total samples (flat / cumulative)
. . 40: nSize -= nNow;
. . 41: }
. . 42: }
. . 43:
. . 44: void AutoFile::write(Span<const std::byte> src)
---
. . 45: {
. . 46: if (!m_file) th
...

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268255975)
I mean I am happy to make the change, but it seems brittle if someone calls `std::fseek` after the constructor?

💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268272983)
Hm good point, I didn't think about that -- it does seem like those functions are pretty contained since they open the file at the start and then close it at exit, but it does seem brittle

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268286275)
In any case the bottleneck doesn't seem to be `std::array<std::byte, 4096> buf;`, so I think this thread can be closed?

💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268291011)
yeah I didn't see allocations come up at all in perf

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268305587)
Maybe I'll pick up https://github.com/bitcoin/bitcoin/pull/28006 again and remove `Get`?

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1642386101)
> needs rebase if still relevant.

For the `previous releases` CI? (I don't find any merge conflicts with current master.)

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1642398008)
Pushed a small change to call `std::ftell` less often if a large data span is written. In a follow-up, changes can be made to call it less often when small data spans are written.

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1642400630)
> I don't find any merge conflicts with current master.

Are you sure you ran a full build after the merge?

💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268324286)
happy to review if you do

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268324570)
Thanks, just fixed & pushed

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1268326494)
ah of course thanks, fixed and pushed

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1642405661)
Ah, the second try did it :)

<details><summary>build log</summary><p>

```
test/fuzz/process_message_e2e.cpp:59:1: error: a type specifier is required for all declarations
FUZZ_TARGET_INIT(process_message_e2e, initialize_process_message_e2e)
^
test/fuzz/process_message_e2e.cpp:59:18: error: use of undeclared identifier 'process_message_e2e'
FUZZ_TARGET_INIT(process_message_e2e, initialize_process_message_e2e)
^
test/fuzz/process_message_e2e.cpp:59:70: error: expected
...

💬 Crypt-iQ commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#discussion_r1268330073)
nice catch!

💬 Crypt-iQ commented on pull request "http: add evhttp_connection_set_closecb to avoid g_requests hang":
(https://github.com/bitcoin/bitcoin/pull/27909#issuecomment-1642413131)
> This assumes that a connection can never have more than 1 request, and I'm not sure if that's the case.

I'm also not sure. This PR body leads me to believe that it is possible to have more than 1 request per connection: https://github.com/libevent/libevent/pull/592#issue-292981989 , however I'm not sure whether it's possible for them to be interleaved and cause problems with your connection-based patch. Something I'll try to investigate.

💬 pinheadmz commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1268398896)
🤦 ty sir. fixed & pushed

💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1268442591)
Good idea! Done.

💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1268442671)
Done, changed from implicit in the declarations to explicit in the definitions.