🤔 ryanofsky reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2711577943)
@Prabhat1308 this seems ready to merge but also has a few recent comments that haven't been responded to. You can let me know if you'd like this to be merged or if you want to make more changes.

👍 hodlinator approved a pull request: "build: Remove bitness suffix from Windows installer"
(https://github.com/bitcoin/bitcoin/pull/32132#pullrequestreview-2711591169)
ACK fb2b05b1259d3e69e6e675adfa30b429424c7625

We still have things like
x86_64-w***64***-mingw***32***/bitcoin-94967c353ed8-win***64***-setup-unsigned.exe
in build output, but I understand wanting to defer those until later.

🤔 marcofleon reviewed a pull request: "fuzz: coinselection: cover `SetBumpFeeDiscount`"
(https://github.com/bitcoin/bitcoin/pull/31806#pullrequestreview-2711621352)
ACK 0ff66b1c4ab0e5cba00a178b24f2c5de75df360f

Checked for the additional coverage to be sure, lgtm

💬 jsarenik commented on issue "Failed transactions on importing mempool":
(https://github.com/bitcoin/bitcoin/issues/32125#issuecomment-2749337486)
Another example of what I was thinking of in the beginning and what is already possible using the script that prioritizes transactions before importing them:

```
newlines added
2025-03-24T20:31:45Z
Imported mempool transactions from file:
1267 succeeded, 0 failed, 0 expired, 29536 already there,
0 waiting for initial broadcast
```

One thing I can think of right now would be an RPC which clears all prioritization. No idea how to do it in Bitcoin Core yet, but the idea is here.

💬 murchandamus commented on issue "BnB untested/unused condition in UTXO exclusion optimization":
(https://github.com/bitcoin/bitcoin/issues/32047#issuecomment-2749339392)
> That would be great to update BnB to be more like coin-grinder. Adding an iteration count would be a good first step towards that.

I don’t think it makes sense to add just the iteration count, if it should be refactored completely anyway.—Better to do it all in one.

> > I could have sworn we had such a test already, but if we don’t that would be a great addition to the [bnb_feerate_sensitivity_test](https://github.com/bitcoin/bitcoin/pull/29532/files#diff-36088c93368e137d955348aba223985bd4f1
...

💬 davidgumberg commented on pull request "build: Drop option to disable hardening.":
(https://github.com/bitcoin/bitcoin/pull/32071#discussion_r2010894527)
Thanks, fixed

🚀 ryanofsky merged a pull request: "net: Block v2->v1 transport downgrade if !fNetworkActive"
(https://github.com/bitcoin/bitcoin/pull/32073)

💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2749384672)
@ryanofsky, thanks again for the review.
Your suggestion seems to point back to the initial implementation of this PR, i.e. https://github.com/bitcoin/bitcoin/pull/31539/commits, where we didn't know the full size of the block, and read and wrote in chunks.
This current alternative was explicitly suggested by @theuni - and while it's a bit messy to always read the size, the resulting code is faster and seem to have fewer moving parts (e.g. doesn't reassign via the parameter, doesn't copy the w
...

💬 theStack commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010928029)
For segwitv0, program sizes other than 20 or 32 are indeed consensus-invalid (see function [VerifyWitnessProgram](https://github.com/bitcoin/bitcoin/blob/b3162d10ea93cdec8e04150289ff61d6fed9a8e9/src/script/interpreter.cpp#L1900-L1902)), so the comment is correct. And yes, for segwitv1+, not-yet-defined programs are only non-standard, but consensus-valid:
https://github.com/bitcoin/bitcoin/blob/a0d737cd7a7fafe2c743360c4ac7b2e72664e5c5/src/script/interpreter.cpp#L1952-L1953

💬 theStack commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010929617)
Will apply if I have to retouch.

💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2010931145)
Could this be part of #32096?

Another part is probably better to change at least as early:
https://github.com/bitcoin/bitcoin/blob/b3162d10ea93cdec8e04150289ff61d6fed9a8e9/doc/bitcoin-conf.md?plain=1#L27

Probably better to use "regtest=1" or "signet=1".

💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2010933670)
Noticed while looking at #32096 that this file was untouched there.

Testnet3 has ***rpc***port=18332 according to how chainparamsbase.cpp sets `CBaseChainParams(..., rpc_port)`...
This config is setting the P2P ports for all networks so that they will collide with the default RPC ports and prevent successful startup (verified with the mainnet config line). Might as well just remove all ports IMO.

💬 ryanofsky commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2749457567)
> @ryanofsky, thanks again for the review. Your suggestion seems to point back to the initial implementation of this PR, i.e. https://github.com/bitcoin/bitcoin/pull/31539/commits

Absolutely yes. There are a few things I would change about that PR but overall it seems a lot cleaner and simpler to me than this PR. If it doesn't perform as well as this one that is surprising, because I thought the performance improvement here came from buffering, not from choosing buffers with magic sizes. But
...

🚀 ryanofsky merged a pull request: "doc: clarify that testnet min-difficulty is not optional"
(https://github.com/bitcoin/bitcoin/pull/32095)

👍 hodlinator approved a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2711717129)
Code Review ACK 8284229a28c09c585356dcf7e4bddbc8f2a23755

Adds basic tests for WitnessV1Taproot & (WitnessV1)PayToAnchor outputs.

Only non-test change is replacing array literal with `std::vector` (which sort of makes sense considering `struct PayToAnchor` (ab)uses `WitnessUnknown` as a base (predating this PR) which takes `vector` as a constructor input).

💬 hodlinator commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010951768)
Thanks for the references! I'm still unfamiliar with that part. So for `MANDATORY_SCRIPT_VERIFY_FLAGS` we reach the return true there for Taproot. :+1:

💬 ryanofsky commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2749480675)
Rebased 26c2136a2bc7fec9697f61eadf422c439e0735a2 -> c9a49633a027f1c28ebe15c0134f1ce38b712e8a ([`pr/ipc.216`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.216) -> [`pr/ipc.217`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc.217), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc.216-rebase..pr/ipc.217)) due to silent conflict with #31519

💬 sipa commented on pull request "contrib: document asmap-tool commands more thoroughly":
(https://github.com/bitcoin/bitcoin/pull/32110#discussion_r2010972739)
Well if the asmap is reasonably up to date, I suspect there will basically be ~no peers in unmapped portions. And if there are, if unfilled it'll use /16 grouping, and if filled it'll use an adjacent ASN. I couldn't say which would be better in general among those.

💬 bigspider commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2010999772)
Thanks for checking out the demo!
For the naming, I tried to use names similar to BIP-345, except that I name the "Vault" and "Unvaulting" state of the FSM which seems more natural in CCV-lingo (where you define the multi-utxo contract as a finite-state-machine).

> Can `alternate_pk` and `recover_pk` just be the same? That way there's only two key sets needed for a vault: one hot, one cold. And you're avoiding the use of a NUMS point.

You could, but I don't think that makes a lot of sense
...

💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2010985584)
Why not move this to #32096? (23607f605c3156ca904f704666f7cbe4808cf76b there touches the entry from the vaguely related `MAGIC_BYTES`).

Why not have a "testnet4" entry?