Bitcoin Core Github
43 subscribers
122K links
Download Telegram
πŸ‘ l0rinc approved a pull request: "doc: add missing copyright headers"
(https://github.com/bitcoin/bitcoin/pull/31864#pullrequestreview-2623350599)
While I admit having stronger feelings against the [copyright headers](https://github.com/bitcoin/bitcoin/issues/29445) than I should (noise + bikeshedding + revoking right to copy in general), I do value consistency, so it's a πŸ‘ from me - I wish we could unify more to get rid of this extra source of disagreement, but this PR makes it a bit better, so:

ACK 01b9a6183eb58dffac00f053e2742d924c84c721
πŸ’¬ l0rinc commented on pull request "doc: add missing copyright headers":
(https://github.com/bitcoin/bitcoin/pull/31864#discussion_r1959618226)
πŸ‘ was removed in https://github.com/bitcoin/bitcoin/commit/3ed772d2219e58d6afea3d12c0ebebb8487445e7
πŸ’¬ l0rinc commented on pull request "doc: add missing copyright headers":
(https://github.com/bitcoin/bitcoin/pull/31864#discussion_r1959683707)
Only found these in Python dependencies, so: πŸ‘

Unrelated: we could update the script to change the end year to `present` instead
πŸ’¬ purpleKarrot commented on pull request "cmake: Make implicit `libbitcoinkernel` dependencies explicit":
(https://github.com/bitcoin/bitcoin/pull/31884#issuecomment-2665638833)
ACK. I agree that this approach is preferred over the mentioned alternative.
πŸ‘ TheCharlatan approved a pull request: "cmake: Make implicit `libbitcoinkernel` dependencies explicit"
(https://github.com/bitcoin/bitcoin/pull/31884#pullrequestreview-2623494959)
ACK 3b42e05aa9e38ba00d52b2338375b4caf032f041

I prefer this over your mentioned alternative too.
πŸ’¬ TheCharlatan commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2665666986)
Thought a guix build might be prudent since we are changing a bunch of paths (built on aarch64):
```
b683fabbea0455a7f8a5c0566079b1bbce438edd5870e11209ce2dd5a016541e guix-build-186680acef66/output/aarch64-linux-gnu/SHA256SUMS.part
4c144bdb301bf7d81dd99a8cef2082950fd5912f8edba6cea545482445cf7dba guix-build-186680acef66/output/aarch64-linux-gnu/bitcoin-186680acef66-aarch64-linux-gnu-debug.tar.gz
c4e4a24a847dd420cd1b9310e13aef1e6376d069c5099f799918dd3f2c3a01e3 guix-build-186680acef66/output/
...
πŸ’¬ jsarenik commented on issue "Memory-only wallet (for faucets)":
(https://github.com/bitcoin/bitcoin/issues/31816#issuecomment-2665671154)
@maflcko just for the record, the current wallet of alt.signetfaucet.com is running in Linux' `tmpfs` (i.e. in RAM) and is being refreshed by `cron` every hour. No leftovers. Actually I think there is no need for the proposed feature.

Thank you! I like Bitcoin Core!
πŸ€” maflcko reviewed a pull request: "scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260#pullrequestreview-2623495377)
left two questions/suggestions
πŸ’¬ maflcko commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1959711651)
Similar feedback here: The default value seems like one that could have been provided by a user, so there should be no risk in setting it via `DefaultFn` and having it be the default. (With the added benefits already mentioned).

This means that the `HelpFn` would (ideally) have a way to query the default value as well. Maybe it is possible to pass the settings object instead of `fmt` and then use it to access `fmt` and the default value?
πŸ’¬ maflcko commented on pull request "scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#discussion_r1959703860)
shouldn't this be using `DefaultFn<[] { return FormatOutputType(DEFAULT_ADDRESS_TYPE); }>` instead?

The benefits would be:

* Code consistency: Integral settings mostly have their default or default fn set, however std::string values do not.
* Parsing consistency: Any parsing (and sanity checking) could happen on the returned string directly, meaning that special casing the fallback-case would no longer be needed and the fallback value would be sanity checked like any other user-provided s
...
πŸ’¬ maflcko commented on issue "test: feature_assumeutxo.py Windows timeout":
(https://github.com/bitcoin/bitcoin/issues/31894#issuecomment-2665688808)
> Hm yeah, but then why does `enabling extra block-relay-only peers` happen after that?

The `StartExtraBlockRelayPeers` function is logging to the `[net]` category (not to be confused with the `[net]` thread) and is called by `CheckForStaleTipAndEvictPeers`, which is running in the `[scheduler]` thread.
πŸ’¬ purpleKarrot commented on pull request "doc: update dev note examples for CMake":
(https://github.com/bitcoin/bitcoin/pull/30739#issuecomment-2665709078)
It may not be clear to all readers of this document that `-B` will instruct cmake to create a directory and that the argument could be anything. This information may be useful for people that want to maintain multiple build directories.

`cmake --build` is useful in generic scripts that handle multiple build systems.

On the command line, invoking the underlying commands explicitly is both easier to understand and faster to type:

```sh
mkdir build
cd build
cmake -GNinja ..
ninja
```
...
πŸ’¬ s373nZ commented on pull request "build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2665715592)
ACK https://github.com/bitcoin/bitcoin/pull/31665/commits/eaf6a5198d2b9033a6d6f0a4c01faddc67152857

Tested generation under the following variety of option combinations on Ubuntu 24.04.2 LTS:

1. `libdb++dev` package not installed: `cmake -B build -DWCONFIGURE_ERROR=ON -DWARN_INCOMPATIBLE_BDB=ON -DENABLE_WALLET=ON -DWITH_BDB=ON --fresh`
<details>
<summary>Test 1 result</summary>

```bash
CMake Error at /usr/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
C
...
πŸ’¬ fanquake commented on pull request "cmake: Add optional source files to libraries directly":
(https://github.com/bitcoin/bitcoin/pull/31880#issuecomment-2665721761)
> This PR provides an extended alternative to https://github.com/bitcoin/bitcoin/pull/31268,

That was merged. Is this still relevant?
πŸ“ NicolaLS opened a pull request: "doc: Improve `dependencies.md`"
(https://github.com/bitcoin/bitcoin/pull/31895)
Small improvements to the `dependencies.md` documentation as a follow up for #31634.

**Linux Kernel** does not need to be in the dependencies as it is not required for cross-compiling from other systems and users building on Linux should not expect they can build using any EOL kernel, see: https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1957123270

**CMake** has a "Version Used" that can be added to the table, see: https://github.com/bitcoin/bitcoin/pull/31634#discussion_r19197358
...
πŸ’¬ NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1959754175)
Here's the draft: https://github.com/bitcoin/bitcoin/pull/31895

Please let me know if this addresses the Linux Kernel problem and other stuff that was out of scope for this PR sufficiently and if its ready to be reviewed @hodlinator
πŸ’¬ philmb3487 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2665733707)
I don’t think we should bother the user with these gratuitous messages. Has it been demonstrated that SD cards lead to corruption? From what I understand, this issue is about Apple-specific software bugs and it should stick to this.

On Mon, Feb 17, 2025 at 22:32, l0rinc ***@***.***(mailto:On Mon, Feb 17, 2025 at 22:32, l0rinc <<a href=)> wrote:

> @l0rinc commented on this pull request.
>
> ---------------------------------------------------------------
>
> In [src/common/init.cpp](https://gith
...
πŸ’¬ sipa commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2665786438)
@l0rinc @philmb3487 I think we shouldn't conflate these issues. From the linked #28552 issue, it appears that there is an actual demonstrable corruption that happens specifically due to exFAT on macOS, and that is what this PR is addressing. In addition, we have vague reports over the years of corruption when running Bitcoin Core on relatively cheap consumer storage (sdcards, external spinning drives, ...), but it's far less of a clear-cut case there.
πŸ‘ rkrux approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2623646788)
tACK 80e695e6f1edd1a6d9aaab0748a5b573c4492cb4

I built and executed the help command on my mac machine, PFB the output. The PR has gone through multiple rounds of review, shared a nit but not inclined on invalidating ACKs for this.

<details>
<summary>
MAC output
</summary>

```
➜ src git:(am-sq/improve-description-loadwallet-rpc) βœ— bitcoinclireg help loadwallet
loadwallet "filename" ( load_on_startup )

Loads a wallet from a wallet file or directory.
Note that all wallet command-
...