💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1438864128)
Nit: In commit 8a164cf087c480e68e9ec87a97f759b97015efea: I feel like this would be a bit more readable as a `CBlockIndex&`.

💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1439029163)
Nit: In commit c60c735320694d124212a41e7e7a9d204ff35a6f: Could add a `block.data` assertion at the top of the method?

💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1439064265)
In commit 1664a63578941bafaa36d42f438e2e5cdac96583: Looking at this function, I am not sure why it is in the kernel namespace. More generally, I am confused by the responsibilities of this file in the first place. Seems like it could just be split up into `chain.cpp` and `node/interfaces.cpp`? Is it weird that a kernel file includes `interfaces/chain.h`? That seems like a higher order include to me that the kernel should not need to know of.

💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1435719406)
NIt: In commit f2a0551eb8ec8e115778dde5b833a616461da9a4: Would it make sense to add an `assert(block.undo_data)` here? I think that could make the code here a bit clearer.

💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1439030135)
Just a question: Why call `get()` here? Is there a difference to `return !m_index.m_notifications`?

💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1439019275)
Nit: Might `prepare_sync` (or something similar) be a better name for this lambda? The way it is named I would expect this to be the function that triggers the syncing to start, but that only happens once `StartBackgroundSync` is called.

💬 TheCharlatan commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1439361568)
In commit 1664a63578941bafaa36d42f438e2e5cdac96583: The return type of `SyncChain` is unused. Are there future plans to use it, or could it be made `void`?

💬 hebasto commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440313980)
Here are excerpts from `config.log`:
- Apple clang version 14.0.3 (clang-1403.0.22.14.1):
```
configure:23855: checking whether C++ preprocessor accepts -Xclang -internal-isystem/usr/local/include
configure:23875: g++ -std=c++20 -E -Werror -Xclang -internal-isystem/usr/local/include conftest.cpp
configure:23875: $? = 0
configure:23885: result: yes
```
- Apple clang version 15.0.0 (clang-1500.0.40.1):
```
configure:23855: checking whether C++ preprocessor accepts -Xclang -internal-isys
...

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1440318695)
Felt more natural to give a `CMainSignals` ownership of `MainSignalsImpl` instead of a `std::unique_ptr<MainSignalsImpl`, so it needs the full declaration of the class.

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1440323203)
Mmh, the destruction of `wallet_loader` is a bit weird, but doesn't it get reset when `node.chain_clients.clear()` is called at `Shutdown(...)`?

💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1440329179)
ok, but in the commit 4b9a37e461aee0c24f798123cdb023a783a130c6 I was looking at, the unique_ptr remains unchanged

🚀 glozow merged a pull request: "test: doc: follow-up #28368"
(https://github.com/bitcoin/bitcoin/pull/29013)

✅ glozow closed an issue: "Test `policyestimator_tests/BlockPolicyEstimates` failure"
(https://github.com/bitcoin/bitcoin/issues/29000)

💬 darosior commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1440349190)
Ah, right, the ownership of the loader is in `node.chain_clients`:
https://github.com/bitcoin/bitcoin/blob/65c05db660b2ca1d0076b0d8573a6760b3228068/src/wallet/init.cpp#L129-L131

So it's dropped before `node.main_signals` is.

💬 maflcko commented on pull request "doc: update docs for `CHECK_ATOMIC` macro":
(https://github.com/bitcoin/bitcoin/pull/28777#discussion_r1440369992)
Same with clang-17. Steps to reproduce on a fresh install of Ubuntu 24.04:

```
CXXLD test/fuzz/fuzz
/usr/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `(anonymous namespace)::PeerManagerImpl::BlockConnected(ChainstateRole, std::shared_ptr<CBlock const> const&, CBlockIndex const*)':
net_processing.cpp:(.text._ZN12_GLOBAL__N_115PeerManagerImpl14BlockConnectedE14ChainstateRoleRKSt10shared_ptrIK6CBlockEPK11CBlockIndex[_ZN12_GLOBAL__N_115PeerManagerImpl14BlockC
...

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440370685)
Sure, but that happens today on current master, unrelated to the changes in this pull request. So it seems best to create a separate issue and fix for this.

Left a comment here: https://github.com/bitcoin/bitcoin/pull/28777/files#r1440369992

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440372713)
Again, this should affect everyone on macOS today already on current master, using clang-15 (or later), so a separate issue and bugfix can be filed?

👍 vasild approved a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1801881895)
ACK 6c603490023317a84c7c96ef4b64f4f96ea03d1f

It would be nice to avoid duplicating the constant `NODE_NETWORK_LIMITED_MIN_BLOCKS` ([discussion](https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1434938857)) but I do not see that as a blocker of this PR.

Thanks!

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1440386706)
No strong opinion. To me `net_processing.h` looks like the natural place to move/expose the constant(s) that were defined as `static` in `net_processing.cpp` if they need to be used outside of that `cpp` file. Do you somehow dislike that idea or have another one?

I dislike having them duplicated because then there is a chance that they will go out of sync. Also, in semantic completion tools one can point to a symbol and ask "show me all places where this is used". If what is used in the test
...

💬 fanquake commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440417362)
> Sure, but that happens today on current master, unrelated to the changes in this pull request.

No? The addition of -latomic here, is only needed with the changes in this PR. Otherwise CI on master would be failing.