🚀 ryanofsky merged a pull request: "Fix issues with CI on forks"
(https://github.com/bitcoin/bitcoin/pull/29274)

💬 ryanofsky commented on pull request "kernel, refactor: return error status on all fatal errors":
(https://github.com/bitcoin/bitcoin/pull/29700#issuecomment-2221105889)
Rebased 692fc11f0aae3c8013f6f01d133139ce8eba7135 -> ad1e73590d102edf3738986c1382b5e36a0ed727 ([`pr/fatalresult.17`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.17) -> [`pr/fatalresult.18`](https://github.com/ryanofsky/bitcoin/commits/pr/fatalresult.18), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/fatalresult.17-rebase..pr/fatalresult.18)) to fix conflicts

💬 hebasto commented on pull request "Bugfix: Correct first-run free space checks":
(https://github.com/bitcoin/bitcoin/pull/29678#issuecomment-2221122443)
Indeed, there is inconsistency in GB/GiB unit usage.

For instance, the value of the `m_assumed_blockchain_size` variable in GiB is used with the "GB" units in the user-faced messages here: https://github.com/bitcoin/bitcoin/blob/45f757c72672fc351bf669e731744f2f5e269233/src/init.cpp#L1707 and here: https://github.com/bitcoin/bitcoin/blob/45f757c72672fc351bf669e731744f2f5e269233/src/qt/forms/intro.ui#L206

Concept ACK.

---

It would be nice to mention the second commit changes in the PR
...

💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#discussion_r1672758736)
oh whoops, thanks. Should I move it now?

🤔 mzumsande reviewed a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394#pullrequestreview-2169946051)
ACK 16bd283b3ad05daa41259a062aee0fc05b463fa6

> Our own test framework was behaving that way before #29353, so maybe the functional tests would have caught it? I thought about reverting [c340503](https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3) locally to find out, but didn't have the time yet.

FYI, I tested reverting c340503b67585b631909584f1acaa327f5af25d3 on top of this branch now and without the latest fix (2) to ProcessMessages, other messages are act
...

🤔 paplorinc requested changes to a pull request: "crypto, refactor: add new KeyPair class"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2169947102)
Not yet sure about adding stuff that "will hopefully used this way one day".

Refactoring is welcome, as long as it's easy to review, which is not yet the case here, could you split the change into trivial, focused commits?

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1672775882)
Moving and modifying the code in the same commit (while many of the changes serve different purposes, mixing low and high-risk changes) makes the review a lot harder since the diff isn't really useful – having separate red and green parts, not really a diff.

Additionally, the commit doesn't explain each change separately. For example:
* The exact reason for memory management changes (via `make_secure_unique`) is not explained, nor are the security implications or potential performance impact
...

👍 TheCharlatan approved a pull request: "test: [refactor] Pass TestOpts"
(https://github.com/bitcoin/bitcoin/pull/30407#pullrequestreview-2169975415)
Nice, ACK fa690c8e532672f7ab53be6f7a0bb3070858745e

📝 TheCharlatan opened a pull request: "kernel: De-globalize static validation variables"
(https://github.com/bitcoin/bitcoin/pull/30425)
In future, users of the kernel library might run multiple chainstates in parallel, or create and destroy multiple chainstates over the lifetime of a process. Having static, mutable variables could lead to state inconsistencies in these scenarios.

💬 achow101 commented on pull request "Assumeutxo: bugfix on loadtxoutset with a divergent chain + test":
(https://github.com/bitcoin/bitcoin/pull/29996#issuecomment-2221235072)
ACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c

💬 achow101 commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#issuecomment-2221253695)
ACK 8789dc8f315a9d9ad7142d831bc9412f780248e7

🚀 achow101 merged a pull request: "Assumeutxo: bugfix on loadtxoutset with a divergent chain + test"
(https://github.com/bitcoin/bitcoin/pull/29996)

🚀 achow101 merged a pull request: "prune, rpc: Check undo data when finding pruneheight"
(https://github.com/bitcoin/bitcoin/pull/29668)

👍 ryanofsky approved a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2169988165)
Code review ACK 0456828a50b86fd9185b7428e9012b7c18e970f9. Left some suggestions, but they are not important

💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1672799422)
In commit "logging: Add DisableLogging()" (ce34ad42c7c19edf726a31551ec3780733147db9)

Could there be a comment here explaining why logging is disabled here? Is it needed because this bitcoin-chainstate is not calling StartLogging(), so the log buffer could fill up?

💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1672814012)
In commit "logging: Limit early logging buffer" (3d4b1f4af406276671b5d0115574dea6b4a64340)

It seems like a mistake to drop this as it is not being set otherwise.

💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1672831214)
In commit "logging: Apply formatting to early log messages" (0456828a50b86fd9185b7428e9012b7c18e970f9)

Maybe something to consider for a followup but some of the arguments to FormatLogStrInPlace and the two LogPrintStr functions could be string_views instead of strings (logging_function, source_file, threadname).

💬 ryanofsky commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1672811803)
In commit "logging: Limit early logging buffer" (3d4b1f4af406276671b5d0115574dea6b4a64340)

Since the log limit discards old log prints instead of skipping new prints, I think it would be a little clearer if "skipped" was replaced with "discarded" throughout this commit.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1672873185)
> I expect that I will have to review and ack your changes many more times before it's merged

I certainly hope that isn't the case :sweat_smile:

💬 brunoerg commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#discussion_r1673007264)
```suggestion
// Can have rawnode under tr()
```

💬 TheBlueMatt commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2221479047)
> Because not everyone needs to run these things but if they want to they can. Cramming everything into one software project is not sustainable.

I don't think you read what I wrote. I said "Obviously I'm not claiming all of these things should be in Bitcoin core, but there's some cost to them being outside". Which implies, obviously I agree with you that "cramming everything into one software project is not sustainable", but claiming that *the* reason people don't run these projects is becaus
...