💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535372567)
Thanks! (I believe that's fixed in this PR - 093980af8e4e594716b9219931fd441266c1fcd4 "init, net: Implement usage of binary-embedded asmap data")

💬 hodlinator commented on pull request "build: Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2535368296)
Ah, wasn't on my radar, cheers! Does simplify things a bit.

💬 instagibbs commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3543708460)
https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2432472005 still has invalidateblock in `test/functional/mempool_packages.py` ? If you don't have the time to carry the patch I can take over, just let me know.

🤔 hebasto reviewed a pull request: "refactor: Avoid -W*-whitespace in git archive"
(https://github.com/bitcoin/bitcoin/pull/33869#pullrequestreview-3474450076)
post-merge ACK.

🤔 glozow reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3455337470)
Code reviewed the "squashme"s (I think they should be squashed) and the end state for MemPoolAccept, miner, reorg, src/policy/* changes. I plan to put most of my comments on #33591. My suggestions here are to clean up the commits so the git history is easy to trace in the future.

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