💬 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?

💬 glozow commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1687853218)
Question: I whiteboarded what this graph + the decision tree looks like to try to understand what makes this a hard graph. Do I understand correctly that
- Within this PR, the feerates of transactions in the graph have no impact on how many iterations `SearchCandidateFinder` might run, since all work items are considered as long as they are topological.
- The feerates of the parent+child transactions {4..n-1} become important after #30286's optimizations that rule out transactions/branches tha
...

💬 Sjors commented on issue "RFC: Assumeutxo and large forks and reorgs":
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2245733622)
> The simplest way to do that might just be to explicitly hardcode the block hash of the alternate chain which forks from ours as invalid, which would immediately address both of these additional problems in addition to making assumeutxo work in this scenario.

That's effectively a (negative) checkpoint. Hopefully this heaver chain is invalid in such a way that we can detect it without context. E.g. perhaps it contains a 4.1MB block block (with less work that the real tip). In that case we wou
...

💬 Sjors commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2245743124)
Post merge concept ACK

💬 Sjors commented on pull request "rpc: Return errors in loadtxoutset that currently go to logs":
(https://github.com/bitcoin/bitcoin/pull/30497#issuecomment-2245749356)
Concept ACK

🤔 theuni reviewed a pull request: "cleanse: switch to SecureZeroMemory for Windows cross-compile"
(https://github.com/bitcoin/bitcoin/pull/26950#pullrequestreview-2194539139)
Concept ACK. Can't think of any reason why not, if mingw supports it.