💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1539999879)
The first commit tends to confuses me, so I'll just note a clarification here. I'm not fully convinced that blocks near the tip can't end up getting processed by the background chainstate. And if they do, I'm not sure if that's bad.

When a block comes in near the tip, we know the header so `LookupBlockIndex` will return a `pblock`. This header is _not_ contained in `m_chain`, because headers are only added to `m_chain` by `ConnectTip()`. So a potential tip block is sent to the snapshot chains
...

💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188458662)
426ebe830b3bb076182505b60233e8c8f6d35069: the `m_snapshot_chainstate` check is now redundant: https://github.com/bitcoin/bitcoin/pull/24008/commits/426ebe830b3bb076182505b60233e8c8f6d35069#r1171549876

💬 MarcoFalke commented on pull request "refactor: Remove unused GetTimeMillis":
(https://github.com/bitcoin/bitcoin/pull/27594#issuecomment-1540006864)
> Curious if you have any plans for GetTime()

Yeah, I guess it can be removed as well, assuming there are no merge conflicts with other open pulls?

💬 Sjors commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1540014195)
@ArmchairCryptologist do you have `peerbloomfilters` on?

💬 ArmchairCryptologist commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1540022998)
> @ArmchairCryptologist do you have peerbloomfilters on?

Nope. The nodes are pretty much running with the default settings except for maxconnections.

💬 instagibbs commented on issue "Parallel compact block download":
(https://github.com/bitcoin/bitcoin/issues/25258#issuecomment-1540028533)
This also seems to happen with spy(?) nodes which are the first to send a header, but then never end up responding to requests for the full block. Due to recent mempool chop, the chances of requiring a round-trip for slightly-later-but-behaving high bandwidth peer's cb goes way up, and currently cannot be accommodated.

couple of questions to think about:
1) should the 2nd download only work for outbound connections? currently there are 3 hb peers possible, and only one outbound is "protected
...

🤔 Sjors reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1418514150)
26cb32c02d76d6635c67942a5eeb70a6199df69d is missing a comment for the use of `ActiveChainstate` in `ProcessGetBlockData` and after receiving a `GETBLOCKS` message.

💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188522463)
c7cdc7ca833ce1b5ce9a696fb4ef633c55528023: this line does more than "add commentary". Maybe move this to 3a7a714721a593e271ce70d4612c995968dbb0da or its own commit directly after that one?

The code comment could clarify why we care about this even for the background sync: "with insufficient work" -> "with less than the minimum chain work."

💬 sdaftuar commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#issuecomment-1540089055)
> @sdaftuar did you have Current Thoughts about this approach? Trying to bridge IRC discussions to the blocking PR

I was trying to come up with my own proof-of-concept to demonstrate an alternate approach before commenting, but just to get everyone up to speed on what I'm thinking:

* The first commit "validation: introduce ChainstateManager::GetChainstateForNewBlock" seems like something we don't conceptually need. I think the underlying issue is that `AcceptBlock` should be chainstate-agn
...

💬 hebasto commented on pull request "refactor: Replace global find_value function with UniValue::find_value method":
(https://github.com/bitcoin/bitcoin/pull/27605#issuecomment-1540094937)
> > Ubuntu 23.04 using GCC 13.0.1
>
> I think it ships with 12.2? [packages.ubuntu.com/lunar/gcc](https://packages.ubuntu.com/lunar/gcc)

https://packages.ubuntu.com/lunar/g++-13 has been installed as `sudo apt install g++13`.

👍 ryanofsky approved a pull request: "refactor: Move chain constants to the util library"
(https://github.com/bitcoin/bitcoin/pull/27491#pullrequestreview-1418615677)
Code review ACK 1ae4d9a37b46a5e69ed79066408a6002c2cc0f73, but I would suggest another change to make second and third commits more reviewable (see below).

re: https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1539094705

> re-ordering the commits and declaring the new ChainType getters before using them in the following commits

Thanks for the update, but this actually does the opposite of what I was trying to suggest. I'm trying move substantive code changes *out* of the giant 3
...

🤔 glozow reviewed a pull request: "add ryanofsky to trusted-keys"
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1418615869)
ACK 59ebee3fb4181baf20fab263cf1b587ece1bd5e2

I feel that we need an "interfaces" maintainer with a deep understanding of our codebase's architecture (how it is and how it should be), particularly as we are working on several large projects spanning multiple subsystems and figuring out how to separate them. I believe @ryanofksy is this person, and generally a natural fit for having merge access because he is one of the most consistent and thorough reviewers on the project.

This key fingerpr
...

💬 fanquake commented on pull request "Introduce platform-agnostic `ALWAYS_INLINE` macro":
(https://github.com/bitcoin/bitcoin/pull/27575#issuecomment-1540131935)
Guix Build:
```bash
4c75c6675a758f8bfddaf3ca395eec58f82078486273bd13c7f03fa949f05f91 guix-build-3f19875d6675/output/aarch64-linux-gnu/SHA256SUMS.part
9736762959e2d9d06c33093ff08db157548fd3f82529cfbace0ceddce0151996 guix-build-3f19875d6675/output/aarch64-linux-gnu/bitcoin-3f19875d6675-aarch64-linux-gnu-debug.tar.gz
a82ceeae1e2b98e65bc363f61648a28e7a0985a2d2e1acc7142c3540ec71e5c4 guix-build-3f19875d6675/output/aarch64-linux-gnu/bitcoin-3f19875d6675-aarch64-linux-gnu.tar.gz
16b8bf25ad364757
...

👍 fanquake approved a pull request: "Introduce platform-agnostic `ALWAYS_INLINE` macro"
(https://github.com/bitcoin/bitcoin/pull/27575#pullrequestreview-1418675956)
ACK 3f19875d667522412408d06873e87ff8150e49c4

🚀 fanquake merged a pull request: "Introduce platform-agnostic `ALWAYS_INLINE` macro"
(https://github.com/bitcoin/bitcoin/pull/27575)

🚀 fanquake merged a pull request: "qt: 25.0rc2 translations update"
(https://github.com/bitcoin/bitcoin/pull/27517)

👍 pinheadmz approved a pull request: "add ryanofsky to trusted-keys"
(https://github.com/bitcoin/bitcoin/pull/27604#pullrequestreview-1418692620)
ACK 59ebee3fb4181baf20fab263cf1b587ece1bd5e2

I verified the key fingerprint in meatspace with the human that I believe is, in fact, @ryanofsky. Confirmed fingerprint matches key that signed this commit.

ryanofsky will be a great maintainer, his reviews are excellent even on PRs that he doesn't agree with.

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 59ebee3fb4181baf20fab263cf1b587ece1bd5e2
-----BEGIN PGP SIGNATURE-----

iQ
...

💬 TheCharlatan commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1540196087)
Thank you for your patience @ryanofsky

Updated 1ae4d9a37b46a5e69ed79066408a6002c2cc0f73 -> d168458d1ff987e0d741c75ac1d4b63ae0cfb7e7 ([kernelChainType_6](https://github.com/TheCharlatan/bitcoin/tree/kernelChainType_6) -> [kernelChainType_7](https://github.com/TheCharlatan/bitcoin/tree/kernelChainType_7), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelChainType_6..kernelChainType_7))

* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27491#pullreques
...

🤔 Sjors reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1418631351)
Other than my remarks I'm happy with c7cdc7ca833ce1b5ce9a696fb4ef633c55528023.

💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1188597311)
8e1d9a4bedc73b1ce2124a8583df8ed78d0c48ed: nit `>=` seems more intuitive, so the else case can't be 0.