💬 theStack 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_r2518748320)
> It is my understanding that the `feature_block.py` test suites turns on [`-acceptnonstdtxn=1`](https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/test/functional/feature_block.py#L90) which turns off this policy check. Thus we don't receive any error message when submitting the transaction to the mempool.
>

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

💬 hebasto commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522533346)
> Not sure how to reproduce locally, but this seems to be failing:
>
> ```
> /__w/bitcoin-core-nightly/bitcoin-core-nightly/src/clientversion.cpp:33:79: error: trailing whitespace [-Werror=trailing-whitespace=]
> 33 | //! git will put "#define GIT_COMMIT_ID ..." on the next line inside archives.
> ```
>
> https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19297432422/job/55182808327#step:6:405

Does the following patch help:
```diff
--- a/src/clientversion.cpp
+++ b/sr
...

💬 kevkevinpal commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3522551720)
Just tested on master commit d4e2a458330512c227b475531e1617d25366be54 and the build still failed with this docker file

```
FROM ubuntu:22.04

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

COPY . /bitcoin
WORKDIR /bitcoin

RUN cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON
RUN cmake --build build -j1
```

are we sure https://github.com/bitcoin/bitcoin/pull/33853 closes this issue? @fanquake

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

💬 fanquake commented on pull request "build: Bump VS minimum supported version to 18.0":
(https://github.com/bitcoin/bitcoin/pull/33861#issuecomment-3522565335)
Seems a bit premature, if this is still waiting for multiple pieces of unreleased software? Could just wait for it to be shown as fixed in a nightly build?

💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3522591267)
Guix Build (x86_64):
```bash
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
083a468e350320244bfb83b46f2015d28a31ab69845fd5f605e31742c9665e27 guix-build-79d5f120f4d1/output/aarch64-linux-gnu/bitcoin-79d5f120f4d1-aarch64-linux-gnu.tar.gz
6031b50
...

🤔 fanquake reviewed a pull request: "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it"
(https://github.com/bitcoin/bitcoin/pull/33857#pullrequestreview-3452535328)
If we are migrating, should we just do this at this same time as we actually migrate, then we don't have to worry about both. Are we dropping support for the old runtime at the same time we switch to using the new ones in releases?

💬 fanquake commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#discussion_r2517636051)
How will someone know what to pick here? Should one be marked as deprecated/unsupported?

💬 fanquake commented on pull request "depends, doc: Learn `x86_64-w64-mingw32ucrt` host and document it":
(https://github.com/bitcoin/bitcoin/pull/33857#discussion_r2517626933)
Unrelated change? I don't think we need to add more `sudo` usage.

💬 hebasto commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3522632001)
> RUN cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON

It should be now:
```
cmake -B build -DENABLE_IPC=OFF -DBUILD_KERNEL_LIB=ON -DCMAKE_C_COMPILER=gcc-12 -DCMAKE_CXX_COMPILER=g++-12
```

💬 fanquake commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3522633115)
> Just tested on master commit https://github.com/bitcoin/bitcoin/commit/d4e2a458330512c227b475531e1617d25366be54 and the build still failed with this docker file

The minimum required GCC is now 12; however that Dockerfile will install 11. If you install and use 12, it will work.

💬 fanquake commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522646642)
> Not sure how to reproduce locally, but this seems to be failing:

Interesting. My Fedora box, with the same GCC (15.2.1), does not fail the same as your CI job: https://github.com/maflcko/bitcoin-core-nightly/actions/runs/19297432422/job/55182808331.

💬 maflcko commented on pull request "build: add `-W*-whitespace`":
(https://github.com/bitcoin/bitcoin/pull/32482#issuecomment-3522730033)
> Does the following patch help:

Yes, I presume it does help. I was just confused why the checkout GH action was using a git archive. Though, the guix release archive should be affected as well.

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2518940184)
Nice, that simplifies the interface, and it can always be added back if needed.

As it touches txgraph too, maybe best left for a follow-up?

💬 ryanofsky commented on pull request "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator.{h,cpp}`":
(https://github.com/bitcoin/bitcoin/pull/33218#issuecomment-3522746646)
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_target, must be between 1 and 1008
...

🤔 vasild reviewed a pull request: "wallet, sqlite: Encapsulate SQLite statements in a RAII class"
(https://github.com/bitcoin/bitcoin/pull/33033#pullrequestreview-3452618086)
Approach ACK 1829b3512d7bf430ec44daa801eb8fadf46cd30b

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2517689053)
Looks like `m_db` can never be `nullptr` since it is initialized by the constructor to a presumably non-null value. Consider making this a reference `sqlite3& m_db;`. That would signal to the readers that it can never be null and will also make it impossible to have weird states of this class that somehow end up having a null member.

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2517714796)
Just an observation:

Some of the callers of `sqlite3_prepare_v2()` call `sqlite3_finalize()` on failure. The doc says that if prepare fails then [*ppStmt is set to NULL](https://sqlite.org/c3ref/prepare.html) and [Invoking sqlite3_finalize() on a NULL pointer is a harmless no-op](https://sqlite.org/c3ref/finalize.html).

So it is ok to omit the finalize call.

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518281428)
I think it would be useful to print the statement as well (`stmt_text`).

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518277982)
The class name is `SQLiteStatement`, is this intentionally `SQLiteDatabase`?