💬 l0rinc commented on pull request "optimization: speed up block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r1956688357)
Absolutely, I already have other optimization ideas in mind after that's merged.

The reviewers can decide the preferred merge order, I don't mind rebasing or doing it in multiple PRs - there's a lot of work left with serialization anyway.

💬 davidgumberg commented on pull request "doc: build: Fix instructions for msvc gui builds":
(https://github.com/bitcoin/bitcoin/pull/31851#issuecomment-2660223577)
> ACK [c3fa043](https://github.com/bitcoin/bitcoin/commit/c3fa043ae560289759b6f836ac62736d97ba91bf)
>
> Am I misremembering or was this, or a similar workaround, part of the previous build instructions at some point?

Yep, #29048 added a similar note to the old `build_msvc/README.md`, which had instructions for doing a static build of Qt, but I guess during the cmake transition, there was a switch from manually building Qt to using vcpkg, but sadly, the same problem still exists.

💬 theuni commented on pull request "cmake: add optional source files to bitcoin_crypto and crc32c directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2660224380)
Agree with @hebasto that it'd be more straightforward to use `target_compile_options()`, if only for the sake of avoiding future footguns.

utACK otherwise.

💬 hodlinator commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1956695414)
Suggested by me here: https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1912232236
Maybe stating the obvious a bit too much for your taste, or how would it be incorrect?

💬 theuni commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31863#discussion_r1956695948)
Thanks @maflcko, I also agree.

💬 hodlinator commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1956696518)
> What does this mean?

My point is that it's not required on other operating systems.

👍 theuni approved a pull request: "random: Initialize variables in hardware RNG functions"
(https://github.com/bitcoin/bitcoin/pull/31863#pullrequestreview-2618764775)
utACk 99755e04ffadd5daad53c686f4f0feee2232eeb2

💬 theuni commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#issuecomment-2660231472)
> > gcc (since v10) and clang (since v13) have intrinsics for __rndr and __rndrrs that would imo be preferable.
>
> Yes. But that's imo out of scope for this workaround PR.

For sure out of scope, yes, I didn't mean to imply that should be done here.

💬 sipa commented on pull request "random: Initialize variables in hardware RNG functions":
(https://github.com/bitcoin/bitcoin/pull/31863#issuecomment-2660238149)
utACK 99755e04ffadd5daad53c686f4f0feee2232eeb2

👍 theuni approved a pull request: "cmake: Add `libbitcoinkernel` target"
(https://github.com/bitcoin/bitcoin/pull/31869#pullrequestreview-2618778587)
ACK 3a914ab96bdeb70cdf848dd74deda5a80d0a7f78

Tested working as intended. Building/installing both targets works identically, no unnecessary rebuilds or installs either way.

Thanks hebasto!

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2660261042)
Opened https://github.com/bitcoin/bips/pull/1768 and putting this in draft for now.

📝 jonatack converted_to_draft a pull request: "doc: improve NODE_NETWORK_LIMITED documentation per BIP159"
(https://github.com/bitcoin/bitcoin/pull/31805)
Per BIP159, the definition is:

NODE_NETWORK_LIMITED || bit 10 (0x400) || If signaled, the peer <I>MUST</I> be capable of serving at least the last 288 blocks (~2 days).

Fix our documentation related to this, and add BIP159 mentions where relevant.

---

Originally this PR also tried to clarify the following, but it was dropped in response to review:

`NODE_NETWORK_LIMITED` is set by default, and it only signals a pruned or otherwise limited node if `NODE_NETWORK` is not also set. See
...

👋 l0rinc's pull request is ready for review: "optimization: speed up `CheckBlock` input checks (duplicate detection & nulls)"
(https://github.com/bitcoin/bitcoin/pull/31682)

🚀 achow101 merged a pull request: "rpc: Remove deprecated dummy alias for listtransactions::label"
(https://github.com/bitcoin/bitcoin/pull/31413)

💬 TheCharlatan commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956747029)
I've been thinking about allocating it on the heap instead, but I kind of like that declaring it as the first field forces us to initialize it first, which a pointer type would not necessarily. Then again, moving the lock as a unique pointer from the options to the actual class would also probably be easier than defining a move constructor the directory lock class.

💬 achow101 commented on pull request "validation: In case of a continued reindex, only activate chain in the end":
(https://github.com/bitcoin/bitcoin/pull/31439#issuecomment-2660323933)
ACK c9136ca90605bbe29f005f538b92ff96ca360a13

🚀 achow101 merged a pull request: "validation: In case of a continued reindex, only activate chain in the end"
(https://github.com/bitcoin/bitcoin/pull/31439)

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956712751)
This seems very cautious. The test first adds 150 txns, then another `DEFAULT_MAX_ORPHAN_ANNOUNCEMENTS` (so there is a 1 to 1 relation between announcements and txns).
If all txns are distinct, this should be enough to assert that `Size()` should shrink by 150 compared to prev_count, not just 1.

💬 mzumsande commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956737572)
the numbers don't add up for me: the tests creates 100 large orphans, sends 20, sends 1 small tx, then sends another 40 large orphans, and finally asserts that there are less than 101 entries in the orphanage. This would also be true without any eviction.

💬 achow101 commented on pull request "wallet: abandon orphan coinbase txs, and their descendants, during startup":
(https://github.com/bitcoin/bitcoin/pull/31794#issuecomment-2660362190)
ACK 3c89cab06a6259fa70eaf3a78c01ee42780c4e27