💬 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_r2535412684)
Related: https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2522389508

Thanks for documenting the C++ implementation!

nit:
```suggestion
// in the input. If they all match, execution continues. If not, the default ASN is returned (or 0 if unset).
```

Could also correct the Python side:
```diff
--- a/contrib/asmap/asmap.py
+++ b/contrib/asmap/asmap.py
@@ -157,7 +157,7 @@ class _Instruction(Enum):
JUMP = 1
# A match instruction, encoded as [1,1,0] inspects 1 or more of the
...

💬 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_r2535226892)
nit in 4c8e4eee00f16bace634221277c694ee5dda23f8: Sorry, accidentally injected possessive apostrophe, "span's", commit message should be:
> The version hash changes due to spans being serialized without their size-prefix (unlike vectors).

💬 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