💬 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.

💬 maflcko commented on issue "CI failure: `ThreadSanitizer: data race /usr/lib/llvm-12/bin/../include/c++/v1/ios:523:12 in std::__1::ios_base::width() const`":
(https://github.com/bitcoin/bitcoin/issues/23366#issuecomment-1875350212)
> To be able to remove the suppression, all these these cout writes would need to be protect with a lock. We can lock `G_TEST_LOG_FUN` ourselves since it is our own code. It looks like we could synchronize the boost cout output by redirecting it with `boost::unit_test::unit_test_log.set_stream( std::ostream& );` https://www.boost.org/doc/libs/1_77_0/libs/test/doc/html/boost_test/test_output/logging_api/log_ct_output_stream_redirection.html with a custom `std::ostream` subclass that locks a mutex
...

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 15":
(https://github.com/bitcoin/bitcoin/pull/29165#discussion_r1440439062)
Sorry, I meant fixing/changing the latomic build system check is unrelated, not the CI changes in this PR.

💬 stickies-v commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1440456087)
We already assume the median can be rounded down, so I don't think extra rounding changes any assumptions, and therefore I wouldn't overcomplicate things, it's just not worth it imo. I'd be okay with casting to a double first, but it's not strictly better as we're trading off precision at the higher ranges (which I also don't think is important at all).

```cpp
if (m_index % 2 == 0) {
return (0.5 * sorted_copy[m_index / 2]) +
(0.5 * sorted_copy[m_index
...

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1875443279)
This is relevant and I am rewriting it as per the discussions above.