💬 hodlinator commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1957199989)
(I'm happy *wallet_multisig_descriptor_psbt.py* tests using isolated nodes, but that also means there's less need for that in this test).
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1957199989)
(I'm happy *wallet_multisig_descriptor_psbt.py* tests using isolated nodes, but that also means there's less need for that in this test).
💬 hodlinator commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1957205213)
`M` should probably be lowercase to signal later mutation? Also is only used in this function.
```suggestion
m = 4 # starts as 4-of-4
```
(Could optionally make `self.N` local too if you passed it in as an argument to `create_multisig()`.
(https://github.com/bitcoin/bitcoin/pull/29156#discussion_r1957205213)
`M` should probably be lowercase to signal later mutation? Also is only used in this function.
```suggestion
m = 4 # starts as 4-of-4
```
(Could optionally make `self.N` local too if you passed it in as an argument to `create_multisig()`.
💬 maflcko commented on issue "ci: Intermittent failure "Could not resolve host: github.com"":
(https://github.com/bitcoin/bitcoin/issues/31889#issuecomment-2663408696)
As a temporary workaround, I started a terminal in the CI task https://cirrus-ci.com/task/5557162807656448 and typed `podman system reset`:
```
ci_worker_1738693519_026122670@core-ci-2:/tmp/cirrus-build-2385794995$ podman system reset
WARNING! This will remove:
- all containers
- all pods
- all images
- all networks
- all build cache
- all machines
- all volumes
- the graphRoot directory: "/home/ci_worker_1738693519_026122670/.loc
...
(https://github.com/bitcoin/bitcoin/issues/31889#issuecomment-2663408696)
As a temporary workaround, I started a terminal in the CI task https://cirrus-ci.com/task/5557162807656448 and typed `podman system reset`:
```
ci_worker_1738693519_026122670@core-ci-2:/tmp/cirrus-build-2385794995$ podman system reset
WARNING! This will remove:
- all containers
- all pods
- all images
- all networks
- all build cache
- all machines
- all volumes
- the graphRoot directory: "/home/ci_worker_1738693519_026122670/.loc
...
💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1958410398)
Indeed, I think I mixed this up at some point during a rebase. Thank!
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1958410398)
Indeed, I think I mixed this up at some point during a rebase. Thank!
🚀 fanquake merged a pull request: "cmake: Add `libbitcoinkernel` target"
(https://github.com/bitcoin/bitcoin/pull/31869)
(https://github.com/bitcoin/bitcoin/pull/31869)
💬 l0rinc commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1958426704)
Yes, but mostly with SSDs, not SD cards.
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1958426704)
Yes, but mostly with SSDs, not SD cards.
💬 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-2663468406)
> [bitcoin-096525e92cc2-arm64-apple-darwin.zip](https://github.com/user-attachments/files/18781660/bitcoin-096525e92cc2-arm64-apple-darwin.zip)
Tested this on a Mac M1. I could download it flawlessly through Firefox and perform most of IBD.
I tried downloading it through Safari and likewise other reviewers i encountered an error:
https://github.com/user-attachments/assets/4f244f13-93ba-4071-bca1-4232cb69b69e
The system language was set to French but i guess what matters here is the
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2663468406)
> [bitcoin-096525e92cc2-arm64-apple-darwin.zip](https://github.com/user-attachments/files/18781660/bitcoin-096525e92cc2-arm64-apple-darwin.zip)
Tested this on a Mac M1. I could download it flawlessly through Firefox and perform most of IBD.
I tried downloading it through Safari and likewise other reviewers i encountered an error:
https://github.com/user-attachments/assets/4f244f13-93ba-4071-bca1-4232cb69b69e
The system language was set to French but i guess what matters here is the
...
💬 maflcko commented on pull request "[POC] cmake: Introduce LLVM's Source-based Code Coverage reports":
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2663471948)
I've removed the milestone here for now, because this looks like it is adding a new feature that never existed before, so shouldn't be a blocker.
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2663471948)
I've removed the milestone here for now, because this looks like it is adding a new feature that never existed before, so shouldn't be a blocker.
💬 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
...