💬 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

💬 hebasto commented on pull request "guix: GCC 12 consolidation":
(https://github.com/bitcoin/bitcoin/pull/30511#issuecomment-2250206015)
My Guix build:
```
4fba0aba8be59fd967b099e19faf6ba6770a83de1e7bee745c16f91fa3eb92fe guix-build-d1592d2eee19/output/aarch64-linux-gnu/SHA256SUMS.part
37e93c9235db88f58e5a45e8a54e1d57602f9107cd23676c91252509b2fe3bb5 guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d1592d2eee19-aarch64-linux-gnu-debug.tar.gz
184fb61be0843b069c4c0d92119b9e29e895f11ef8687b220ab3898999c06afa guix-build-d1592d2eee19/output/aarch64-linux-gnu/bitcoin-d1592d2eee19-aarch64-linux-gnu.tar.gz
1db81362865b56197
...

📝 maflcko opened a pull request: "doc: rpc: Use "output script" consistently (2/2)"
(https://github.com/bitcoin/bitcoin/pull/30524)
Small follow-up to https://github.com/bitcoin/bitcoin/pull/30408 to fixup the three RPCs that were forgotten.

💬 maflcko commented on pull request "rpc: doc: use "output script" terminology consistently in "asm"/"hex" results":
(https://github.com/bitcoin/bitcoin/pull/30408#issuecomment-2250209869)
> Affects the help text of the following RPCs:

Looks like 3 RPCs were forgotten, so I created https://github.com/bitcoin/bitcoin/pull/30524

💬 maflcko commented on pull request "rpc: Return errors in loadtxoutset that currently go to logs":
(https://github.com/bitcoin/bitcoin/pull/30497#issuecomment-2250213872)
Rebased. Should be trivial to re-ACK with `git range-diff bitcoin-core/master fa5c253b4e fa91404c68`

💬 willcl-ark commented on pull request "doc: add release notes for #22729":
(https://github.com/bitcoin/bitcoin/pull/30502#discussion_r1691365458)
There's some duplication in the first three sentences (repeating that we bind to 127.0.0.1:8334) that can perhaps be removed.

Additionally, not mentioned is the new startup behaviour where we abort on failure to bind to any P2P port.

Suggestion:

```suggestion
- Previously if Bitcoin Core was listening for P2P connections, either using
default settings or via `bind=addr:port` it would always also bind to
`127.0.0.1:8334` to listen for Tor connections. It was not possible to switch
...

👍 hebasto approved a pull request: "guix: GCC 12 consolidation"
(https://github.com/bitcoin/bitcoin/pull/30511#pullrequestreview-2199185214)
ACK d1592d2eee1913e734a4f92907e796eb3350c64a.

👍 fanquake approved a pull request: "depends: Cleanup postprocess commands after switching to CMake"
(https://github.com/bitcoin/bitcoin/pull/30506#pullrequestreview-2199187950)
ACK a0314c151679a348d842b68c5ecb7a556700811c
Guix build (aarch64):
```bash
a35aa8777223dbb7fdb3b9f600067303a5846c26a854d7d5aaa41b5fe12a047f guix-build-a0314c151679/output/aarch64-linux-gnu/SHA256SUMS.part
917c0ebbb4c8cbddc6e314a2f6bcee22d9399356b56ecdecd6fb008882a6eb48 guix-build-a0314c151679/output/aarch64-linux-gnu/bitcoin-a0314c151679-aarch64-linux-gnu-debug.tar.gz
884d1264b2d8bcb2975c61248f5794fba26c60deb445a07f12b69c17db8c4cc6 guix-build-a0314c151679/output/aarch64-linux-gnu/bitcoin
...

💬 fanquake commented on pull request "depends: remove Darwin ENV unsetting":
(https://github.com/bitcoin/bitcoin/pull/30451#discussion_r1691383416)
Heh, yea. These should/would only be needed for macOS, and only for Qt-related code. I guess everything still currently builds even with them unset, because when we compile our objc++ code, we would never be looking in any GCC related include/std-lib/related dirs in any case? Will push some changes.

🚀 fanquake merged a pull request: "rest: Reject truncated hex txid early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30482)

🚀 fanquake merged a pull request: "test: Add arguments for creating a slimmer TestingSetup"
(https://github.com/bitcoin/bitcoin/pull/30399)

🚀 fanquake merged a pull request: "guix: GCC 12 consolidation"
(https://github.com/bitcoin/bitcoin/pull/30511)