💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1453524696)
nit: Any reason to call it "main", when main.cpp has long been split up and renamed to validation.cpp? Could call it `validation_signals`, or leave it as is.

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1453516899)
Wouldn't it be better to use existing `chainman` references to get the signals reference, instead of using unchecked pointer dereferences? (Same in all other places)

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1453413538)
nit in the first commit: Could drop `signals` and use `active_chainstate.GetMainSignals()` instead?

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1453405057)
first commit: Why check below, but not here, for nullptr?

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1453382756)
in the first commit: Any reason to re-order this and remove the doc comment?

💬 vasild commented on pull request "ci: run test_bitcoin with DEBUG_LOG_OUT in RUN_UNIT_TESTS_SEQUENTIAL":
(https://github.com/bitcoin/bitcoin/pull/28736#issuecomment-1893896190)
`2b7888ec3e...57e8bc6b31`: rebase due to conflicts

💬 furszy commented on issue "Use of a wallet shouldn't be blocked in prune mode ("wallet loading failed... beyond pruned data")":
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1893908170)
@shovas, I have created #29183 to focus our efforts and delineate the working path in the clearest possible manner. It should help pushing the project forward.

💬 vasild commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1893912633)
@achow101, this is now solely in the wallet. Maybe you want to take a look?

👍 TheCharlatan approved a pull request: "depends: add NM output to gen_id"
(https://github.com/bitcoin/bitcoin/pull/29249#pullrequestreview-1823792580)
ACK a1db512f069039530b9fb70f8c004a4f25c3e257

Guix build (x86)
```
580df4b97c2e045d93e98c2bf2fbccad05117aa54594ec8849969ae18dd0d865 guix-build-a1db512f0690/output/aarch64-linux-gnu/SHA256SUMS.part
d8ee47f067d42f087f1c2f01d0613d57c3f887eb2be746498b674b27671c2678 guix-build-a1db512f0690/output/aarch64-linux-gnu/bitcoin-a1db512f0690-aarch64-linux-gnu-debug.tar.gz
77655b1a8946a131c5bd0c5083d9faeae83d08d9d718ac43a79d1872c98a27e0 guix-build-a1db512f0690/output/aarch64-linux-gnu/bitcoin-a1db51
...

💬 fanquake commented on pull request "build: depends move macOS C(XX) FLAGS out of C & CXX":
(https://github.com/bitcoin/bitcoin/pull/29233#issuecomment-1893982810)
Guix Build (aarch64):
```bash
94d2fe95101baf9ba74e5c1be6a93708bd55b3700567c768e72ceb4af4951bb9 guix-build-cbc9bf11fe84/output/aarch64-linux-gnu/SHA256SUMS.part
3cacc8c3d726e78b1db8ea07c25186e589f32c991cd7fd45d4390135adf199f2 guix-build-cbc9bf11fe84/output/aarch64-linux-gnu/bitcoin-cbc9bf11fe84-aarch64-linux-gnu-debug.tar.gz
39f42920ea452ea67f0f8df3199c50d73660009368fccfab900204d27b8e50cc guix-build-cbc9bf11fe84/output/aarch64-linux-gnu/bitcoin-cbc9bf11fe84-aarch64-linux-gnu.tar.gz
dc1a2b
...

🚀 fanquake merged a pull request: "contrib: add macho branch protection check"
(https://github.com/bitcoin/bitcoin/pull/29170)

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-1894004822)
Updated 67058d4a82e222f770ede96dc948f758b0a8386c -> 2cb38d93a8fea16bf84b072a90ac023ef345134d ([kernelRmKey_0](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_0) -> [kernelRmKey_1](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmKey_0..kernelRmKey_1))

* Implemented @maflcko's [suggestion](https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-1893335214), calling `ECC_Stop` instead of relaxing the `ECC_
...

💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453627000)
Removing this will lead to compile errors when a `unique_ptr` member is added with a fwd decl, no?

💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453627918)
need to remove include as well, no?

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453636097)
Yes.

💬 Sjors commented on pull request "build: depends move macOS C(XX) FLAGS out of C & CXX":
(https://github.com/bitcoin/bitcoin/pull/29233#issuecomment-1894038057)
I was able to make and run a depends build on Intel macOS 14.2.1 for cbc9bf11fe84deb96daf9b97a8e7499979360db2.

I'm also baking a Guix build.

💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453639815)
Seems better to leave as-is, or remove the comment, if it is no longer accurate?

💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-1894042338)
lgtm ACK 2cb38d93a8fea16bf84b072a90ac023ef345134d 🍂

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 2cb38d93a8fea16b
...

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453643762)
Will leave as is then. Seems likely that something will end up here.

👍 fanquake approved a pull request: "refactor: C++20: Use std::rotl"
(https://github.com/bitcoin/bitcoin/pull/29085#pullrequestreview-1824029448)
ACK 60446285436da62adef1c0a9b11c3336d82b4d89

Had a look at the difference in master vs this PR (rebased) on aarch64. Seems like for the most part, this gives the same output +- a handful of instructions. (Identical is identical minus difference in addresses / argument ordering etc).

| Function | GCC 13.2 | Clang 17.0.6 |
| --- | --- | --- |
| MurmurHash3 | Identical | PR gives 5 less instructions: https://www.diffchecker.com/Jmlpl4rG/ |
| ChaCha20Aligned::Keystream | PR gives 1 less in
...