Bitcoin Core Github
44 subscribers
122K links
Download Telegram
🤔 glozow reviewed a pull request: "doc: truc packages allow sub min feerate transactions"
(https://github.com/bitcoin/bitcoin/pull/33220#pullrequestreview-3180848706)
ACK 7270839af425adddb3ed436a58a41b5bc843dab5
glozow closed an issue: "doc: Mempool Policy documentation Outdated since TRUC"
(https://github.com/bitcoin/bitcoin/issues/32067)
🚀 glozow merged a pull request: "doc: truc packages allow sub min feerate transactions"
(https://github.com/bitcoin/bitcoin/pull/33220)
🤔 glozow reviewed a pull request: "[29.x] finalise v29.1"
(https://github.com/bitcoin/bitcoin/pull/33271#pullrequestreview-3180870194)
ACK 084c95a18c9978c0a047ffe219a9eef8ab327ea6
👍 willcl-ark approved a pull request: "[29.x] finalise v29.1"
(https://github.com/bitcoin/bitcoin/pull/33271#pullrequestreview-3180882773)
ACK 084c95a18c9978c0a047ffe219a9eef8ab327ea6

This all looks correct to me
💬 ryanofsky commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249470528)
This is a nice idea and approach seems ok if it works in practice. I think a more general approach might to have the bitcoin build set a variable the libmultiprocess build can use to show better error messages like:

```
set(MP_SUBPROJECT_ERROR "Configure cmake with with -DENABLE_IPC=OFF if you do not need IPC functionality.")
```

It could append this message if it can't find the cap'n proto package, or doesn't find a compatible version (for https://github.com/bitcoin-core/libmultiproces
...
⚠️ maflcko opened an issue: "ci: GHA fallback centos task runs out of space"
(https://github.com/bitcoin/bitcoin/issues/33293)
https://github.com/bitcoin-core/gui/actions/runs/17433074590/job/49495964258?pr=884#step:8:3785

```
...
+ eval 'TEST_RUNNER_EXTRA=()'
++ TEST_RUNNER_EXTRA=()
+ LD_LIBRARY_PATH=/home/runner/work/_temp/depends/x86_64-pc-linux-gnu/lib
+ /home/runner/work/_temp/build/test/functional/test_runner.py --ci -j4 --tmpdirprefix /home/runner/work/_temp/ci/scratch/test_runner/ --ansi --combinedlogslen=99999999 --timeout-factor=40 --quiet --failfast
WARNING! There may be insufficient free space in /home/runn
...
💬 maflcko commented on issue "ci: GHA fallback centos task runs out of space":
(https://github.com/bitcoin/bitcoin/issues/33293#issuecomment-3249499925)
It may be possible to free up some space with something like https://github.com/google/oss-fuzz/blob/14cab84abbc736e24fede3f0267a8e220fb4974e/.github/workflows/project_tests.yml#L65-L74
💬 maflcko commented on pull request "Fix compatibility with `-debuglogfile` command-line option":
(https://github.com/bitcoin-core/gui/pull/884#issuecomment-3249501476)
(ci failure can be ignored, see https://github.com/bitcoin/bitcoin/issues/33293)
💬 willcl-ark commented on issue "ci: GHA fallback centos task runs out of space":
(https://github.com/bitcoin/bitcoin/issues/33293#issuecomment-3249508043)
```
Free disk space:
+ [[ ci_native_centos == \c\i\_\n\a\t\i\v\e\_\a\s\a\n ]]
++ /home/runner/work/_temp/depends/config.guess
Filesystem Size Used Avail Use% Mounted on
overlay 72G 48G 25G 67% /
tmpfs 64M 0 64M 0% /dev
shm 64M 0 64M 0% /dev/shm
/dev/root 72G 48G 25G 67% /etc/hosts
tmpfs 7.9G 0 7.9G 0% /proc/acpi
tmpfs 7.9G 0 7.9G 0% /proc/scsi
tmpfs 7.9G 0 7.9G 0% /sys/firmw
...
🤔 theStack reviewed a pull request: "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys"
(https://github.com/bitcoin/bitcoin/pull/29675#pullrequestreview-3180916078)
Reviewed up to 38d74136c6552ae746bec6e05b8ec3069cd581ce, left a few non-blocking suggestions below.

I think the order of commits d65c8df972de55ade06557cabb1b8972d4169fb1 ... 38d74136c6552ae746bec6e05b8ec3069cd581ce is currently slightly confusing for reviewers, as it doesn't reflect the protocol flow, i.e. the signature aggregation function `CreateMuSig2AggregateSig` should ideally be introduced _after_ the partial signature creation function `CreateMuSig2PartialSig`.
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2319159859)
in commit a7901710a6b222cbdeba474bbe8b78788841e58a: it's only a few lines of code, but to deduplicate with the next commit (introducing `CreateMuSig2PartialSig`), could introduce a helper function like `MuSig2SessionId`
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2319145295)
in commit d65c8df972de55ade06557cabb1b8972d4169fb1: naming nit: `participants` doesn't say much imho, could rename to `participant_pubkeys` (or just `part_pubkeys` / `pubkeys`, if that's too long) to be more specific
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2319173890)
in 38d74136c6552ae746bec6e05b8ec3069cd581ce: if `our_pubkey` is not contained in the `pubkeys` list, I suppose we want to return early with `std::nullopt`, rather than accessing at index 0?
💬 theStack commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2319170915)
in commit 38d74136c6552ae746bec6e05b8ec3069cd581ce: nit, could rename to something like `our_pubkey_index` to be more expressive
💬 ryanofsky commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3249543838)
It seems like there's not much downside to this change, other than that it may need to be reverted later if an IPC fuzz test is added. And I think if a fuzz test is added we would also want to revert https://github.com/google/oss-fuzz/pull/13854?

Only Sjors above https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210571615 expressed a preference for not making this change and maflcko https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3210147334 raised an early question about
...
💬 Sjors commented on pull request "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/pull/33290#issuecomment-3249574033)
@ryanofsky I like your approach better since it can be used under more circumstances, such as not having the right version. But we're close to branch-off so maybe this approach suffices for now.
💬 Sjors commented on pull request "build: set ENABLE_IPC to OFF when fuzzing":
(https://github.com/bitcoin/bitcoin/pull/33235#issuecomment-3249576984)
I'm fine with this change if no fuzz PR is merged before branch-off.
🚀 glozow merged a pull request: "[29.x] finalise v29.1"
(https://github.com/bitcoin/bitcoin/pull/33271)
💬 purpleKarrot commented on issue "Revisiting us self-hosting parts of our CI":
(https://github.com/bitcoin/bitcoin/issues/31965#issuecomment-3249588115)
> I quite like the idea of all of CI being defined as Github Actions

I prefer the idea that all CI is decoupled from the project, and that both the project and the CI are implemented against a defined interface: the cmake project definition. I presented this approach at C++Now 2025 as [Effective CTest](https://schedule.cppnow.org/session/2025/effective-ctest/).

Defining CI in a YAML file that is part of the project essentially states "it works on my machine(s)". The project will start to suppo
...