💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1687121805)
`iterations` is not modified here, so you're just reporting 0 iters used everywhere, and always PostLinearizing

Do we want to change the `Linearize` signature to report iters left, which can infer optimal or not as well?

💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2245563392)
> One thing that may be useful to clarify, IIUC, although you can connect, nothing can be done via these connections yet. That is what #30510 introduces specifically for the Mining interface.

Thank you, added this to the description

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2245579974)
re-ACK 09ce3501fa2ea2885a857e380eddb74605f7038c 🕓

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 09ce3501fa2ea2885a85
...

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2245586329)
Is the idea to incorporate the interface changes proposed in #30440 and #30409 after they're merged? And in the mean time keep them in #30437?

💬 Sjors commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#issuecomment-2245622810)
There's discussion in #29770 about whether to use some or all the things in this PR. Can you give it a rebase?

💬 Sjors commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2245638221)
In the unlikely event of such a deep reorg I think it's acceptable if the user _might_ need to restart their node in order to follow the new longest headers.

💬 maflcko commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#issuecomment-2245641239)
Upgrading my review with a test:

* `FUZZ=utxo_snapshot /usr/bin/time --format='%MKB' ./src/test/fuzz/fuzz -runs=1 ../b-c-qa-assets/fuzz_seed_corpus/utxo_snapshot/` gives: 2MB less peak memory (~1%) and 600 less `cov` (~10%) (which is desired here, to avoid reporting non-fuzzing coverage in a fuzz target) and no speedup/slowdown
* Same for `FUZZ=coins_view` gives: 50MB less peak memory (~25%) and no other change

lgtm

📝 fanquake opened a pull request: "guix: GCC 12 consolidation"
(https://github.com/bitcoin/bitcoin/pull/30511)
This PR contains 3 changes:

* Bump GCC in Guix from [12.3.0 to 12.4.0](https://gcc.gnu.org/gcc-12/). A patch was sent upstream, https://lists.gnu.org/archive/html/guix-patches/2024-06/msg01025.html, but has not landed.
* Consolidate all build environments back to using a GCC 12 toolchain. After #21778, the macOS environment is no-longer pinned to 11 (12 would otherwise cause issues building cctools). So, instead of requiring all builders to compile an additional GCC toolchain, use 12.
* Use
...

👍 dergoegge approved a pull request: "test: Add arguments for creating a slimmer TestingSetup"
(https://github.com/bitcoin/bitcoin/pull/30399#pullrequestreview-2194451669)
utACK f46b2202560a76b473e229b77303b8f877c16cac

💬 dergoegge commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#discussion_r1688346831)
nit: A bit weird that this will be available as an option on all testing setups when only `TestingSetup` and it's derived classes care about it.

💬 hebasto commented on pull request "contrib: simplify `test-security-check`":
(https://github.com/bitcoin/bitcoin/pull/30423#issuecomment-2245682275)
My Guix build:
```
x86_64
222a30ae2094e1e2536e5a1e8794d04f300fbf6ed0f67e0f6dcb4160e3d075a7 guix-build-1bc9f64bee91/output/aarch64-linux-gnu/SHA256SUMS.part
f7a7b2a6f7e85aa633d6cdb9907bf11cf05c3bdabdf32a4fefaf2bb63264583d guix-build-1bc9f64bee91/output/aarch64-linux-gnu/bitcoin-1bc9f64bee91-aarch64-linux-gnu-debug.tar.gz
8884a25e5a214b224d2c7d73e19786174ddb4eed37c2daa0dc57c23d1f49fc3c guix-build-1bc9f64bee91/output/aarch64-linux-gnu/bitcoin-1bc9f64bee91-aarch64-linux-gnu.tar.gz
36855764c
...

💬 maflcko commented on pull request "test: Add arguments for creating a slimmer TestingSetup":
(https://github.com/bitcoin/bitcoin/pull/30399#discussion_r1688353703)
I did this on purpose when adding `TestOpts`, because deriving a nested stack of `TestOpt` classes would make designated initializers harder, something like `{ { { .inner = true } }, .outer = false}`, instead of just `{ .inner = true , .outer = false}`. (Not sure if there were also compiler warnings in the first case)

👋 fanquake's pull request is ready for review: "cleanse: switch to SecureZeroMemory for Windows cross-compile"
(https://github.com/bitcoin/bitcoin/pull/26950)

💬 ismaelsadeeq 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_r1688371478)

Is there something that will prevent us from deriving `PayToAnchor` from `WitnessUnknown`?

```c++
struct PayToAnchor: public WitnessUnknown
{
PayToAnchor() : WitnessUnknown(1, {0x4e, 0x73}) {};
friend bool operator==(const PayToAnchor& a1, const PayToAnchor& a2) {
return true;
}
friend bool operator<(const PayToAnchor& a1, const PayToAnchor& a2) {
return true;
}
};
```

This way we can reuse the `WitnessUnknown` operator() that should be reus
...

💬 ismaelsadeeq 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_r1688246363)
Why is this false, shouldn't this be true also?

💬 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_r1688376618)
since they're always equal, I suspect they can never be unequal? Perhaps I'm violating some C++ custom here?

🤔 glozow reviewed a pull request: "cluster mempool: cluster linearization algorithm"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2191951101)
ACK cad318fa843

I did code review to convince myself that the code does what it's supposed to be doing / what's described in the delving post. Everything is a bit abstract so I manually tested by writing some naive versions of `BlockAssembler` and `MiniMiner` using cluster_linearize. I also code reviewed the tests and introduced bugs to check their coverage. I didn't really dive into `DepGraphFormatter` but it seems to work.

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688140732)
I suppose this is the same as `chunking.Intersect(intersect)` except without exiting early?

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1686776442)
2597f096afde07d9c7f6466c0970a73947562be3

nit: found this comment hard to parse. "Remove transactions from the chunking object, and check various..." ?

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1688134395)
There wouldn't be any problem with resetting `subset = SetInfo(depgraph, ReadTopologicalSet(depgraph, todo, reader));` here, right?