🤔 jonatack reviewed a pull request: "test: add mocked Sock that can read/write custom data and/or CNetMessages"
(https://github.com/bitcoin/bitcoin/pull/30205#pullrequestreview-2461700619)
Light ACK 5766bbefa94d40f8d37639c2752961617d7a262a

Similar change to the larger one I reviewed in https://github.com/bitcoin/bitcoin/pull/26812.

💬 jonatack commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1858640053)
I also wondered why this was moved. Not moving it would be preferable to save reviewers' time.

💬 jonatack commented on pull request "test: add mocked Sock that can read/write custom data and/or CNetMessages":
(https://github.com/bitcoin/bitcoin/pull/30205#discussion_r1858648731)
2f6ce54212926f7f8642dfa5021f4ff089a4de61 Suggest not needlessly moving this to save reviewer time.

🤔 hodlinator reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2461686975)
Code review a3a4d199e298a76725d0c0424195ae54c7e95fe0

Great to test more aspects of the protocol!

Why is this being removed/changed?
https://github.com/bitcoin/bitcoin/blob/22ef95dbe3e467039e6cd18988e66557d94041d1/src/test/script_standard_tests.cpp#L271-L276

As I understand it we were creating a taproot output (v1) using a compressed pubkey with the type-prefix prepended, meaning it was 33 bytes instead of the standard 32. Maybe you could add it back with a clearer comment?

---

Pu
...

💬 hodlinator commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r1858636182)
Would prefer:
- Capitalizing constant as suggested above.
- Clearly associating the scope of the new constant with the witness type by putting it inside `struct PayToAnchor`.
- Using `constexpr` & `array` to minimize dynamic memory use.
- (Might as well send member data into `Assume()`).
```C++
struct PayToAnchor : public WitnessUnknown
{
/** Witness program for output script */
static constexpr std::array<unsigned char, 2> PROGRAM{0x4e, 0x73};

static constexpr unsigned i
...

👍 dergoegge approved a pull request: "build: Fix coverage builds"
(https://github.com/bitcoin/bitcoin/pull/31337#pullrequestreview-2461824497)
tACK 01a7298818d7bd66bb8c800d7cd30ae0a4d3b1d1

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2501064149)
@achow101 @ryanofsky @glozow Would appreciate if one of you could take a look :)

💬 jonatack commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2501079577)
Is this fix related to the "no coverage data" results in https://corecheck.dev, or is that a different issue?

💬 dergoegge commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2501117599)
> Is this fix related to the "no coverage data" results in https://corecheck.dev/ (i.e. https://corecheck.dev/bitcoin/bitcoin/pulls/31337), or is that a different issue?

I think the reason for "no coverage data" on corecheck's site for this PR is that this PR doesn't change any source files covered by any tests. It does show coverage changes under "Lost/Gained baseline coverage" but those are likely caused by non-determinism in the tests.

This PR was triggered by coverage builds on oss-fuz
...

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2501141395)
It might be useful if someone expands `doc/multisig-tutorial.md` to add a MuSig2 section. That doesn't have to go in this PR, but it will make testing and review easier. The functional test added in this PR can be used for inspiration.

💬 hebasto commented on issue "CMake `CheckPIESupported` doesn't always work":
(https://github.com/bitcoin/bitcoin/issues/30771#issuecomment-2501164856)
The upstream issue has been [fixed](https://gitlab.kitware.com/cmake/cmake/-/merge_requests/10034) for CMake 3.32.

💬 jonatack commented on issue "Prioritize processing of peers based on their CPU usage":
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-2501239124)
Concept ACK on potentially adjusting peers based on their resource usage, though the devil may be in the details. If I'm not misremembering, this has been brought up as an idea worth exploring since several years.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1858826260)
Yep, I'm planning to drop it

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1858826625)
I'm planning to simulate both cases and report back

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1858828041)
@naumenkogs can this be resolved?

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1858829100)
Can this be resolved?

🤔 darosior reviewed a pull request: "Testnet4 including PoW difficulty adjustment fix"
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2462021059)
One comment on the timewarp fix. It's OK in this specific case to have the check in `ContextualCheckBlockHeader()` since this is a new network and a previous version could not conceivably have let in an invalid header. However for a patch to an existing network, we should consider something akin to what Matt did in #15482 for the 64 bytes txs check. See also this comment in `ConnectBlock()`:
https://github.com/bitcoin/bitcoin/blob/f7144b24be09e7db2173a0b15d73f99a08b98118/src/validation.cpp#L243
...

💬 hebasto commented on pull request "cmake: Add `CheckLinkerSupportsPIE` module":
(https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2501271136)
The upstream [fix](https://gitlab.kitware.com/cmake/cmake/-/merge_requests/10034) has been mentioned in the comment.

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1858866659)
That commit is from an old revision

📝 furszy opened a pull request: "wallet: fix crash during watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/31374)
The crash occurs because we assume the cached scripts structure will not be empty,
but it can be empty for watch-only wallets that start blank.

Testing Notes:
This can be verified by cherry-picking and running the test commit on master.
It will crash on there but pass on this branch.