💬 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
...
💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1641021202)
> unbootstrappable
Simply claiming this doesn't seem helpful. What is the compile error you encountered?
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1641021202)
> unbootstrappable
Simply claiming this doesn't seem helpful. What is the compile error you encountered?
💬 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#issuecomment-1641078447)
> I thought we were going to avoid unproductive refactors?
I'm unaware of the context behind that comment but see similar refactoring in many pull requests and, in general, there are good reasons for them; in this case, removing unneeded exports and simplifying and removing code. (If you'd like to review a pull that fixes something, may I re-review beg for https://github.com/bitcoin/bitcoin/pull/27231 that was updated to take your feedback? Thanks!)
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1641078447)
> I thought we were going to avoid unproductive refactors?
I'm unaware of the context behind that comment but see similar refactoring in many pull requests and, in general, there are good reasons for them; in this case, removing unneeded exports and simplifying and removing code. (If you'd like to review a pull that fixes something, may I re-review beg for https://github.com/bitcoin/bitcoin/pull/27231 that was updated to take your feedback? Thanks!)