💬 dergoegge commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#discussion_r1440301216)
Afaict oss-fuzz already uses a ram disk for its environments: https://github.com/google/clusterfuzz/blob/c461a961d8fb2afe47fb4af5eee3d1434a324a40/docker/base/setup_clusterfuzz.sh#L38 (i.e. `/tmp` is mounted in ram).

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1875164441)
> This kind of thinking is precisely the problem: you've listed a bunch of potential problems, without any discussion of how actual protocols are impacted by those problems.

Peter, please read the [RBF improvements mailing list thread](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html) to get up to date on users' problems with RBF.

Yes, v3 makes no sense if you think RBF is perfect today. I agree that we cannot have a productive discussion about how to solve
...

💬 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_r1430067474)
Nit: `s/best block not the same/best block are not the same/`

🤔 TheCharlatan reviewed a pull request: "indexes: Stop using node internal types and locking cs_main, improve sync logic"
(https://github.com/bitcoin/bitcoin/pull/24230#pullrequestreview-1786770579)
Approach ACK

This change looks good, albeit its daunting length. The only thing I am still not sure about is the commit "indexes: Initialize indexes without holding cs_main" c8fde60faad36d454cdd1300184a5169499f9e90 and if there is some observable improvement. Maybe this commit could be left for another PR? Or does it achieve something that is strictly needed for this PR?

The amount of churn in the commits is a bit unfortunate, but I don't have suggestions on how to improve this. Some of th
...

💬 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_r1435025414)
Nit: In commit 8d0cc07587d4acfab3a8b4f636b631467c8e5f19 why is this assertion not moved as well to `blockConnected`? Similarly the assertion in [here](https://github.com/bitcoin/bitcoin/pull/24230/commits/8d0cc07587d4acfab3a8b4f636b631467c8e5f19#diff-5251d93616c116c364d2d502ad2a59e901a3512194462fabacc9756f96fd8dddR394) in `IgnoreChainstateFlushed`.

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