💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2390087376)
That's a clever way of doing it, did something very close to it now (hopefully it's small enough that source code license issues don't apply).
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2390087376)
That's a clever way of doing it, did something very close to it now (hopefully it's small enough that source code license issues don't apply).
💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2390038435)
Did something similar to https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389662377
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2390038435)
Did something similar to https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389662377
💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2392650479)
To me there's a silent, implied, "Enables ..." or "Provides ..." at the beginning of the description for these options that would get repetitive if spelled out, which may make "support" more natural.
Note that simply enabling the option does not pause startup, one also has to start with `-waitfordebugger`.
Curious to hear what other reviewers think.
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2392650479)
To me there's a silent, implied, "Enables ..." or "Provides ..." at the beginning of the description for these options that would get repetitive if spelled out, which may make "support" more natural.
Note that simply enabling the option does not pause startup, one also has to start with `-waitfordebugger`.
Curious to hear what other reviewers think.
🤔 hodlinator reviewed a pull request: "ci: Check macos-cross executables on macOS"
(https://github.com/bitcoin/bitcoin/pull/33509#pullrequestreview-3288065733)
Concept ACK d40e61e87cfb18f75742c2efa6e705f38e3b36c9
It feels more complete to test something closer to what the end user will be running (although we are still not using Guix builds).
Since we don't have high hopes of this actually detecting any issues that the rest of the CI would not have found, seems to make more sense as a nightly job?
Runs fine:
https://github.com/bitcoin/bitcoin/actions/runs/18132888148/job/51606766678?pr=33509
(https://github.com/bitcoin/bitcoin/pull/33509#pullrequestreview-3288065733)
Concept ACK d40e61e87cfb18f75742c2efa6e705f38e3b36c9
It feels more complete to test something closer to what the end user will be running (although we are still not using Guix builds).
Since we don't have high hopes of this actually detecting any issues that the rest of the CI would not have found, seems to make more sense as a nightly job?
Runs fine:
https://github.com/bitcoin/bitcoin/actions/runs/18132888148/job/51606766678?pr=33509
💬 willcl-ark commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3355296742)
One possible "tightening" could be to only export the [exact variables needed](https://github.com/bitcoin/bitcoin/actions/runs/18126966670/job/51584371561?pr=33508#step:6:333):
```
Exporting ACTIONS_RUNNER_HOOK_JOB_STARTED # <-- likely not required for buildx gha cache
Exporting ACTIONS_RUNTIME_URL
Exporting ACTIONS_RUNTIME_TOKEN
Exporting ACTIONS_CACHE_URL
Exporting ACTIONS_RESULTS_URL
Exporting ACTIONS_CACHE_SERVICE_V2
```
But IMO exporting all variables prefixed with `ACTIONS_` t
...
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3355296742)
One possible "tightening" could be to only export the [exact variables needed](https://github.com/bitcoin/bitcoin/actions/runs/18126966670/job/51584371561?pr=33508#step:6:333):
```
Exporting ACTIONS_RUNNER_HOOK_JOB_STARTED # <-- likely not required for buildx gha cache
Exporting ACTIONS_RUNTIME_URL
Exporting ACTIONS_RUNTIME_TOKEN
Exporting ACTIONS_CACHE_URL
Exporting ACTIONS_RESULTS_URL
Exporting ACTIONS_CACHE_SERVICE_V2
```
But IMO exporting all variables prefixed with `ACTIONS_` t
...
💬 brangnu commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-3355366242)
Concept NACK.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-3355366242)
Concept NACK.
💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2393958445)
> I don't think multisig inputs tend to be mixed with non-multisig inputs.
Ah, I should have rephrased the sentence to highlight spending multiple MuSig2 unspents in the PSBT instead of highlighting the other one.
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2393958445)
> I don't think multisig inputs tend to be mixed with non-multisig inputs.
Ah, I should have rephrased the sentence to highlight spending multiple MuSig2 unspents in the PSBT instead of highlighting the other one.
💬 hodlinator commented on pull request "ci: Switch to dynamic library linkage in native Windows job":
(https://github.com/bitcoin/bitcoin/pull/32182#issuecomment-3355516795)
Noticed the build artifact `x86_64-w64-mingw32-executables-<run>` is 916MB (https://github.com/bitcoin/bitcoin/actions/runs/18155350909?pr=31723 / https://github.com/bitcoin/bitcoin/actions/runs/18155350909/job/51673850541?pr=31723#step:9:161).
Artifact contents:
```shell
$ unzip -l x86_64-w64-mingw32-executables-18155350909.zip
Archive: x86_64-w64-mingw32-executables-18155350909.zip
Length Date Time Name
--------- ---------- ----- ----
617942101 10-01-2025 07:57 bi
...
(https://github.com/bitcoin/bitcoin/pull/32182#issuecomment-3355516795)
Noticed the build artifact `x86_64-w64-mingw32-executables-<run>` is 916MB (https://github.com/bitcoin/bitcoin/actions/runs/18155350909?pr=31723 / https://github.com/bitcoin/bitcoin/actions/runs/18155350909/job/51673850541?pr=31723#step:9:161).
Artifact contents:
```shell
$ unzip -l x86_64-w64-mingw32-executables-18155350909.zip
Archive: x86_64-w64-mingw32-executables-18155350909.zip
Length Date Time Name
--------- ---------- ----- ----
617942101 10-01-2025 07:57 bi
...
👍 rkrux approved a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3288310595)
> receive and spend from imported musig(0 descriptors.
Is this a typo in the PR description?
----
This was a big PR that required me to split the review into multiple parts in which I tried to think of scenarios that could cause the flow could break or be unsafe.
Few points from my understanding of this PR:
1. Appropriate steps are taken to secure the `MuSig2SecNonce`.
- It is stored only in memory - never backed up on disk, or serialised. Okay with the tradeoff of having to
...
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3288310595)
> receive and spend from imported musig(0 descriptors.
Is this a typo in the PR description?
----
This was a big PR that required me to split the review into multiple parts in which I tried to think of scenarios that could cause the flow could break or be unsafe.
Few points from my understanding of this PR:
1. Appropriate steps are taken to secure the `MuSig2SecNonce`.
- It is stored only in memory - never backed up on disk, or serialised. Okay with the tradeoff of having to
...
✅ vasild closed a pull request: "fuzz: set the output argument of FuzzedSock::Accept()"
(https://github.com/bitcoin/bitcoin/pull/31316)
(https://github.com/bitcoin/bitcoin/pull/31316)
💬 vasild commented on pull request "fuzz: set the output argument of FuzzedSock::Accept()":
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-3355567303)
Closing this because https://github.com/bitcoin/bitcoin/pull/28584 was merged which includes the commit in this PR.
This PR: 83199523c90591d57cd5046212c878a4d54d621d
#28584 contains: e883b37768812d96feec207a37202c7d1b603c1f
```sh
$ git range-diff e883b37768~..e883b37768 83199523c9~..83199523c9
1: e883b37768 = 1: 83199523c9 fuzz: set the output argument of FuzzedSock::Accept()
$
```
(https://github.com/bitcoin/bitcoin/pull/31316#issuecomment-3355567303)
Closing this because https://github.com/bitcoin/bitcoin/pull/28584 was merged which includes the commit in this PR.
This PR: 83199523c90591d57cd5046212c878a4d54d621d
#28584 contains: e883b37768812d96feec207a37202c7d1b603c1f
```sh
$ git range-diff e883b37768~..e883b37768 83199523c9~..83199523c9
1: e883b37768 = 1: 83199523c9 fuzz: set the output argument of FuzzedSock::Accept()
$
```
💬 stickies-v commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3355697586)
re-ACK 1aaaaa078bb2efed126e3f41ecf7c81ccf005818
(https://github.com/bitcoin/bitcoin/pull/33489#issuecomment-3355697586)
re-ACK 1aaaaa078bb2efed126e3f41ecf7c81ccf005818
👍 vasild approved a pull request: "test: fix p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/33121#pullrequestreview-3288480537)
ACK 14ae71f323dd011c6d51470ea15cf00750970f65
(https://github.com/bitcoin/bitcoin/pull/33121#pullrequestreview-3288480537)
ACK 14ae71f323dd011c6d51470ea15cf00750970f65
👍 hebasto approved a pull request: "ci: Remove bash -c from cmake invocation using eval"
(https://github.com/bitcoin/bitcoin/pull/33401#pullrequestreview-3288490644)
ACK 50194029e7c2581b751931080f5999785a39929f.
(https://github.com/bitcoin/bitcoin/pull/33401#pullrequestreview-3288490644)
ACK 50194029e7c2581b751931080f5999785a39929f.
🚀 hebasto merged a pull request: "ci: Remove bash -c from cmake invocation using eval"
(https://github.com/bitcoin/bitcoin/pull/33401)
(https://github.com/bitcoin/bitcoin/pull/33401)
💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2394150885)
done.
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2394150885)
done.
💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2394151442)
ah I see how it could be confusing, clarified it to "since the previous commit removes BLOCK_FAILED_CHILD usage in InvalidateBlock"
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2394151442)
ah I see how it could be confusing, clarified it to "since the previous commit removes BLOCK_FAILED_CHILD usage in InvalidateBlock"
💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2394152350)
good point. I've removed it since `BLOCK_FAILED_MASK = 96` is never written to disk but only used while reading BlockStatus values. if 96 exists on disk, it is the responsibility of `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` and we have dealt with those cases.
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2394152350)
good point. I've removed it since `BLOCK_FAILED_MASK = 96` is never written to disk but only used while reading BlockStatus values. if 96 exists on disk, it is the responsibility of `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` and we have dealt with those cases.
💬 stratospher commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2394152594)
done.
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2394152594)
done.
💬 trevarj commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3355880463)
> Hopefully Guix makes a release tag again one day.
This is coming soon, there have been talks of it.
In the meantime, you can simply `guix pull` and receive the freshest version (from master). If you don't want master, you can edit your Guix channels (`~/.config/guix/channels.scm`) and pin the commit to something "stable".
I'm not certain that would cause an issue though. To me it just sounds like unstable substitute servers. For context, if you try installing Guix System it could tak
...
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3355880463)
> Hopefully Guix makes a release tag again one day.
This is coming soon, there have been talks of it.
In the meantime, you can simply `guix pull` and receive the freshest version (from master). If you don't want master, you can edit your Guix channels (`~/.config/guix/channels.scm`) and pin the commit to something "stable".
I'm not certain that would cause an issue though. To me it just sounds like unstable substitute servers. For context, if you try installing Guix System it could tak
...