💬 stickies-v commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1900780796)
Should we do this before calling `m_node.chainman.reset();`? I'm not sure if any of the callbacks actually depend on the chainman (it doesn't look like it, but I'll need to investigate more), but I think it's a bit more logical at least?

💬 stickies-v commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1900712774)
nit: I would go further and merge it with the above docstring. IIUC correctly, they all talk about L349?

<details>
<summary>git diff on 7a37f20083</summary>

```diff
diff --git a/src/init.cpp b/src/init.cpp
index 695e365867..b558a1594d 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -344,16 +344,17 @@ void Shutdown(NodeContext& node)
}
}

- // After there are no more peers/RPC left to give us new data which may generate
- // CValidationInterface callbacks, fl
...

💬 stickies-v commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1900781622)
nit: the docstring is now a bit out of date:

```
// Simulate a restart of the node by flushing all state to disk, clearing the
// existing ChainstateManager, and unloading the block index.
```

📝 maflcko opened a pull request: "ci: Bump centos stream 10"
(https://github.com/bitcoin/bitcoin/pull/31593)
This is a follow-up to fa47baa03bcfcf44fb2ed05f009a32d32f860c45, which bumped the gcc version to avoid a warning bloat in the CI log. However, it is also required to bump python3, see https://github.com/bitcoin/bitcoin/issues/31476#issue-2735206340

> This will uncover an issue in the centos task that the correct python version is missing. I guess this should be fixed by installing and activating an acceptable python version.

Instead of bumping the packages individually in centos stream 9,
...

💬 maflcko commented on issue "CI: Cmake warnings should be errors":
(https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-2567748954)
> This will uncover an issue in the centos task that the correct python version is missing. I guess this should be fixed by installing and activating an acceptable python version.

Fixed in https://github.com/bitcoin/bitcoin/pull/31593

🤔 glozow reviewed a pull request: "include verbose "reject-details" field in testmempoolaccept response"
(https://github.com/bitcoin/bitcoin/pull/28121#pullrequestreview-2527544463)
ACK b6f0593f43304f4ff31e8b68558ceeb1b588403c

💬 ryanofsky commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900870464)
In commit "refactor: Allow std::byte in Read(LE/BE)" (fa83bec78ef3e86445e522afa396c97b58eb1902)

There is a shorter syntax for this that I think would make code easier to read (thoughout this file):

```diff
-template <ByteType B>
-inline uint16_t ReadLE16(const B* ptr)
+inline uint16_t ReadLE16(const ByteType auto* ptr)
```

📝 maflcko opened a pull request: "28.1rc3 backports"
(https://github.com/bitcoin/bitcoin/pull/31594)
Backports:

- #31502
- #31563

💬 maflcko commented on pull request "[28.x] Finalize 28.1":
(https://github.com/bitcoin/bitcoin/pull/31582#issuecomment-2567775660)
> There are still things tagged for backport for 28.x that haven't been backported.

Fixed in https://github.com/bitcoin/bitcoin/pull/31594

💬 maflcko commented on pull request "depends: Fix `CXXFLAGS` on NetBSD":
(https://github.com/bitcoin/bitcoin/pull/31502#issuecomment-2567776487)
28.x backport in https://github.com/bitcoin/bitcoin/pull/31594

💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900881285)
No strong opinion from my side. I'll leave as-is for now, but other reviewers are encouraged to chime in with up/down votes.

🤔 tdb3 reviewed a pull request: "qa: Use `sys.executable` when invoking other Python scripts"
(https://github.com/bitcoin/bitcoin/pull/31541#pullrequestreview-2527577705)
Concept ACK

Ran both tests with `test_runner` on NetBSD 10.1 and Ubuntu 24.04. Both passed. Encountered a failure with `tool_wallet` on NetBSD that I want to check out before full ACK.

💬 maflcko commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#discussion_r1900884815)
not sure about hardcoding. It would be better to have a symbol for this. This would also allow to remove `mktemp` and just use the build dir.

💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900895399)
Personally, I find the `ByteType auto` construct slightly more confusing than the more verbose template.

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1900884712)
The chainman reset may trigger validation interface notifications, so I think it makes sense to sync with the queue afterwards? Unlike the shutdown procedure, we keep the validation interface around beyond the restarts there.

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1900883476)
I could not come up with a satisfactory alternative yet, suggestions welcome!

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1900883589)
Yeah, that makes more sense.

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1900905175)
This seems fine as is? I'd prefer not to document these deeper behaviours, that the caller can expect to "just work".

💬 stickies-v commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1900927965)
Sorry, I meant to to remove the flushing reference since that's now automatically handled by clearing chainman, so I agree with your comment.

> Simulate a restart of the node by clearing the existing ChainstateManager, and unloading the block index.

💬 ismaelsadeeq commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#discussion_r1900961662)
Done, thanks