💬 sipa commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2704741455)
I think the goal should be that there is as little "unexpected inconsistency" between builds. Our release (guix) binaries are created with -O2, and most of our testing is with -O2 I believe, so I think the goal should be that people don't get an -O3 build unless they specifically want that. If cmake "Release" builds create -O3 binaries by default, and there are developers out there who set "Release" by default to build binaries that are used in production, that's perhaps undesirable if we don't
...

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1983946363)
done

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1983947215)
done

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1983947384)
done

💬 ariard commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2704794077)
@pinheadmz Sure, the conceptual aspects of this PR are better discussed on the mailing list or delving or even on nostr with no moderators. Generally, this is up to the PR author to indicate on which communication channel, they have posted the conceptual motivation of a code change for discussion. Here we have a code change motivated by other arguments (bundle OP_CSFS, OP_CAT) which are not mentioned on the Delving Bitcoin thread itself, and as such making the conversation on the technical ratio
...

💬 instagibbs commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1983963718)
You should add a test for this, but I'm pretty sure version 0 was never standard for relay.

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2704807320)
Updated for @achow101's suggestion, added a commit by @l0rinc to take his review feedback, and also updated `.gitignore` in `/contrib/seeds`. However, 2 commits that I have locally seem to be missing here on GitHub from the latest push.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2704807684)
I added a change to make sure that the most recent best block is written to disk on wallet unload. However, it seems like this uncovered some weirdness in the wallet unloading on shutdown. In particular, `StopWallets()` was being called before `UnloadWallets()` which I think is incorrect. I added a commit which flips this order, and it seems to work, although there may be side effects I have not discovered yet. This primarily affects BDB wallets as the shutdown for BDB wallets is much more invol
...

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2704834143)
Thanks for the reviews @l0rinc and @achow101. Updated to take @achow101's suggestion, added a commit by @l0rinc with his review feedback (makeseeds output appears unchanged by the robustness/consistency changes to the regexes), updated `.gitignore` in `/contrib/seeds`, and re-ran the scripts.

💬 reardencode commented on pull request "Implement OP_CHECKSIGFROMSTACK(VERIFY)":
(https://github.com/bitcoin/bitcoin/pull/29270#issuecomment-2704843854)
@fanquake please reopen this PR.

I've rebased on master and ensured that tests pass locally.

#29198 was closed because this (and other PRs I opened subsequently) superseded it by splitting its contents up into smaller units.

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2704847903)
Re-pushed to add a missing escape to the regexes commit.

👍 ismaelsadeeq approved a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2665558763)
Code review ACK 72a97c0a07ea6e5a95ab37c8d95e1ea02cff8e92 🛸

I've reviewed each commit and also looked at how the interface is being used by `CTxMemPool` in the big PR

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2665561910)
Approach ACK will review in-depth soon

💬 darosior commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2704884264)
The code in this pull request is of good quality and is well tested, but it's not clear to me what's the goal with this pull request nor how it fits in the big picture. As this introduces a new consensus feature, it seems premature to merge this into Bitcoin Core until widespread consensus is reached among Bitcoin users to eventually activate this new feature in a soft fork.

To be clear, i am not saying the details of the activation need to be figured out before the implementation of the feat
...

💬 achow101 commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2704913533)
Needs rebase

💬 jonatack commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2704915598)
Rebased

💬 achow101 commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#issuecomment-2704952974)
ACK 89fd9a4a87d772798671da18538e45f0272365e4

💬 davidgumberg commented on pull request "RFC contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2704999209)
Concept ACK, while it might not be strictly necessary for this tarball to be deterministic, I think matching hashes of tarballs is the simplest way to verify that you've followed the same procedure as others to build the macOS sdk and have gotten the same result.

150MB seems to me a small price to pay to fix #31873 and reduce surface area for nondeterminism in generating the macOS sdk.

💬 l0rinc commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984087810)
For consistency this can also be:
```suggestion
PATTERN_ONION = re.compile(r"^([a-z2-7]{56}\.onion):(\d{1,5})$")
```

💬 l0rinc commented on pull request "seeds: add signet/testnet4, update makeseeds regex, minblocks, fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/31960#discussion_r1984089787)
1) the `chainparams: add signet fixed seeds if default network` commit should go after `seeds: update fixed dns seeds` which introduces `chainparams_seed_signet`
2) Now that we're pre-clearing it at the beginning, it may be slightly simpler to do:
```suggestion
vFixedSeeds.assign(std::begin(chainparams_seed_signet), std::end(chainparams_seed_signet));

```