💬 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
...
🤔 luke-jr requested changes to a pull request: "util: Replace std::filesystem with util/fs.h"
(https://github.com/bitcoin/bitcoin/pull/28076#pullrequestreview-1535903008)
Standing Concept NACK to Rust while it has security issues (unbootstrappable)
"It's just for CI" - but we shouldn't be relying on CI alone.
(https://github.com/bitcoin/bitcoin/pull/28076#pullrequestreview-1535903008)
Standing Concept NACK to Rust while it has security issues (unbootstrappable)
"It's just for CI" - but we shouldn't be relying on CI alone.
💬 luke-jr commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1640997776)
I thought we were going to avoid unproductive refactors?
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1640997776)
I thought we were going to avoid unproductive refactors?
💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#issuecomment-1641009949)
Putting this back to draft until I've invested the time to properly address @ryanofsky's comments.
(https://github.com/bitcoin/bitcoin/pull/27866#issuecomment-1641009949)
Putting this back to draft until I've invested the time to properly address @ryanofsky's comments.
📝 TheCharlatan converted_to_draft a pull request: "blockstorage: Return on fatal flush errors"
(https://github.com/bitcoin/bitcoin/pull/27866)
The goal of this PR is to establish that fatal blockstorage flush errors should be treated as errors at their call site.
Prior to this patch `FlushBlockFile` may have failed without returning in `Chainstate::FlushStateToDisk`, leading to a potential write from `WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet. By returning if either `FlushUndoFile` or `FlushBlockFile` fail, we ensure that no further write operations take place that may lead to an inconsistent
...
(https://github.com/bitcoin/bitcoin/pull/27866)
The goal of this PR is to establish that fatal blockstorage flush errors should be treated as errors at their call site.
Prior to this patch `FlushBlockFile` may have failed without returning in `Chainstate::FlushStateToDisk`, leading to a potential write from `WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet. By returning if either `FlushUndoFile` or `FlushBlockFile` fail, we ensure that no further write operations take place that may lead to an inconsistent
...