💬 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`?

💬 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