💬 maflcko commented on issue "ci: failure in Windows cross-test":
(https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2814631853)
The error message is a bit confusing, because the benchmarks should be a single process? Thus, it seems odd that another process was accessing the wallet file.

I don't have Windows to debug this, so I guess it is best to temporarily revert. A follow-up can fix the underlying issue and enable the CI check again.

📝 maflcko opened a pull request: "ci: Temporarily revert "Drop bench -priority-level in win cross CI""
(https://github.com/bitcoin/bitcoin/pull/32302)
This reverts commit 27f11217ca63e0f8f78f14db139150052dcd9962.

The commit was nice and useful. However, CI doesn't pass, see https://github.com/bitcoin/bitcoin/issues/32291. Temporarily revert it, so that it can be enabled again along with the issue fixed.

💬 maflcko commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2050124932)
> Before the fix this also fails with the following (built without sanitizers, doesn't fail in Release mode):

That's -ftrapv, added in 2643fa50869f22672cbc72ac497d9c30234075b8

💬 maflcko commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#issuecomment-2814647839)
lgtm ACK 5cb1241814b9c541c5e5e8daadbbea595f2960ae

💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2814808645)
This or a followup needs to update `doc/dependencies.md` and update the build instructions to change opt-in wording to opt-out. I'll update that here if https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813734342 lands earlier.

💬 Sjors commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2050262297)
I made a note to add it.

💬 Sjors commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2814813703)
re-ACK 7f5a35cf4b31f176ff9138d21f3ec677ae7d1bcf

💬 l0rinc commented on pull request "[IBD] prevector: allocate `P2WSH`/`P2TR`/`P2PK` scripts on stack":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2814858797)
A few previous measurements were done on `34` byte prevector, the latest version bumped it to `36` (which included P2PK scripts on stack as well).
I've rerun the `AssumeUTXO`, `reindex-chainstate` and `IBD` benchmarks (both on *SSD* and *HDD*, since they have different performance profiles) and couldn't find a single instance where `master` was any faster.

> this provides an improvement for nodes with default dbcache

@achow101, what other usecases do you think I should measure to make sur
...

💬 janb84 commented on pull request "doc: Add note for building on macOS (Intel) with CMake ≥ 4.0":
(https://github.com/bitcoin/bitcoin/pull/32289#issuecomment-2814877177)
> > ... that should probably be fixed _properly_, in some way.
>
> I haven't found a way to do it that covers all use cases, is user-friendly, and doesn't introduce excessive complexity.

Maybe one "solution" is not to lean only on homebrew but offer also an alternative package manager e.g. nix ? (nix is still on cmake 3.31 so not sure what will happen when they are on 4.0 )

💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2814926794)
re-ACK 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a

Main changes since my last review:
- commit 3841da0f62fa5e26f679a12c6efbafe1cb17c25f to preserve PSBT GUI coverage
- #32214 landed which made 3841da0f62fa5e26f679a12c6efbafe1cb17c25f and 1ffea3fba3b22c40bbcc44cd7c3a637e0450315a smaller
- #31866 landed which required some some changes in 7dbf06c33508820486eed59aba78d852c6212864

💬 medobrens83 commented on issue "Bitcoin Kernel Library Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27587#issuecomment-2814945804)
👍👍🏻💯

💬 l0rinc commented on pull request "ci: Temporarily revert "Drop bench -priority-level in win cross CI"":
(https://github.com/bitcoin/bitcoin/pull/32302#issuecomment-2815018301)
ACK fadccd9e4ab29fa703dd5a6f42fae030176a86f3

Build passes now, we will have to rebase a few PRs after this so it makes sense to merge it ASAP.

💬 l0rinc commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2815039995)
Rebased, cherry picked https://github.com/bitcoin/bitcoin/pull/32302 to fix unrelated CI failure and added back the `unsigned-integer-overflow:CCoinsViewCache` suppressions since it seems like a genuine error that I will have to tackle in a separate PR instead.

💬 fanquake commented on pull request "ci: Temporarily revert "Drop bench -priority-level in win cross CI"":
(https://github.com/bitcoin/bitcoin/pull/32302#issuecomment-2815107502)
Can we add a different change, to continue running all benchmarks (I guess just not Windows), so we aren't back to the same state that allowed #32277 to happen?

💬 fjahr commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2815218248)
re-ACK e261eb8d50c7192260a449e653453e63f59dbeed

CI failure is unrelated, see #32291

👍 theStack approved a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2778346130)
re-ACK e261eb8d50c7192260a449e653453e63f59dbeed
(as per `$ git range-diff debacd1f...e261eb8d`)

Verified that since my previous ACK, the changes didn't affect the logic, and were mostly about code deduplication (for deserializing participant pubkeys), getting rid of magic numbers, variable naming (s/key/pubkey/), and adapting the terminology to the [recent BIP-373 changes](https://github.com/bitcoin/bips/pull/1705) (plain public key -> compressed public key). LGTM.

💬 theStack commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2050497355)
nit: could add parentheses for better readability (here and below for `PSBT_IN_MUSIG2_PARTIAL_SIG`)
```suggestion
} else if (key.size() != (2 * CPubKey::COMPRESSED_SIZE + 1) && key.size() != (2 * CPubKey::COMPRESSED_SIZE + CSHA256::OUTPUT_SIZE + 1)) {
```

🤔 fjahr reviewed a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2778403152)
Code review ACK acee5c59e68f31acba111bc4d1892b08243ea5e0

💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r2050530082)
nit: could make `other` const if you retouch

💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#issuecomment-2815320668)
> LGTM, code review [acee5c5](https://github.com/bitcoin/bitcoin/commit/acee5c59e68f31acba111bc4d1892b08243ea5e0)

Was that an ACK an you just forgot the word or did you just want to say you reviewed? :)