Bitcoin Core Github
43 subscribers
122K links
Download Telegram
πŸ“ 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
...
πŸ’¬ maflcko commented on pull request "Clear out space on GHA jobs":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3548426978)
Can this be closed, or is there something left to be done here?

Looks like a lot of hassle/bike-shedding for something that hasn't happened for about a month now.
πŸ’¬ sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2538838043)
Fixed in 95a8297d481e96d65ac81e4dac72b2ebecb9c765.
πŸ’¬ sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2538842194)
Done in 838d7e3553661cb6ba0be32dd872bafb444822d9
πŸ“ pinheadmz converted_to_draft a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747)
Introduces a new low-level socket manager `SockMan` as an abstract class with virtual functions for implementing higher-level networking protocols like HTTP. This is the next step in https://github.com/bitcoin/bitcoin/issues/31194

This is a minimal, alternative version of #30988 ("Split CConnman") without any changes to working code (P2P is not affected). It adds a stripped-down version of the `SockMan` introduced in that pull request that implements only what is needed for the HTTP server im
...
πŸ’¬ sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2538844903)
Took this wording in 34e32985e811607e7566ae7a6caeacdf8bd8384f
πŸ’¬ sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2538847018)
Updated the comment in 47ab32fdb158069d4422e0f92078603c6df070a6.
πŸ’¬ sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2538849747)
Took this comment and incorporated in 21b5cea588a7bfe758a8d14efe90046b111db428.
πŸ’¬ sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2538850474)
Done.
πŸ’¬ pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#issuecomment-3548455862)
rebased on master to fix conflict but also converted to draft.

The plan is now to integrate this I/O loop directly with the new http server. So I will rebase #32061 on this branch to include the feedback the socket stuff has received, and then combine this sockman with the http server and remove the abstraction.