💬 darosior commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2663476018)
Another thing worth mentioning is that the Liana GUI would download and start a `bitcoind`. This means a notarized application can run non-notarized binaries just fine. This may be helpful to know in considering how we approach notarization here (and in a possible multiprocess future).
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2663476018)
Another thing worth mentioning is that the Liana GUI would download and start a `bitcoind`. This means a notarized application can run non-notarized binaries just fine. This may be helpful to know in considering how we approach notarization here (and in a possible multiprocess future).
💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2663481343)
> I wonder if it may be easier to just disable the MP configure option when compiling for fuzzing
Not sure I understand the suggestion. Is it to not set ENABLE_IPC in the CI configuration? Show an error in the build when ipc and fuzzing are enabled together? Ignore the ipc option when fuzzing is enabled?
The bugfix here is simple, even though the bug is complicated to explain. It builds libraries with `-fsanitize=fuzzer-no-link` which is how libraries should be built, and builds the fuzz
...
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2663481343)
> I wonder if it may be easier to just disable the MP configure option when compiling for fuzzing
Not sure I understand the suggestion. Is it to not set ENABLE_IPC in the CI configuration? Show an error in the build when ipc and fuzzing are enabled together? Ignore the ipc option when fuzzing is enabled?
The bugfix here is simple, even though the bug is complicated to explain. It builds libraries with `-fsanitize=fuzzer-no-link` which is how libraries should be built, and builds the fuzz
...
💬 hebasto commented on issue "qa: `wallet_importdescriptors` failure "TypeError: 'bool' object is not subscriptable"":
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2663497225)
Another one: https://github.com/bitcoin/bitcoin/actions/runs/13373718736/job/37348002275
(https://github.com/bitcoin/bitcoin/issues/31881#issuecomment-2663497225)
Another one: https://github.com/bitcoin/bitcoin/actions/runs/13373718736/job/37348002275
💬 hebasto commented on pull request "cmake: Fix `-pthread` flags in summary":
(https://github.com/bitcoin/bitcoin/pull/31724#issuecomment-2663500472)
> Fixes #31482.
The issue been fixed is assigned to the "29.0" milestone.
Should the milestones for an issue and a PR with its fix be aligned?
(https://github.com/bitcoin/bitcoin/pull/31724#issuecomment-2663500472)
> Fixes #31482.
The issue been fixed is assigned to the "29.0" milestone.
Should the milestones for an issue and a PR with its fix be aligned?
💬 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?
(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.
(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
...
(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
(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.
(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.
(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.
(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
...
(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)
****
(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?
(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
...
(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?
(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.
```
(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.
(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_
...
(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)`.
(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)`.