💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822388)
I am not touching this line of code, and don't want to, so maybe a separate issue or pull request seems more appropriate?

💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822444)
No, signed chars are explicitly disallowed. See also: `git grep delete ./src/serialize.h`

```
src/serialize.h:template <typename Stream, CharNotInt8 V> void Serialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t
src/serialize.h:template <typename Stream, CharNotInt8 V> void Unserialize(Stream&, V) = delete; // char serialization forbidden. Use uint8_t or int8_t

💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822481)
No, std::memcpy is not `constexpr`, so adding it serves no purpose.

💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900822539)
I am not touching this line of code, and don't want to, so maybe a separate issue or pull request seems more appropriate?

💬 glozow commented on pull request "[28.x] Finalize 28.1":
(https://github.com/bitcoin/bitcoin/pull/31582#discussion_r1900830981)
Should this be 2025?

🤔 glozow reviewed a pull request: "[28.x] Finalize 28.1"
(https://github.com/bitcoin/bitcoin/pull/31582#pullrequestreview-2527491597)
ACK 898db487f3f4b4ec131e46cb66ab01b35de2c4c7 unless copyright year should be changed

✅ maflcko closed a pull request: "Debug Console implementation of generate method"
(https://github.com/bitcoin-core/gui/pull/692)

💬 maflcko commented on pull request "Debug Console implementation of generate method":
(https://github.com/bitcoin-core/gui/pull/692#issuecomment-2567705751)
Closing for now, due to inactivity for more than a year.

💬 maflcko commented on pull request "[28.x] Finalize 28.1":
(https://github.com/bitcoin/bitcoin/pull/31582#discussion_r1900835187)
I think no code from 2025 was backported, so this seems fine to keep as-is for now.

💬 maflcko commented on pull request "[28.x] Finalize 28.1":
(https://github.com/bitcoin/bitcoin/pull/31582#issuecomment-2567708618)
lgtm ACK 898db487f3f4b4ec131e46cb66ab01b35de2c4c7

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

💬 l0rinc commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1900842605)
sure, I don't have a strong preference either way (while this reflects the method-name-to-size connection better, I admit it's more verbose), please resolve the comment

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