💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2016928022)
Done in #32151.

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2016928925)
re: https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2016904181

Will look more closely, but I think both the old code and new code are assuming that the chainstate tip is an ancestor of the chainstate target block, or points directly to it. If that's not true I think you are right that the behavior of this function before and after this change would be different, but a lot of other things would be broken too.

💬 hodlinator commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2758458019)
What I'm missing is a `SLOWCHECK`/`DCHECK` macro that is active in debug and always active in fuzz builds (and fails fuzz-builds on failure even in release). For cases when something very unexpected is happening but it's not worth crashing the node in production over it. Hopefully this need is not due to wanting to make up for sloppy comprehension of the code.

(It's also unfortunate that `Assume()` uses the same term as the C++23 `[[assume]]` feature with very different semantics).

💬 laanwj commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2758496926)
> pcp: Requesting port mapping for addr 0.0.0.0 port 9654
> I'd have thought it would be either my LAN or WAN address?

No, that message is OK. For IPv4 it doesn't know (nor care) what the internal address is, so it requests 0.0.0.0 when calling the mapping function. This will figure out what the relevant internal IPv4 address (the one toward the gateway) and external IPv4 address (the one that the router will give us) is.

🤔 hodlinator reviewed a pull request: "descriptors: Multipath/PR 22838 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2722269042)
Attempted to improve naming in latest push in response to https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2721049447.

💬 hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2016990022)
Thanks for pointing this out. Context implies "multipath" now that the lifetime has been narrowed, making the naming poorer.

"nums" feels quite general though. Renamed to `seen_substitutes` in latest push. Made me modify an additional line but I think it might be worth it.

💬 hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2016981177)
I would conceptually call them derivation path components... :) But like shorter names, changed to `substitutes`. How about that?

Also changed `segment_index` to `placeholder_index`, inspired by the code comment
> // Replace the multipath placeholder with each value while generating paths

💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2017019283)
> re: [#30214 (comment)](https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2016904181)

> What if the snapshot block is never reached? Does background validation then just continue on indefinitely?

I think you are right and there is an improvement that can be made here. The case where the validation chainstate tip somehow points to a higher block than than target block (snapshot block) is supposed to be impossible and should never occur. That case used to return `BASE_BLOCKHASH_MISM
...

💬 yancyribbens commented on pull request "refactor: Optimize BnB exploration":
(https://github.com/bitcoin/bitcoin/pull/32150#issuecomment-2758592756)
Concept ACK

💬 yancyribbens commented on pull request "refactor: Optimize BnB exploration":
(https://github.com/bitcoin/bitcoin/pull/32150#issuecomment-2758612172)
I ran benchmarking on master and benchmarking on this branch since it's fairly large change.

master:
```
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 43,633,696.00 | 22.92 | 0.1% | 0.48 | `CoinSelection`
```

https://github.com/bitcoin/bitcoin/pull/32150/commits/e2a4d86131e8f1661eb450c50b8072147965b111:
```

| ns/op |
...

🤔 instagibbs reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2713729194)
just a few comments after first read through

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2012192461)
why not sort with size tiebreaks here? It isn't clear based on context.

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2012087403)
`Assume` neither cluster set is oversized?

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2012288718)
should say that the FeePerWeight is the cfr, and that it cannot be called on an oversized graph

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2012233270)
I'm wondering what the best way of catching stuff that should be missing on both sides being added to both diagrams erroneously.

You could imagine a degenerate case where a bug turns this into a whole mempool check. (and implies we're doing a bunch of work maintaining these clusters for no reason)

A rough suggestion to think about, perhaps you can tighten the bounds?

```
diff --git a/src/test/fuzz/txgraph.cpp b/src/test/fuzz/txgraph.cpp
index 7249827103..f30b650313 100644
--- a/src/t
...

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2012199860)
"comparable" through me off here since I was thinking about comparable diagrams.

Can't immediately think of better wording.

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2012318226)
```Suggestion
/** Cache of discarded ChunkIndex node handles to re-use, avoiding additional allocations. */
```

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2016674189)
```suggestion
auto main_full_diagram = get_diagram_fn(/*main_only=*/true);
```

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2016679161)
```suggestion
auto stage_full_diagram = get_diagram_fn(/*main_only=*/false);
```
the argument could also just take an explicit level/staging arg since they're fully determined at each call site for now

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2016730122)
```suggestion
// Do the same for chunks in stage_diagram missing from stage_cmp_diagram.
```