💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518476250)
Previously on error `ReadPragmaInteger()` would have returned `nullopt`. Now it would `throw`. Its only caller is `SQLiteDatabase::Verify()` which would have returned `false` and now would let the exception propagate. The only caller of `SQLiteDatabase::Verify()` is `MakeSQLiteDatabase()`. Before, on `Verify()` failure (return `false`) `MakeSQLiteDatabase()` would have set `status = DatabaseStatus::FAILED_VERIFY` and now (exceptioin) it would set `status = DatabaseStatus::FAILED_LOAD`. Is this o
...

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518340792)
From https://sqlite.org/c3ref/column_blob.html:
> if the column index is out of range, the result is undefined

Maybe at the beginning of the function, add:

```cpp
Assert(col < sqlite3_column_count());
```

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518298601)
`Reset()` is not needed here?

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518397420)
What happens if `T` is unsigned? Maybe add https://en.cppreference.com/w/cpp/types/is_signed.html here: `std::integral<T> && std::is_signed<T>`? (and leave it to drop to "else assert always false" for unsigned `T`s)

💬 vasild commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2518504627)
Now `ReadPragmaInteger()` would return `nullopt` on some errors and `throw` on others. What about making it to only `throw`? Then its return type would change from `std::optional<int>` to `int` and its callers would not need to check `if (!read_result.has_value()) ...`.

I guess also `SQLiteStatement::Step()` can `throw` if the return value of `sqlite3_step()` is not `SQLITE_ROW` or `SQLITE_DONE`.

💬 fanquake commented on pull request "depends: drop Qt patches":
(https://github.com/bitcoin/bitcoin/pull/33860#issuecomment-3522776226)
Guix Build (aarch64):
```bash
6eb3c63d243ad8b9e10bed1aa9ec545d4599a10e2d333a69b346c8aaab941fb2 guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/SHA256SUMS.part
b7bf29e4e64a207eca6ec785f2a8944d0051c177636b6f8db78da5836404ae89 guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/bitcoin-3e9aca6f1b52-aarch64-linux-gnu-debug.tar.gz
dea243b0ce4f6a389ad1612d4f27f0bca816f5f11fe77c2ff4ca703a1db607ee guix-build-3e9aca6f1b52/output/aarch64-linux-gnu/bitcoin-3e9aca6f1b52-aarch64-linux-gnu.tar.gz
341a70
...

📝 sipa opened a pull request: "txgraph: drop move assignment operator"
(https://github.com/bitcoin/bitcoin/pull/33862)
This removes the only place where move-assignment of `TxGraph::Ref` is used (in tests), and drops supports for it.

Suggested in https://github.com/bitcoin/bitcoin/pull/33629/commits/51430680ecb722e1d4ee4a26dac5724050f41c9e#r2516214890

💬 sipa commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3522786219)
cc @ajtowns

⚠️ fanquake opened an issue: "fees: leftover references to `policy/fees.cpp`"
(https://github.com/bitcoin/bitcoin/issues/33863)
Pointed out here: 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:
...

💬 fanquake commented on issue "fees: leftover references to `policy/fees.cpp`":
(https://github.com/bitcoin/bitcoin/issues/33863#issuecomment-3522823603)
cc @ismaelsadeeq

💬 kevkevinpal commented on issue "`test_kernel` fails to build on Ubuntu 22.04":
(https://github.com/bitcoin/bitcoin/issues/33846#issuecomment-3522836798)
> > Just tested on master commit [d4e2a45](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.

ok gotcha I assumed https://github.com/bitcoin/bitcoin/pull/33853 alone closed the issue, will attempt to build again using GCC 12

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