🤔 instagibbs reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2725655591)
Looking good 0630995fee22990402771547be1480b8706c76ce

No real show-stoppers except for the diagram check fuzz test conceptual issue I'm still having.

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018742079)
micro-nit
```Suggestion
std::vector<FeeFrac> chunk_feerates;
```

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018744853)
We don't care or it will be respected within clusters due to CFR?

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018933019)
nit: const chunk_count

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018913351)
maybe overkill but `m_main_chunkindex` since there's a lot of level==0 checks everywhere gating access/modification

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018886603)
1d500a50b698a638c840ca79504c2a9a421e8756 stretch goal, have SanityCheck check m_chunk_count directly?

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018829062)
micro-nit: I know they're identical, but having the list being built via `stage_real_diagram` seems easier to digest what's being computed

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018906562)
nit: Could we note that this is initialized first in `AddTransaction`?

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019037611)
```Suggestion
// The same order should be obtained through a BlockBuilder as implied by CompareMainOrder, if nothing is skipped.
```

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2018984013)
b130e185f54c7f762da6ef435882335e4f0b98ff

Could the documentation get beefed up a bit for this? What the shape that range should look like, etc. Was only clear later when seeing usages.

💬 glozow commented on pull request "fuzz: extract unsequenced operations with side-effects":
(https://github.com/bitcoin/bitcoin/pull/32141#issuecomment-2762140874)
Backported in #32136

💬 glozow commented on pull request "cmake: Add `NO_CACHE_IF_FAILED` option for checking linker flags":
(https://github.com/bitcoin/bitcoin/pull/32027#issuecomment-2762141394)
Backported in #32136

💬 glozow commented on pull request "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain":
(https://github.com/bitcoin/bitcoin/pull/31849#issuecomment-2762141871)
Backported in #32136

💬 yancyribbens commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2019167894)
> The traceback in this case does print the source code line, so a comment on the same line could be sufficient. It depends on how the error is presented. Example failure:

This is an annoying draw back that the stack trace won't contain the line in the test that's in error, but instead the line in the `assert_not_equal` helper. In Rust, one can decorate the helper function with the `track_caller` macro. I don't see anything that is similar for C++ sadly.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019170353)
As far as diagram is concerned (incl. for `FeeRateCompare`), the order of equal-feerate element is irrelevant, as they just represent different sections of an overall line segment with a fixed slope. As far as `FeeRateCompare` is concerned, they could even be merged into a single line segment with the combined fee and size.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019171810)
Because fees can be negative in the fuzz test. If they were all non-negative, then I think this suggested assertion should hold.

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019173335)
ah! please leave a note as to that, I always forget that's possible

💬 Christewart commented on pull request "RFC: Accept non-std transactions in Testnet4 by default again":
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2762182397)
> > the question is why we are not also moving back on this setting, too, so that people can use non-std transactions on Testnet4.
>
> I don't think this is enough. There are many non-std transactions that will still be rejected, even if this is turned on. One example is #29843.

Another example is 64-byte transactions which was surprising behavior to me when working the BIP to disallow them:

https://github.com/bitcoin/bitcoin/blob/e486597f9a57903600656fb5106858941885852f/src/validation.
...

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019177841)
Oh, I misread this, but I'm still having trouble understanding what is intended. "Sort by feerate" ... "as these are just feerates".

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2019183993)
Maybe I'm making things more confusing by mentioning it at all?

My thinking is this: in general, to construct the full-graph chunk ordering, you *cannot* just sort by feerate, because you may sort two equal-feerate chunks within the same cluster incorrectly w.r.t. each other, breaking topology. However, here we are just construct a diagram, and don't care about what chunks they correspond to, so as far as the feerates of line segments in the diagram is concerned, we can just ignore this probl
...