💬 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).

💬 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.

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(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.

💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(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.

💬 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.

💬 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.

💬 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
...

🤔 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.

💬 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?

💬 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.

📝 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
...

💬 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?

💬 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!)

💬 real-or-random commented on issue "ci: Future of macOS and Windows MSVC CI tasks":
(https://github.com/bitcoin/bitcoin/issues/28098#issuecomment-1641104328)
> Someone sets up Windows+macOS VMs and connects them to the bitcoin and bitcoin-core Cirrus CI account as a persistent worker. (I've only tested this with Linux, so no idea if it works)

If we can make this happen, this will probably be ideal in the short term. (Heck, even if we convince a sponsor to hire someone, this is much cheaper than paying for compute credits...)

> Last time we looked it required write access,

See the discussion here: https://github.com/bitcoin/bitcoin/issues/178
...

👋 luke-jr's pull request is ready for review: "During IBD, prune as much as possible until we get close to where we will eventually keep blocks"
(https://github.com/bitcoin/bitcoin/pull/20827)

💬 luke-jr commented on pull request "During IBD, prune as much as possible until we get close to where we will eventually keep blocks":
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1641248977)
Rebased

💬 jonatack commented on pull request "During IBD, prune as much as possible until we get close to where we will eventually keep blocks":
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1641253768)
Concept ACK

📝 kevkevinpal opened a pull request: "Changing -torcontrol help to specify that a default port is used"
(https://github.com/bitcoin/bitcoin/pull/28101)
Right now when we get the help for -torcontrol it says that there is a default ip and port we dont specify if there is a specified ip that we would also use port 9051 as default

Also I create a new const instead of using 9051 directly in the function

linking this PR because this was discussed here https://github.com/bitcoin/bitcoin/pull/28018

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
i
...