🤔 willcl-ark reviewed a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275#pullrequestreview-2196260920)
Looks good to me, left a micro style nit if you end up touching to address @glozow 's comments.

It makes sense to default to more economical fee estimation by default, since users can use RBF (or "conservative" estimation) in those cases when it may be required. RBF opt-in has been enabled by default in Bitcoin Core wallets since ~2022 and there is a proposal to enable "full" RBF as default as mempool policy currently open: https://github.com/bitcoin/bitcoin/pull/30493 (previously https://gi
...

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689578142)
Thanks for the feedback, but I don't see the benefit of changing this, so I'll leave it as-is for now.

💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1689580744)
> no side-effects should follow from running `IsHex`, right?

Yes, IsHex is pure.

📝 fanquake opened a pull request: "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job"
(https://github.com/bitcoin/bitcoin/pull/30519)
Add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to one of the libc++ CI jobs, to catch missing includes, that are otherwise hidden by transitive includes inside libc++. A more appropriate place for this might be the tidy job, but that does not use libc++.

See https://libcxx.llvm.org/DesignDocs/HeaderRemovalPolicy.html for more information.

🤔 danielabrozzoni reviewed a pull request: "refactor: Add FlatFileSeq member variables in BlockManager"
(https://github.com/bitcoin/bitcoin/pull/30517#pullrequestreview-2196466308)
ACK 7aa8994c6fceae5cf8fb7e661371cdb19d2cb482

💬 glozow commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689635561)
done

💬 glozow commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689635405)
Hm, added a `ACQUIRED_BEFORE`, but compiler didn't complain when I added a `LOCK2(m_mempool.cs, m_tx_download_mutex)`

💬 glozow commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#discussion_r1689635780)
done

👋 glozow's pull request is ready for review: "m_tx_download_mutex followups"
(https://github.com/bitcoin/bitcoin/pull/30507)

💬 maflcko commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1689634589)
nit: The same can be achieved, by setting C++23, according to the docs.

This may be beneficial for other reasons as well, even if it is just turning existing compile warnings into errors (https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1675775175)

So adding `-std=c++23` here, and a comment that `libc++` is required for the feature seems better. But this is fine either way.

💬 maflcko commented on pull request "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job":
(https://github.com/bitcoin/bitcoin/pull/30519#discussion_r1689636199)
nit: If it compiles, while touching, could fix all headers?

```
init.h should add these lines:
#include <atomic> // for atomic

init.h should remove these lines:
- #include <any> // lines 9-9
- #include <memory> // lines 10-10
- #include <string> // lines 11-11

👍 maflcko approved a pull request: "ci: add `_LIBCPP_REMOVE_TRANSITIVE_INCLUDES` to TSAN (libc++) job"
(https://github.com/bitcoin/bitcoin/pull/30519#pullrequestreview-2196512417)
lgtm

This is useful to catch build failures before they hit users that compile from source.

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1689639934)
Matches my diagrams. Thanks for the additional context! :+1:

👍 tdb3 approved a pull request: "refactor: Add FlatFileSeq member variables in BlockManager"
(https://github.com/bitcoin/bitcoin/pull/30517#pullrequestreview-2196603604)
ACK 7aa8994c6fceae5cf8fb7e661371cdb19d2cb482

👍 TheCharlatan approved a pull request: "Early logging improvements"
(https://github.com/bitcoin/bitcoin/pull/30386#pullrequestreview-2196253338)
Nice, ACK b4dd7ab43e8cfc2c171f67588e4e1ec2705393c2

💬 TheCharlatan commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1689628041)
Nit: Why is this being set again? I'm guessing it is not declared as `const` to future-proof it against introducing an option controlling its size, but it is not clear why disconnecting should clear such a value again.

💬 TheCharlatan commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1689526333)
Nit: Unneeded newline.

💬 hebasto commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#issuecomment-2247822717)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/280.

💬 hebasto commented on pull request "Add fuzz test for FSChaCha20Poly1305, AEADChacha20Poly1305":
(https://github.com/bitcoin/bitcoin/pull/28263#issuecomment-2247823778)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/280.

💬 hebasto commented on pull request "depends: Bump `libmultiprocess` for CMake fixes":
(https://github.com/bitcoin/bitcoin/pull/30513#issuecomment-2247862187)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/277.

💬 hebasto commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2247865660)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/278.