🤔 hodlinator reviewed a pull request: "refactor: Convert GenTxid to `std::variant`"
(https://github.com/bitcoin/bitcoin/pull/32631#pullrequestreview-2916032256)
Code review d5a6e7a0a40bb78c2fcadd2662f2d64b86c374b7

Thanks for incorporating most of my feedback!

Mostly focused on reviewing non-test code.

💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2139469824)
nit in a105a10d71c266cf7971fc66109c9ac8558e6d60:
Could avoid copy as `it` will keep the instance alive.
```C++
const GenTxidVariant& hash = *it;
```

💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2139451786)
Yeah, makes sense to draw the line somewhere. (Noticed I had accidentally omitted the last 30 lines of the diff, fixed FWIW).

💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2139485731)
(nit in ab954a169f07c86f85e292d3f488df0cc066b722: Happy to see Freemasons have embraced Bitcoin. Might be neater to have 32 spaces for a multiple of 4 instead of current 33).

💬 hodlinator commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2139967356)
So for segwit peers we use `Wtxid` and for non-segwit peers we use `Txid`?
Scary that the `variant` fails comparison although the hash is the same, so we have to manually find places where it shouldn't matter and do this kind of thing. :\

Might be easier to ACK this PR if you peel off txrequest.cpp/h into it's own PR.

💬 scott-weeden commented on pull request "Add initial OpenAPI/Swagger specification for Bitcoin Core RPC and REST interfaces (issue #29912)":
(https://github.com/bitcoin/bitcoin/pull/32728#issuecomment-2962509457)
The description of the pull request is an AI summary of the issue, the code is not. I did not have the test case properly wired in with the rest, I will close this pull request and make sure class TestOpenAPICodegen(unittest.TestCase) is running correctly.

Also the build/test dependency can be easily installed with npm but I'm not sure the CI build process has it available.

✅ scott-weeden closed a pull request: "Add initial OpenAPI/Swagger specification for Bitcoin Core RPC and REST interfaces (issue #29912)"
(https://github.com/bitcoin/bitcoin/pull/32728)

🤔 instagibbs reviewed a pull request: "test: Fix reorg patterns in mempool tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-2916996532)
LGTM c98f3615e7eb6774b8f49fe7b2aeed8c056622c7

feel free to add more cases

💬 instagibbs commented on pull request "test: Fix reorg patterns in mempool tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2140044139)
could assert that the node's new chaintip indeed is the last submitted block

💬 TheCharlatan commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2140050478)
I'm not sure about adding all these tests. Are they actually improving coverage? I see some extra coverage in the corecheck report for the duplicate case, but that does not seem to warrant this entire test, which I guess is trying to cover a broad selection of possible failure cases, though there seems to be a large overlap with `mining_basic.py`. Maybe I am missing something? The existing `mining_basic.py` test is also already documented as being intended to exercise template proposals.

💬 ryanofsky commented on pull request "depends: fix cmake compatibility error for freetype":
(https://github.com/bitcoin/bitcoin/pull/32693#issuecomment-2962595286)
> but I don't know enough about `cmake` to know why the policy really exists in the first place.

Cmake just uses policies to be able to implement bugfixes and feature improvements in new cmake versions while providing a way to back backwards compatible with old versions, and attempting to be backwards compatible by default if a project does not explicitly enable recent policies. There is a long debate about which policy version is best to set in https://github.com/bitcoin-core/libmultiprocess
...

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2962598663)
> Can you update the PR description to reflect the current state of the PR, it seems like it describes this as one of multiple approaches, and mostly talks about a different approach?

Right, fixed.

> Looking through the changes here, I can't see any documentation about where the .dat comes from, who's responsible for generating it (what's required determinism wise), how often it needs updating (who's resposible for that, as well as maintaining any relevant tooling, and where that lives), h
...

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2140085665)
The existing coverage probably stems from the standard check after we generate our own block? But those are always valid. The checks here are for block templates from untrusted sources.

I would imagine we'll expand them later once sv2 pools start actually receiving things from users in the wild. And perhaps we'll need to add additional rejection reasons like a max validation time, that don't apply to our own blocks.

💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2140091166)
It could make sense to depulicate things by moving a few things out of `mining_basic.py` here.

🤔 marcofleon reviewed a pull request: "test: refactor: overhaul (w)txid determination for `CTransaction` objects"
(https://github.com/bitcoin/bitcoin/pull/32421#pullrequestreview-2917122093)
code review ACK 4ef6253017672a74c584e6e9b449ffff79f67273

Lgtm, nice refactor. Removing the cached hash makes the class cleaner and there's less potential for misuse. The new naming is a clear improvement as well. I ran all the functional tests to confirm, all good.

💬 TheCharlatan commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2140143326)
Most of the cases tested here are also triggered in `mining_basic.py`, so maybe add the few extra ones here to over there? There is also a valid block test right at the beginning there, which implicitly checks that no state change happened.

💬 theuni commented on pull request "Refactor: Redefine CTransaction equality to include witness data":
(https://github.com/bitcoin/bitcoin/pull/32723#issuecomment-2962726743)
> `CWalletTx::IsEquivalentTo` needs to consider malleated txs as equivalent, so the original behavior of ignoring the scriptSigs and scriptWitness is intended.

@achow101 Thanks for confirming. Indeed there should be no functional change here from the original behavior. It currently works by virtue of the equality operator comparing txids, but that's not _at all_ obvious just from looking at `IsEquivalentTo`. I figured it just kinda accidentally kept working correctly post-segwit, but I guess
...

💬 pithosian commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2962870847)
> But technically, there is no need to structure things in that way. If you can create equivalent proofs, that a chain of signatures was correct

You aren't just validating signatures when you validate the chain.

> Being able to store data forever does not scale. Sooner or later, if blockchain will be too bloated, there will be more and more nodes, working on proofs, instead of providing all historical data for everything.

Existing pruning is a better (though still bad) solution.

> If
...

💬 hebasto commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2963030000)
My Guix build:
```
aarch64
8ee8f51264bd5f28d838207486f83db69a1dd4d00eeda009f35ef9ebff794d0f guix-build-4f56c9145a60/output/aarch64-linux-gnu/SHA256SUMS.part
aaf5a45736f062df340bbd434295a087f90897401a8d9e1489f5ad56c4a82fab guix-build-4f56c9145a60/output/aarch64-linux-gnu/bitcoin-4f56c9145a60-aarch64-linux-gnu-debug.tar.gz
d6b4a08a7e9dad84dcfdc504896cf0b13a4c2c416ef8608903cd542297936f74 guix-build-4f56c9145a60/output/aarch64-linux-gnu/bitcoin-4f56c9145a60-aarch64-linux-gnu.tar.gz
c236ea00
...

🤔 hebasto reviewed a pull request: "deps: Bump lief to 0.16.6"
(https://github.com/bitcoin/bitcoin/pull/32431#pullrequestreview-2917471636)
Post-merge re-ACK 4f56c9145a60c4fb837f11e47c5aa39ad8fa3523.