💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535526914)
> > If the most recent evaluation of statement S failed, then sqlite3_finalize(S) returns the appropriate [error code](https://sqlite.org/rescode.html) or [extended error code](https://sqlite.org/rescode.html#extrc)
>
> I am not sure what is the best way to handle this. Ignoring the return value does not look good.

This text reads to me as `sqlite3_finalize` propagating any previous errors, e.g. from `sqlite3_step`. Since any errors from that should already be handled, I think it's ok to i
...

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535561586)
Done

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535561856)
Done

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535562445)
Done

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535562616)
Done

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535562832)
Removed

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535563023)
Done

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535563262)
Done

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535564145)
I've changed this to catch the exception and return `std::nullopt`.

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535564673)
Changed to catch the exception.

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535566375)
Done, returns an empty string now.

But the only time `sqlite3_column_text` should return NULL is if the column contains a NULL value. Empty strings are not NULL.

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535566840)
Done

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2535567070)
Done

💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2535608194)
It's unnecessary to log this kind of information. This would be more appropriate as a comment, but it's really not necessary at all (yes this test does that elsewhere, but it shouldn't).

💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2535612380)
This log is unnecessary.

💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2535612003)
It's not necessary to generate `COINBASE_MATURITY` blocks since we're not spending those coins. This also makes the previously existing comment incorrect as that states only 10 blocks are being mined.

💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2535613879)
I don't think this test case is necessary, it doesn't seem to me to be testing anything that isn't already being tested.

💬 achow101 commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-3543990625)
Are you still working on this? This has needed rebase for a few months now.

💬 achow101 commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3543995583)
Are you still working on this? This has needed rebase for a few months now.

💬 achow101 commented on pull request "psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows":
(https://github.com/bitcoin/bitcoin/pull/32419#issuecomment-3544000265)
ACK d31158d3646f3c7e4832b9ca50f6ffe02800ff4c