💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1691238319)
@glozow I don't think so.

💬 fanquake commented on pull request "depends: Cleanup postprocess commands after switching to CMake":
(https://github.com/bitcoin/bitcoin/pull/30506#issuecomment-2250064713)
Looks good. Want to pull in the additional CMake cleanup here: https://github.com/fanquake/bitcoin/tree/30506_more_cleanup?

🚀 fanquake merged a pull request: "ci: Add missing qttools5-dev install to Asan task"
(https://github.com/bitcoin/bitcoin/pull/30522)

💬 sipa commented on issue "Unexpected behaviour when using `sortedmulti_a` descriptor":
(https://github.com/bitcoin/bitcoin/issues/30518#issuecomment-2250066455)
This apparent inconsistency is due to the fact that `multi_a` exists both as a top-level descriptor fragment and inside miniscript, but `sortedmulti_a` only as a top-level fragment.

We should probably add `sortedmulti_a` to miniscript too.

💬 fanquake commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2250067668)
Seems like if a script such as this is going to exist, it should be getting run as part of the CI, so that regressions are actually caught (if that's what we want to do)?

note: `libbitcoin_crypto_arm_shani` is missing from `LIBS[crypto]` crypto.

💬 maflcko commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#issuecomment-2250077244)
test-only pull with two ACKs, rfm?

💬 hebasto commented on pull request "depends: Cleanup postprocess commands after switching to CMake":
(https://github.com/bitcoin/bitcoin/pull/30506#issuecomment-2250080208)
> Looks good. Want to pull in the additional CMake cleanup here: https://github.com/fanquake/bitcoin/tree/30506_more_cleanup?

Thanks! Pulled.

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1691281014)
Hm, I was confused because when I was looking at the code, I expected the branch with `depgraph.AddDependency(i, 4)` to have lots of T4-->Tx in its diagram. And the branch with `depgraph.AddDependency(i, 3)` to have lots of T3-->Tx.

{T0...T9} is 10 transactions, and {T0...T10} is 11 transactions. Am I missing something?

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1691293586)
Errr, yes, you're right of course!

The odd and even are correct, but the mermaid code for the two are swapped.

🤔 glozow reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2199052326)
reACK df749c06e9d via range-diff

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1691300770)
Whew, so not wholly unmeritorious.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1691305266)
Yeah, not totally without merit.

Actually, I don't understand at all what is going on. The "// Odd cluster size." code constructs a graph with an odd number of transactions, and the "// Even cluster size." constructs one with an even number. Further, the mermaid code's fees/sizes matches the corresponding code, but does not match the transaction counts?!

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_1 <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1691329195)
will do if I touch the PR again

💬 instagibbs commented on pull request "m_tx_download_mutex followups":
(https://github.com/bitcoin/bitcoin/pull/30507#issuecomment-2250164949)
reACK https://github.com/bitcoin/bitcoin/pull/30507/commits/7c29e556c573a63351096c34bc072ae0c60ffa29

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1691340097)
Sigh! I believe this was the problem: it was constructing the intended-to-be-even-sized graph with an odd number of transactions and vice verse:

```diff
diff --git a/src/bench/cluster_linearize.cpp b/src/bench/cluster_linearize.cpp
index ddd7c6a7a61..3ebcc383bb4 100644
--- a/src/bench/cluster_linearize.cpp
+++ b/src/bench/cluster_linearize.cpp
@@ -33,10 +33,10 @@ DepGraph<SetType> MakeHardGraph(ClusterIndex ntx)
{
DepGraph<SetType> depgraph;
for (ClusterIndex i = 0; i < ntx
...

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2250179956)
reACK https://github.com/bitcoin/bitcoin/pull/30126/commits/df749c06e9d45ba92a4699e1d3e58522f93ad6bb

via `range-diff master cad318fa843f411e52c6761a4882bfaf0ad21812 df749c06e9d45ba92a4699e1d3e58522f93ad6bb`

Didn't verify the mermaid diagrams in the comments.

At this point could we maybe not rebase on master unless necessary? :)

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2250182427)
> At this point could we maybe not rebase on master unless necessary? :)

I shall resist.

💬 instagibbs commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2250188693)
reACK https://github.com/bitcoin/bitcoin/pull/30126/commits/448374f2c901c48824904357b4e1297105e97315

via `git range-diff master df749c06e9d45ba92a4699e1d3e58522f93ad6bb 448374f2c901c48824904357b4e1297105e97315`

only changes to MakeHardGraph to reflect the intended topology

💬 TheCharlatan commented on pull request "depends: remove Darwin ENV unsetting":
(https://github.com/bitcoin/bitcoin/pull/30451#discussion_r1691348558)
If I am reading this right, this sets OBJC vars for non-darwin hosts. Is that intended? I would have expected the opposite.

🤔 instagibbs reviewed a pull request: "test: add creating/spending validity checks for rare output scripts"
(https://github.com/bitcoin/bitcoin/pull/30481#pullrequestreview-2199143485)
~0 on this file tbh

it's kind of a hodgepodge of things being tested that might already have functional test coverage, like future witness spends which is an intended upgrade hook and should be well-tested?

If they already don't have functional coverage maybe there are better spots for each?

💬 instagibbs commented on pull request "test: add creating/spending validity checks for rare output scripts":
(https://github.com/bitcoin/bitcoin/pull/30481#discussion_r1691351307)
not sure it's worth the effort for such a niche thing that already has unit test coverage