💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267253028)
Would it make any sense here to manually set `peer.m_ping_queued = true` to guarantee (not "maybe") sending ping?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267253028)
Would it make any sense here to manually set `peer.m_ping_queued = true` to guarantee (not "maybe") sending ping?
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267244372)
IIUC this line schedules a replacement sensitive connection because the one we were passed in has the wrong services and so it gets dumped. I think this is the only line in this function that makes it apply strictly to only sensitive relays. So I wonder if that should be asserted at the top? Or could be renamed to `PushUnbroadcastTxToSensistiveRelay()`? At the very least maybe sensitive-only should be explained in the function description on L933 ?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267244372)
IIUC this line schedules a replacement sensitive connection because the one we were passed in has the wrong services and so it gets dumped. I think this is the only line in this function that makes it apply strictly to only sensitive relays. So I wonder if that should be asserted at the top? Or could be renamed to `PushUnbroadcastTxToSensistiveRelay()`? At the very least maybe sensitive-only should be explained in the function description on L933 ?
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267277048)
Does this guy need an `else`? Maybe raise error that a connection request was made after all destinations have been used?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267277048)
Does this guy need an `else`? Maybe raise error that a connection request was made after all destinations have been used?
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267279962)
Why did you need this to support wtxid? Is that how we are verifying our TX made the round trip out and back (without being malleated)?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267279962)
Why did you need this to support wtxid? Is that how we are verifying our TX made the round trip out and back (without being malleated)?
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267248349)
wow, is this really the fastest way to get a random element from `std::set` ? funny.
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267248349)
wow, is this really the fastest way to get a random element from `std::set` ? funny.
💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267276546)
I don't see `.node` used below and I don't think you really use `.requested_to_addr` anywhere after setting it below? Could this just be an array of `CAddress` or something like that?
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1267276546)
I don't see `.node` used below and I don't think you really use `.requested_to_addr` anywhere after setting it below? Could this just be an array of `CAddress` or something like that?
💬 stickies-v 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_r1267282655)
nit: not sure the snake case is worth the added LoC (+further down). Would prefer to keep as is.
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267282655)
nit: not sure the snake case is worth the added LoC (+further down). Would prefer to keep as is.
💬 stickies-v 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_r1267277566)
nit: no need to specify where it's used for a public method imo
```suggestion
* Whether this object is a privacy network.
```
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267277566)
nit: no need to specify where it's used for a public method imo
```suggestion
* Whether this object is a privacy network.
```
💬 stickies-v 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_r1267281474)
nit: any reason the order of these lines got switched? if not necessary, wouldn't do that
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267281474)
nit: any reason the order of these lines got switched? if not necessary, wouldn't do that
💬 stickies-v 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_r1267279181)
Absolutely, thanks. Just left some small comments.
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267279181)
Absolutely, thanks. Just left some small comments.
💬 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_r1267286727)
I think it's important to provide the context here.
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267286727)
I think it's important to provide the context here.
💬 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_r1267289243)
Alphabetical order, first use order, and consistency (see the last use).
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267289243)
Alphabetical order, first use order, and consistency (see the last use).
💬 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_r1267291414)
> As we're now touching all these LoC, would suggest to clean up even further. Slightly increases the diff but worth it for readability imo.
Seems consistent with this and the improved naming in that change.
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1267291414)
> As we're now touching all these LoC, would suggest to clean up even further. Slightly increases the diff but worth it for readability imo.
Seems consistent with this and the improved naming in that change.
💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1267298480)
Done in #28100.
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1267298480)
Done in #28100.
💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1267298613)
Done in #28100.
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1267298613)
Done in #28100.
💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1267298759)
Done in #28100.
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1267298759)
Done in #28100.
💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1267298869)
Done in #28100.
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1267298869)
Done in #28100.
💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1267299659)
I haven't run benchmarks against statically allocating a buffer, but it may be useful since `AutoFile::write` is called many times per block. Basically every time the pattern `CAutoFile << x` is used which happens for every element of the block header, each transaction input, output, etc. I also ran a small unit test through `perf` where there were many writes and found that `std::ftell` dominated with `std::fwrite` close behind.
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1267299659)
I haven't run benchmarks against statically allocating a buffer, but it may be useful since `AutoFile::write` is called many times per block. Basically every time the pattern `CAutoFile << x` is used which happens for every element of the block header, each transaction input, output, etc. I also ran a small unit test through `perf` where there were many writes and found that `std::ftell` dominated with `std::fwrite` close behind.
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1267302191)
It may be good to have a heat-map (Percent of time spent in each line) for this function for IBD, or a unit test that writes a block.
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1267302191)
It may be good to have a heat-map (Percent of time spent in each line) for this function for IBD, or a unit test that writes a block.
💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1267304093)
I _think_ you can move this `std::ftell` to right before the loop and then use the result from `std::fwrite` to increment `current_pos` each loop iteration since from https://en.cppreference.com/w/cpp/io/c/fwrite: "The file position indicator for the stream is advanced by the number of characters written.". I ran some tests and it gives a performance improvement if `src.size()` is ever > 4096 bytes (though I'm not sure that is ever the case currently with block serialization hitting this functi
...
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1267304093)
I _think_ you can move this `std::ftell` to right before the loop and then use the result from `std::fwrite` to increment `current_pos` each loop iteration since from https://en.cppreference.com/w/cpp/io/c/fwrite: "The file position indicator for the stream is advanced by the number of characters written.". I ran some tests and it gives a performance improvement if `src.size()` is ever > 4096 bytes (though I'm not sure that is ever the case currently with block serialization hitting this functi
...