💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519145535)
I agree your output looks better; I'm working on a commit that would implement this and will look to include it in #33591.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519151874)
Also, see https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2516471068 -- I decided to go ahead and switch all the new cluster-mempool related RPCs to output chunk and cluster feerates using FeePerWeight, rather then FeePerVSize. This makes it so that we don't introduce rounding error in things like the feerate diagram output or chunk output, which would be confusing (as it could make it look like chunks are sorted in an incorrect order).

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519153245)
This is hidden now.

💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3523047473)
Diffing `bitcoind.exe` built on x86_64 and aarch64, the offending symbols are:
```bash
wallet::SQLiteDataFile(fs::path const&)
wallet::ListDatabases[abi:cxx11](fs::path const&)
std::pair<fs::path, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >& std::vector<std::pair<fs::path, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<fs::path, std::__cxx11::basic_string<char, std::char_traits<char>, std::allo
...

🤔 stickies-v reviewed a pull request: "kernel: Allow null arguments for serialized data"
(https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3454620265)
Post-merge ACK a3ac59a4316305fb38a5338b48940682889d0dc2

It seems like the same reasoning should apply to `btck_chainstate_manager_options_create`'s `data_directory` and `blocks_directory`? Perhaps useful for a follow-up, with a brief docstring on how to use and interpret `BITCOINKERNEL_ARG_NONNULL` so we're clear on its usage? E.g.:

```
/**
* BITCOINKERNEL_ARG_NONNULL is a compiler attribute used to indicate that
* certain pointer arguments to a function are not expected to be null.

...

💬 stickies-v commented on pull request "kernel: Allow null arguments for serialized data":
(https://github.com/bitcoin/bitcoin/pull/33853#discussion_r2519176246)
Probably useful to `LogError` here, and in the other 2 places?

✅ maflcko closed an issue: "Enable C++26 P3471 "Standard Library Hardening" ahead of time?"
(https://github.com/bitcoin/bitcoin/issues/32265)

💬 maflcko commented on issue "Enable C++26 P3471 "Standard Library Hardening" ahead of time?":
(https://github.com/bitcoin/bitcoin/issues/32265#issuecomment-3523076321)
This seems nice, but probably has limited benefit in practise, given the tradeoffs. Closing for now.

👍 l0rinc approved a pull request: "txgraph: drop move assignment operator"
(https://github.com/bitcoin/bitcoin/pull/33862#pullrequestreview-3454502686)
code review ACK aef40b93cf057d2a27d61881b0858d491206bcd3

Simplifies the public interface of `TxGraph` by deleting the move assignment operator only used in tests.
The usage seems correct as it is currently, but left a question about the move-constructor which the fuzzers seem to be confused about.

💬 l0rinc commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#discussion_r2519205585)
This seems used to me, but https://storage.googleapis.com/oss-fuzz-coverage/bitcoin-core/reports/20251112/linux/src/bitcoin-core/src/txgraph.cpp.html#L3469 seems confused by it.

💬 instagibbs commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3523123058)
link to discussion, I think https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2518940184

📝 ismaelsadeeq opened a pull request: "scripted-diff: fix leftover references to `policy/fees.h`"
(https://github.com/bitcoin/bitcoin/pull/33864)
Fixes #33863

ryanofsky wrote
> I still see some references to the src/policy/fees.h file removed by this PR:

```
$ git grep -n policy/fees.h
src/wallet/rpc/spend.cpp:206: * @param[in] conf_target UniValue integer; confirmation target in blocks, values between 1 and 1008 are valid per policy/fees.h;
test/functional/rpc_estimatefee.py:39: # max value of 1008 per src/policy/fees.h
test/functional/rpc_psbt.py:604: assert_raises_rpc_error(-8, "Invalid conf_
...

👋 ismaelsadeeq's pull request is ready for review: "scripted-diff: fix leftover references to `policy/fees.h`"
(https://github.com/bitcoin/bitcoin/pull/33864)

💬 ismaelsadeeq commented on issue "fees: leftover references to `policy/fees.cpp`":
(https://github.com/bitcoin/bitcoin/issues/33863#issuecomment-3523137257)
Thanks @ryanofsky @fanquake I opened #33864 with a fix

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3523154890)
I believe at this point I've responded to all the review feedback either in this PR or for non-blocking suggestions, in the followups PR (#33591) -- please let me know if any reviewers think I've missed anything that should be addressed here.

💬 hebasto commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3523159466)
@ryanofsky

Would you like to take another look at this PR?

💬 kevkevinpal commented on pull request "scripted-diff: fix leftover references to `policy/fees.h`":
(https://github.com/bitcoin/bitcoin/pull/33864#issuecomment-3523327846)
ACK [b0a3887](https://github.com/bitcoin/bitcoin/pull/33864/commits/b0a38871546dfcdd3a578c1ae4c28a88b6ee32d5)

A doc change and scripted-diff are pretty straightforward

💬 cobratbq commented on pull request "doc: update interface, --stdin flag, IPC-command signtx (#31005)":
(https://github.com/bitcoin/bitcoin/pull/33765#discussion_r2519455789)
Oookaaaaaay .. that changes things considerably. That was not at all clear to me.

Any chance you can drop a link to the place in source-code (assuming it is somewhat concentrated), such that I can have a quick peek. I could search myself, but I suspect you are familiar with the code-base.

💬 willcl-ark commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3523497358)
```
❯ uname -m; find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
x86_64
eacb251187a66b61d420c134b48b4081cac704cfaef3965bc10f61f2dedbe9b1 guix-build-79d5f120f4d1/output/aarch64-linux-gnu/SHA256SUMS.part
245e7592e13a90600f6ccff66442e98027d9f36b868942c88a117c05625e603e guix-build-79d5f120f4d1/output/aarch64-linux-gnu/bitcoin-79d5f120f4d1-aarch64-linux-gnu-debug.tar.gz
083a468e350320244bfb83b46f2015d28a31ab69845fd5f605e31742
...

🤔 w0xlt reviewed a pull request: "net_processing: reorder the code that handles the VERSION message"
(https://github.com/bitcoin/bitcoin/pull/33792#pullrequestreview-3455027702)
The reordering of `WTXIDRELAY` and `SENDADDRV2` looks safe to me and keeps them before the tx‑reconciliation announcement.

What’s the rationale for moving `m_addrman.Good(pfrom.addr)` earlier ?