Bitcoin Core Github
44 subscribers
122K links
Download Telegram
πŸ’¬ maflcko commented on issue "Source code mapping for debugger has changed since cmake":
(https://github.com/bitcoin/bitcoin/issues/31204#issuecomment-3548048120)
For reference, inside GDB, the following worked for me:

```
(gdb) set substitute-path ./ /path/to/project/root/src
```

*Not* using the (outdated?) gdbinit advice:

```
set substitute-path ./src /path/to/project/root/src
```

As an alternative, i could use debugedit:

```
debugedit --base-dir ./ --dest-dir /path/to/project/root/src bld-cmake/bin/test_kernel
πŸ’¬ maflcko commented on issue "callgrind_annotate broken after cmake migration?":
(https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3548048725)
Ah, looks like this is related to the prefix-map and the following workaround fixes it:

```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index f264acc..9ed5b46 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -488,12 +488,6 @@ try_append_cxx_flags("-fno-extended-identifiers" TARGET core_interface SKIP_LINK
# which can cause issues with coverage builds, particularly when using
# Clang in the OSS-Fuzz environment due to its use of other options
# and a third party script, or with GCC.
-
...
πŸ’¬ roconnor-blockstream commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3548052465)
> Solver() calls MatchMultisig() which invokes num_keys = GetScriptNuymber(opcode, data, required_sigs, MAX_PUBKEYS_PER_MULTISIG).

This checking m ≀ n behaviour in the Solver seems to have been added in #13194. However this wouldn't count as soft-confiscations because multisigs with m > n are presumably unspendable anways.

> What we support by consensus matches the (earlier) ANSI X9.62-1998 standard

Ah, I was not aware of this.

I wouldn't object to dropping the hybrid key protections. I pr
...
πŸ’¬ maflcko commented on issue "callgrind_annotate broken after cmake migration?":
(https://github.com/bitcoin/bitcoin/issues/31957#issuecomment-3548070707)
Maybe the prefix-map can be disabled by default or an option could be added to disable it, or it could be disabled on `-DWITH_CCACHE=NO`, as there is no need for it then?
πŸ’¬ waketraindev commented on pull request "net: Decouple `CConnman::GetAddresses` from `CNode`":
(https://github.com/bitcoin/bitcoin/pull/33900#issuecomment-3548073525)
> it probably makes sense to check if those refactors are in line with the Net Split WG (ref https://achow101.com/ircmeetings/2025/bitcoin-core-dev.2025-11-13_16_00.log.html and the coredev meeting notes).
>
> Otherwise, this may be touched again soon after.



> it probably makes sense to check if those refactors are in line with the Net Split WG (ref https://achow101.com/ircmeetings/2025/bitcoin-core-dev.2025-11-13_16_00.log.html and the coredev meeting notes).
>
> Otherwise, this ma
...
πŸ’¬ waketraindev commented on pull request "net: Decouple `CConnman::GetAddresses` from `CNode`":
(https://github.com/bitcoin/bitcoin/pull/33900#issuecomment-3548115837)
@theuni is this refactor okay with you?
πŸ“ maflcko converted_to_draft a pull request: "ci: Call docker exec from Python script to fix word splitting"
(https://github.com/bitcoin/bitcoin/pull/33732)
The remaining `ci/test/02_run_container.sh` is fine, but has a bunch of shellcheck SC2086 word splitting violations.

This is fine currently, because the only place that needed them had additional escaping, and all other commands happened to split fine on spaces.

However, this may change in the future. So fix it now, by rewriting it in Python, which is recommended in the dev notes.
πŸ“ willcl-ark opened a pull request: "Document compiler configuration for native depends packages"
(https://github.com/bitcoin/bitcoin/pull/33902)
Fixes: #33859

Previously one had to read the Makefile (and various *.mk configuration
files) to see how to correctly override CC and CXX when building native
depends packages.

Detail this in README.md to make it clearer.
πŸ€” hebasto reviewed a pull request: "Document compiler configuration for native depends packages"
(https://github.com/bitcoin/bitcoin/pull/33902#pullrequestreview-3478388624)
Concept ACK. Thanks!
πŸ“ maflcko opened a pull request: "ci: Remove redundant busybox option"
(https://github.com/bitcoin/bitcoin/pull/33903)
The option was fine, but it never found an issue, IIRC.

Also, now that there is a dedicated Alpine Linux task, which uses BusyBox, it seems redundant.
(See: `ci/test/00_setup_env_native_alpine_musl.sh`)

So remove the `USE_BUSY_BOX` option, along with the `BINS_SCRATCH_DIR` env var.
πŸ’¬ willcl-ark commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#issuecomment-3548180569)
Forgot to say, very happy to drop the second commit if wanted.
πŸ’¬ hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2538653945)
How is this better than `make build_CC=clang build_CXX=clang++ CC=clang CXX=clang++`?
πŸ’¬ willcl-ark commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2538676311)
They should be exactly equivalent.

I used my version as I had just described `build_CC` and `host_CC` and it would seem a little odd (to me) to exemplify with `build_CC` and `CC`, although as I re-read it now this example appears in the `CC, CXX` section.

I think therefore I should set `make build_CC=clang build_CXX=clang++ CC=clang CXX=clang++` as the primary example for this section, and perhaps use `make build_CC=clang build_CXX=clang++ host_CC=clang host_CXX=clang++` as an "ultimately
...
πŸ’¬ hebasto commented on pull request "ci: Remove redundant busybox option":
(https://github.com/bitcoin/bitcoin/pull/33903#issuecomment-3548243409)
> The option was fine, but it never found an issue, IIRC.

I’m not disputing this specific change, but that line of reasoning isn’t valid, as the absence of past errors does not imply their absence in the future. It seems as a form of [the appeal to ignorance fallacy](https://en.wikipedia.org/wiki/Argument_from_ignorance).
πŸ’¬ fanquake commented on pull request "ci: Remove redundant busybox option":
(https://github.com/bitcoin/bitcoin/pull/33903#issuecomment-3548309839)
Concept ACK
πŸ’¬ ryanofsky commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3548316676)
Concept ACK cce7eba489d5fb8b9f603584b5939dd42ccc6e8. This PR improves documentation and error handling for subcommand specific options.

I do think the PR could use a more detailed description. (I wasn't very familiar with existing subcommand support so I didn't know what was changing.)

Right now, `ArgsManager` supports subcommands like `bitcoin-wallet dump` and supports subcommand-specific options like `-dumpfile`, e.g.:

```bash
bitcoin-wallet -wallet=foo -dumpfile=foo.txt dump
```

...
πŸ‘ maflcko approved a pull request: "Document compiler configuration for native depends packages"
(https://github.com/bitcoin/bitcoin/pull/33902#pullrequestreview-3478515255)
lgtm

review ACK 17577646f77e20783ffdd9f322f85e96da2265 🌴

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 17577
...
πŸ’¬ maflcko commented on pull request "ci: Remove redundant busybox option":
(https://github.com/bitcoin/bitcoin/pull/33903#issuecomment-3548389377)
> > The option was fine, but it never found an issue, IIRC.
>
> I’m not disputing this specific change, but that line of reasoning isn’t valid, as the absence of past errors does not imply their absence in the future. It seems as a form of [the appeal to ignorance fallacy](https://en.wikipedia.org/wiki/Argument_from_ignorance).

Thx, removed the line from the pull description. It being redundant is the real reason, also put in the pull title. It not having found an issue is more a "fun-fact
...
βœ… maflcko closed an issue: "ci: GHA fallback centos task runs out of space"
(https://github.com/bitcoin/bitcoin/issues/33293)
πŸ’¬ maflcko commented on issue "ci: GHA fallback centos task runs out of space":
(https://github.com/bitcoin/bitcoin/issues/33293#issuecomment-3548421494)
> > This will be "fixed" by [#33480](https://github.com/bitcoin/bitcoin/pull/33480) (as a side-effect).
>
> Are you sure? The task build config should be unchanged in this pull request (only the runtime distro is changed) . So if the pull works around it, it doesn't seem like something that can be relied upon.

Given that reviewers in the pull prefer the workaround to be specific to the tasks that need it (https://github.com/bitcoin/bitcoin/pull/33514#pullrequestreview-3307376880), I think this
...