💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2535403848)
Continuing https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2514801922

We still mix `std::span` and `const std::span&` here in:

https://github.com/bitcoin/bitcoin/blob/eaf5e12d2b52d66a75a68cf94d328d0ec9048986/src/util/asmap.h#L16-L25

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535431623)
> Exactly, let's exercise those differences, that's exactly what we want from our tests, to ruthlessly try to break the code, right?

We’re going to side-track the discussion a bit, but I’m not sure I completely agree.
Generally, I see unit tests as being meant for correctness, not stress testing. For example, we want to ensure certain behavior is always retained — it’s not about trying to break the code in a non-deterministic way. That’s what fuzz tests are for, where we randomize inputs. I’
...

💬 hodlinator commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2535456270)
Agree with the rename suggestion, would make `assert(addr_bytes.size() == ip_bits.size());` less surprising below.

💬 plebhash commented on issue "IPC via TCP Sockets":
(https://github.com/bitcoin/bitcoin/issues/32802#issuecomment-3543755211)
what about encryption?

assuming authentication is solved, would it be safe to use this outside of a controlled LAN environment?

💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2535484827)
I imagine the problem is that you haven't compared the current http code with the `ThreadPool` code, which is pretty much the same but properly documented and test covered. That's one of the reasons behind the not modernization of the underlying sync mechanism in this PR, and why I added the "Note 2" paragraph as well as wrote https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3444568607.

Look at the current `WorkQueue`:

```c++
void Run() EXCLUSIVE_LOCKS_REQUIRED(!cs)
{

...

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