💬 fanquake commented on pull request "cmake: Fix `-pthread` flags in summary":
(https://github.com/bitcoin/bitcoin/pull/31724#discussion_r1958459118)
> generator expressions that evaluate to `-pthread` in this project.

So couldn't we just stop using `Threads::Threads` and use `-pthread` directly?

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2663514029)
Rebased and moved the Windows and OpenBSD comment to #31802.

💬 maflcko commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2663523653)
> Agree you would probably not fuzz test multiprocess code, but IPC code should be a good candidate for fuzz testing. With #29409, the FuzzedWallet could be passed a capnproto Chain interface instead of the default one, and IPC code could be used without even writing new tests.

Sure, but the ipc code is mostly just serialization/wrapper code, so if there is full coverage via non-fuzz tests, running objects in the fuzz tests through it doesn't increase coverage. For example, if the serialize c
...

💬 maflcko commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2663533427)
I haven't tried, but https://github.com/bitcoin/bitcoin/pull/31768#discussion_r1954481759 by @hodlinator may fix it

💬 maflcko commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2663539627)
It is using the coverage build type, which this issue is about. In any case, all mentioned projects are open-source, and I am happy to review pull requests, if there are any. Removing the cmake scripts may be fine, but I don't see it as a release blocker.

💬 furszy commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2663589075)
> I haven't tried, but [#31768 (comment)](https://github.com/bitcoin/bitcoin/pull/31768#discussion_r1954481759) by [@hodlinator](https://github.com/hodlinator) may fix it

It fixes it. Can test it by adding a small `sleep(3)` before the `getwalletinfo()` call.

But... this test is not deterministic. If the scanning process runs slower, the "0" duration check will also fail. I'm not sure we should keep it.

👍 theStack approved a pull request: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247#pullrequestreview-2621515920)
re-ACK 7064b1a45a54f4fae2cb390a444c8eca27410c88

Didn't check the individual test vectors in detail though; from what I can see the two corrections in the BIP (https://github.com/bitcoin/bips/pull/1769) don't affect this PR.

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2663597395)
It's definitely not true that if serialization code works in one case it works in all cases. Bitcoin serialization code does all kinds of weird things including [crashing](https://github.com/bitcoin/bitcoin/blob/9da0820ec55e136d5213bf5bb90c36499debc034/src/coins.h#L63) when it is in a state it doesn't feel like serializing. And IPC code in general seems like an natural place for fuzz testing. For example, writing a fuzz test to write bytes to a socket serving the `Echo` interface could expose po
...

🤔 sins921 reviewed a pull request: "doc: update dev note examples for CMake"
(https://github.com/bitcoin/bitcoin/pull/30739#pullrequestreview-2621549804)
****

🤔 Sjors reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2621093939)
Reviewed code for 9911ba82be9ec2243b86bb149d6ec8509d9717d7, but I still need to review the last two commits.

`rpc_psbt.py --descriptors` consistently fails for me in commit 121c8fb494df4e81566eb0706f7b9500a870ac85 "psbt: Check sighash types in SignPSBTInput and take sighash as optional", at "Check that non-witness UTXOs are removed for segwit v1+ inputs" with `assert not non_witness_utxo` at line 136.

But only on Ubuntu, not on macOS, so maybe a gcc vs clang thing?

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958439049)
In 121c8fb494df4e81566eb0706f7b9500a870ac85 "psbt: Check sighash types in SignPSBTInput and take sighash as optional": I'm a little worried that burying this in PSBT code will lead to a bug one day when a new soft fork allows e.g. a witness version 1 scriptPubKey of 40 bytes. And then instead of expanding `IsPayToTaproot()` a new helper is added and not used here.

I don't have a good suggestion though, other than maybe adding another helper like `CanSighashDefault()`. But that might be forgot
...

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958251025)
In c49cccf9aac2d4e783c633930ceca4534f05c710 "psbt: Return PSBTError from SignPSBTInput": would be useful if the commit describes this behavior change, especially since there's no test. It seems that previously `SignPSBTInput` failures were just ignored, and now they're not. But this wasn't a problem before?

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958525188)
In https://github.com/bitcoin/bitcoin/commit/293fb5d16b51cf259c9a8fe379954d1d9f303029 "psbt: Add sighash types to PSBT when not DEFAULT or ALL": this is a good place for the commit message:

```
// When an atypical sighash type is specified by the user,
// add it to the PSBT so that further signing can enforce sighash type matching.
```

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958481668)
In 121c8fb494df4e81566eb0706f7b9500a870ac85 "psbt: Check sighash types in SignPSBTInput and take sighash as optional": this makes the call to `RemoveUnnecessaryTransactions` with `*sighash_type` below dangerous.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958499227)
In 293fb5d16b51cf259c9a8fe379954d1d9f303029 "psbt: Add sighash types to PSBT when not DEFAULT or ALL" nit: is this really `else`-worthy?

This boolean juggling is really hard to read, try:

```cpp
// Allow SIGHASH_DEFAULT for non-taproot inputs, to support transactions
// that also have taproot inputs. CreateSig() will adjust it.
if (utxo.scriptPubKey.IsPayToTaproot() ? sighash != SIGHASH_DEFAULT :
(sighash != SIGHASH_DEFAULT && sighash != SIGHASH_
...

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958493444)
In https://github.com/bitcoin/bitcoin/commit/121c8fb494df4e81566eb0706f7b9500a870ac85 "psbt: Check sighash types in SignPSBTInput and take sighash as optional" nit: it would be slightly more clear if you move this comment line directly above `if (input.sighash_type && input.sighash_type != sighash)`.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958409095)
In 59186f20f24c8ab14ac34a69c61ed0c40793cea6 "wallet: change FillPSBT to take sighash as optional": I think I get it now, see comment on 293fb5d16b51cf259c9a8fe379954d1d9f303029 "psbt: Add sighash types to PSBT when not DEFAULT or ALL"

📝 yancyribbens opened a pull request: "refactor: remove redundant `for` constructs"
(https://github.com/bitcoin/bitcoin/pull/31891)
Remove redundant for loops. This refactor is a no-brainer.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958557261)
(although maybe just indenting the second line with one extra space would do the trick

📝 fanquake opened a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892)
This check is only used in release builds, where hardening should always be enabled. I can't think of a reason we'd want to silently skip these checks if hardening was inadvertently disabled.