📝 TheCharlatan opened a pull request: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382)
This simplifies the shutdown code a bit and allows for getting rid of the `goto` in `bitcoin-chainstate` as well as making it a bit easier to use for future users of the kernel library.

Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. Tweak the chainman tests to actually exercise this new tear down behaviour.

💬 maflcko commented on pull request "cmake: Check `-Wno-*` compiler options for `leveldb` target":
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503864322)
Personally I think it is fine append `-Wno...` in 5 lines of code. I'd presume filtering would require even more code?

💬 fanquake commented on pull request "cmake: Check `-Wno-*` compiler options for `leveldb` target":
(https://github.com/bitcoin/bitcoin/pull/31366#issuecomment-2503869092)
> I'd presume filtering would require even more code?

I don't see why it'd be more than 1 line.

💬 0xB10C commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860668423)
I'm using [rootless-docker](https://docs.docker.com/engine/security/rootless/) which runs dockerd as a user account. Inside the container, you're root and can tcpdump on the containers `eth0` interface, but you can't* e.g. mount and edit the hosts `/etc/passwd` like you can't with the user account.

*until someone finds a vuln in rootless-docker

📝 maflcko opened a pull request: "test: Add missing node.setmocktime(self.mocktime) to p2p_ibd_stalling.py"
(https://github.com/bitcoin/bitcoin/pull/31383)
This was forgotten by myself in commit fa5b58ea01fac1adb6336b8b6b5217193295c695.

This time, there is a diff to test, which fails on current master and passes with this pull request.

```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index e503a68382..16438ebd08 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -112,9 +112,9 @@ static_assert(MAX_BLOCKTXN_DEPTH <= MIN_BLOCKS_TO_KEEP, "MAX_BLOCKTXN_DEPTH too
* want to make this a per-peer adaptive
...

📝 TheCharlatan converted_to_draft a pull request: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382)
This simplifies the shutdown code a bit and allows for getting rid of the `goto` in `bitcoin-chainstate` as well as making it a bit easier to use for future users of the kernel library.

Moving the second flush out of the Shutdown function into the ChainstateManager destructor, should be safe since there should be no new events created after the first flush. Tweak the chainman tests to actually exercise this new tear down behaviour.

💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860714243)
It may be that docker rootless has a different capabilities set, compared to podman. (Can be checked with `capsh --print`).

In any case, my preference would be to explicitly list the required caps, instead of relying on a vendor default.

✅ l0rinc closed a pull request: "kernel, refactor: Replace `goto` with RAII in `bitcoin-chainstate`"
(https://github.com/bitcoin/bitcoin/pull/31362)

💬 l0rinc commented on pull request "kernel, refactor: Replace `goto` with RAII in `bitcoin-chainstate`":
(https://github.com/bitcoin/bitcoin/pull/31362#issuecomment-2503982559)
Closing in favor of https://github.com/bitcoin/bitcoin/pull/31382

💬 fanquake commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1860735726)
I don't really have one, given it's a somewhat obscure issue, but which is also why it'd be good to have proper documentation for. Anyone reading this in future isn't going to have any information other than, "don't use ffile-prefix-map otherwise it might break something in oss-fuzz". However given how long the build has been broken now, going to merge this to unbreak it.

💬 l0rinc commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1860729040)
nit: this being a console app, it might make sense to return a non-zero error code when `cerr` is used

🤔 l0rinc reviewed a pull request: "kernel: Flush in ChainstateManager destructor"
(https://github.com/bitcoin/bitcoin/pull/31382#pullrequestreview-2465133813)
Concept ACK

Closed https://github.com/bitcoin/bitcoin/pull/31362 in favor of this

🚀 fanquake merged a pull request: "build: Fix coverage builds"
(https://github.com/bitcoin/bitcoin/pull/31337)

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1860756141)
Yeah, done.

💬 maflcko commented on pull request "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile":
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-2504013215)
> It may be better to modify `CAutoFile` in that case. The file handle in the auto-file should[1] always be unique and pinned to a single thread, so I using `fread_unlocked` seems better, as it improves performance everywhere. (Though, it looks like `std::fread_unlocked` does not exist in C++, so I wonder if it exists on Windows and macOS?)
>
> [1] I may submit a related pull to better enforce this at compile-time.

For reference, `AutoFile::Get` was removed in commit a240e150e837b5a95ed197
...

💬 instagibbs commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#issuecomment-2504021426)
reACK 37a5c5d83664c31d83fc649d3c8c858bd5f10f21


blockhash is now an optional return

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#issuecomment-2504098705)
Seems like I am running into this here: https://github.com/bitcoin/bitcoin/pull/25073#issuecomment-1194417234, maybe time to introduce a replace mempool method like Carl suggested there?

✅ yancyribbens closed a pull request: "Add coin-grinder example test"
(https://github.com/bitcoin/bitcoin/pull/31352)

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1860890755)
> The problem is that no one will notice if this isn't run on any machine

I changed it to insist that the tcpdump file was created on the ASAN env. So the ASAN job will be red if this stops working on it.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1860910578)
I'd like to revive my original concerns here: [`c4eb31b` (#30906)](https://github.com/bitcoin/bitcoin/pull/30906/commits/c4eb31be3e56936479921b7556fcba1747867d02#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33L778-L782) removes this method completely, which I think is a better fix for it than the current workaround.