💬 Christewart commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2519041227)
Hi @theStack , thanks for taking a look at this.

>I assume you are referring to the functional test p2p_invalid_tx.py rather than feature_block.py, as the latter only sends blocks to the node rather than individual txs?

No, my intention was relaying invalid blocks. This is useful for testing _new_ consensus proposals that are _currently_ policy - [which 64 byte transactions are currently disallowed by policy.](https://github.com/bitcoin/bitcoin/blob/48d4b936e09f82a3bcb65fe5850977e0e526e6
...

🤔 janb84 reviewed a pull request: "cmake: Move IPC tests to `ipc/test`"
(https://github.com/bitcoin/bitcoin/pull/33774#pullrequestreview-3454452001)
ACK 866bbb98fd365962840ee99df0d9f7ad557cc025

This PR moves the test files to the IPC directory, equal to the wallet folder structure. This removes the tidy exception file from the test folder which was only applicable (and there) for the ipc_tests. And some CMakeLists changes to accommodate the design choices made in mpgen (regarding prefix paths capnp)

Have invalidated a ipc_test to see if the ipc_tests would indicate a failure (it did)

💬 pablomartin4btc commented on pull request "Added test coverage for qt gui#901 console history filter":
(https://github.com/bitcoin-core/gui/pull/910#discussion_r2519071104)
nit: since you are there please update the copyright (`2016-present`)...

`// Copyright (c) 2016-2021 The Bitcoin Core developers`

🤔 pablomartin4btc reviewed a pull request: "Added test coverage for qt gui#901 console history filter"
(https://github.com/bitcoin-core/gui/pull/910#pullrequestreview-3454486228)
tACK 0e9f330bdc9435a1ebea229eba58d434b07ab3aa

💬 kevkevinpal commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3522953367)
thanks! I was able to get it to build with GCC 12 with this Docker build script

```
FROM ubuntu:22.04

RUN apt update
RUN apt install -y git build-essential cmake pkgconf python3 libevent-dev libboost-dev libsqlite3-dev gcc-12 g++-12

COPY . /bitcoin
WORKDIR /bitcoin

RUN cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON -DCMAKE_C_COMPILER=gcc-12 -DCMAKE_CXX_COMPILER=g++-12
RUN cmake --build build -j1
```

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