🤔 furszy reviewed a pull request: "qt, wallet: Convert uint256 to Txid"
(https://github.com/bitcoin/bitcoin/pull/32238#pullrequestreview-2813321727)
Would be good to register this new type into the Qt's meta-object system. Look at the `RegisterMetaTypes()` function on `src/qt/bitcoin.cpp`.
If we don't do it, the framework will (should) throw an error "not registered object type" when this new type is used as an argument in a signal or slot, or wrapped into a `QVariant` response (if I recall correctly, this was due to framework's internal dynamic casting).

✅ andrewtoth closed a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414)

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2848863531)
> is this comment still accurate? https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L2782 Guess it just refers to the size based trigger, not the time based - which is probably fine given its small context.

I think it's still accurate, and #30611 doesn't make it stale. It could possibly be updated to

> Regardless, I think we should cover this with a test - do you think we could extend e.g. https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_reindex.py wit
...

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072487540)
Actually we can't do that because the lock can't be held for `m_chainman.snapshot_download_completed();` :/.

💬 jamesob commented on pull request "scripted-diff: adapt script error constant names in feature_taproot.py":
(https://github.com/bitcoin/bitcoin/pull/32415#issuecomment-2848902968)
crACK https://github.com/bitcoin/bitcoin/pull/32415/commits/b5f580c580257d28d295cae3f787b55eb1863f16

Scripted-diff/func-test-only makes this an easy review. Good cleanup for this daunting file.

🤔 shahsb reviewed a pull request: "build: replace header checks with `__has_include`"
(https://github.com/bitcoin/bitcoin/pull/32405#pullrequestreview-2813356567)
Concept ACK.

This improves code readability and simplicity. This is indeed a nice feature from portability and backward compatibility perspective. Thanks for making the changes!!

💬 shahsb commented on pull request "build: replace header checks with `__has_include`":
(https://github.com/bitcoin/bitcoin/pull/32405#issuecomment-2848993111)
ACK https://github.com/bitcoin/bitcoin/pull/32405/commits/e1f543823b300b28c9edaf5d1a3e1e9badde471b

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2849074101)
You didn't mean to close it, right?

------

I can confirm that this change retains the flushing behavior during IBD as well as far as I can tell from the plots:


---


I'll compare the `reindex-chainstate` performance next, before & after, let's see if there's a regression (we do ex
...

💬 rebroad commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r2072559328)
snake_case?

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r2072583637)
Thanks for taking a look :)

I think the problem here is that we are moving the mempool pointer around between different objects, while they don't really share ownership of it at any one time. It not having a value at some points in time does have a logical meaning in the current code. In general I don't like shared pointers for a number of reasons. In this instance I agree that it would solve the problem of not knowing when to destruct the mempool. I am a bit apprehensive to commit this chang
...

👍 theStack approved a pull request: "crypto: Use secure_allocator for `AES256_ctx`"
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-2813438811)
Code-review ACK 3d7d2c37f7fe10e77d50c8b8fa4d6c74ad52a3c6

💬 maflcko commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072592135)
the function args are missing `std::common_type_t`, as explained in the function immediately above.

💬 maflcko commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#discussion_r2072592239)
any reason to remove the trailing whitespace?

🤔 maflcko reviewed a pull request: "fuzz: Suppress abort message on Windows"
(https://github.com/bitcoin/bitcoin/pull/32409#pullrequestreview-2813444120)
No objection, but assert/Assert seems to be used widely in the codebase, so shouldn't this be done for all test binaries, or none? Otherwise the same assert could lead to inconsistent behavior, depending on which test binary ran into it.

💬 maflcko commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2072606185)

https://cirrus-ci.com/task/5276299050090496?logs=ci#L3261:

```
[15:13:21.244] node2 2025-05-02T19:13:19.879966Z [httpworker.4] [rpc/request.cpp:241] [parse] [rpc] ThreadRPCServer method=getmempoolinfo user=__cookie__
[15:13:21.244] test 2025-05-02T19:13:19.882000Z TestFramework.node2 (DEBUG): RPC successfully started
[15:13:21.244] test 2025-05-02T19:13:20.885000Z TestFramework (ERROR): Assertion failed
[15:13:21.244] Traceback (most recent cal
...

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2072612696)
yeah i see it -- gotta figure out why `sock.recv(1024)` is non blocking on the two failing platforms

✅ maflcko closed a pull request: "Bugfix: Correct first-run free space checks"
(https://github.com/bitcoin/bitcoin/pull/29678)

💬 maflcko commented on pull request "Bugfix: Correct first-run free space checks":
(https://github.com/bitcoin/bitcoin/pull/29678#issuecomment-2849209362)
(close-open for fresh GitHub CI, I guess the Cirrus failure can be ignored for now)

📝 maflcko reopened a pull request: "Bugfix: Correct first-run free space checks"
(https://github.com/bitcoin/bitcoin/pull/29678)
It's not clear what `m_assumed_*_size` are actually set based on, but historically it was in GB, not GiB, and that's still used in the GUI which is more user-facing.

Could just as easily change the GUI if GiB is preferred.

💬 maflcko commented on pull request "[IBD] specialize block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#issuecomment-2849214120)
> > If IBD speedup is not the main motivation, what else is the motivation, given that the title mentions [IBD]?
>
> I think block serialization being fast could be useful for future kernel users. Reading and de-serializing blocks is one of the common operations I'd expect outside of just pure block validation.

Ok, seems fine. It would be good to list the main motivation(s) in the OP. Currently, it only says IBD and that IBD *isn't* the main motivation. However, I don't understand how this
...