💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519820632)
ccf7c5ac417 looks good and can be squashed

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519700544)
nit if you retouch: commit message for cec26203732 should replace "descendantsize" with "descendant count"

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519770412)
If you retouch: f02ff837008 can be squashed into the commit above it. I think the diff may have shrunk over time - the commit message mentions `addUnchecked` which no longer exists in the codebase.

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2519738828)
nit in 1eddff0b0ab if you retouch: `GetDescendantCount`, `GetAncestorCount` would be more similar to existing naming, and slightly less confusing since the count includes the tx itself.

💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2535401285)
nit in c9efae8a00daf0ab1b9f94db231668aa823aba4e commit message: our docs call this carveout "Single-Conflict RBF Carve Out". It's distinct from CPFP carveout, so it could be confusing to mention that in the commit message.

🤔 hodlinator reviewed a pull request: "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation"
(https://github.com/bitcoin/bitcoin/pull/33878#pullrequestreview-3473921944)
Reviewed eaf5e12d2b52d66a75a68cf94d328d0ec9048986

Thanks for peeling off this PR as I suggested!

💬 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_r2535184260)
In c2274746d1b3d1a2712ef8bd567d25f5aa5cf5bb "refactor: Operate on bytes instead of bits in Asmap code": Line seems like a leftover from a merge?

💬 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_r2535169330)
In c2274746d1b3d1a2712ef8bd567d25f5aa5cf5bb "refactor: Operate on bytes instead of bits in Asmap code": It's unfortunate that you need to make the hex data a `vector` again when the later span commit will accept the `std::array`.

You could do:
```C++
NetGroupManager netgroup{std::vector(ASMAP_DATA.begin(), ASMAP_DATA.end())};
```
In this commit and avoid changing the type of `ASMAP_DATA`.

💬 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_r2535040100)
In c2274746d1b3d1a2712ef8bd567d25f5aa5cf5bb "refactor: Operate on bytes instead of bits in Asmap code": The `for` doing `ip_bits[byte_i] = std::byte(IPV4_IN_IPV6_PREFIX[byte_i]);` can now be removed since we have `copy_n` doing the same thing above.

💬 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_r2535279441)
In 0196f70a8c99b9c03c81e879cd9f05560fc20543: Could replace second sentence with "Returns true if the data is valid for 128 bits long inputs" or just remove it (no longer returns span).

💬 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