💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900843215)
That was my understanding as well, what I suggested was just to remove the redundant `inline`

💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900844439)
Yes, I saw, but is this the level where it should be disallowed (since this would technically work with signed char, disallowing it seems like a different abstraction level)? But if it is, please resolve the comment

📝 maflcko opened a pull request: "ci: Run functional tests in msan task"
(https://github.com/bitcoin/bitcoin/pull/31592)
Now that the CI machines have a bit more CPU, it seems good to run the functional tests as well under msan. (Also, bump the llvm minor version)

💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900848028)
If there is a use-case for this, it can be added, but I think limiting to "unsigned" bytes seems fine and preferable for now.

💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900850994)
I don't have a strong feeling, see https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1874382161.

My preference would be to have a clang-tidy (plugin) rule for this, instead of using human review resources.

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

Simplifies the shutdown logic, and makes ChainstateManager more self contained. I'll need to think a bit more about how to test or verify the safety of changes such as in 7a37f2008372c021541809d06d9300278fd5a8da.

💬 stickies-v commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1888637633)
Shouldn't this be checked right after making the mempool?

💬 stickies-v commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1900683877)
I wonder if it would make sense to (probably in a separate PR, I suspect it'll be quite a big change) remove the `Chainstate::m_mempool` member. It feels like a footgunny shortcut (but need to investigate more), conceptually doesn't make a lot of sense to me, and not having it would avoid having to do e.g. the`DummyChainState` gymnastics. Is this something you've explored already?

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