🤔 maflcko reviewed a pull request: "scripted-diff: Type-safe settings retrieval"
(https://github.com/bitcoin/bitcoin/pull/31260#pullrequestreview-2623495377)
left two questions/suggestions
(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?
(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
...
(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.
(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
```
...
(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
...
(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?
(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
...
(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
(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
...
(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.
(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-
...
(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-
...
💬 rkrux commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959793328)
Nit: The Windows example could be more generic in case of absolute path.
```
HelpExampleCli("loadwallet", "\"DriveLetter:\\path\\to\\walletname\\\"")
```
or
```
HelpExampleCli("loadwallet", "\"C:\\path\\to\\walletname\\\"")
```
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959793328)
Nit: The Windows example could be more generic in case of absolute path.
```
HelpExampleCli("loadwallet", "\"DriveLetter:\\path\\to\\walletname\\\"")
```
or
```
HelpExampleCli("loadwallet", "\"C:\\path\\to\\walletname\\\"")
```
💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2665802465)
Rebased due to the conflict with the merged bitcoin/bitcoin#31268.
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2665802465)
Rebased due to the conflict with the merged bitcoin/bitcoin#31268.
💬 l0rinc commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1959806856)
Please resolve the comment, seems it's not strictly related to the issue
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1959806856)
Please resolve the comment, seems it's not strictly related to the issue
💬 dergoegge commented on pull request "wip: Split fuzz binary (take 2)":
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2665824563)
The full report (https://storage.googleapis.com/oss-fuzz-introspector/bitcoin-core/inspector-report/20250216/fuzz_report.html) still seems blocked on our dynamic harness look up. Maybe I'm looking at the wrong thing.
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2665824563)
The full report (https://storage.googleapis.com/oss-fuzz-introspector/bitcoin-core/inspector-report/20250216/fuzz_report.html) still seems blocked on our dynamic harness look up. Maybe I'm looking at the wrong thing.
💬 maflcko commented on pull request "wip: Split fuzz binary (take 2)":
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2665838423)
No it is the right thing. I just wanted to leave a comment to say that FI will now fallback to the light version by default, so the build is already passing, but the result didn't look like it could be used.
It could still make sense to try a FI run on this branch to see if it will help.
(https://github.com/bitcoin/bitcoin/pull/30882#issuecomment-2665838423)
No it is the right thing. I just wanted to leave a comment to say that FI will now fallback to the light version by default, so the build is already passing, but the result didn't look like it could be used.
It could still make sense to try a FI run on this branch to see if it will help.
💬 NicolaLS commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2665841137)
[Preview](https://github.com/NicolaLS/bitcoin/blob/doc-followup/doc/dependencies.md)
(https://github.com/bitcoin/bitcoin/pull/31895#issuecomment-2665841137)
[Preview](https://github.com/NicolaLS/bitcoin/blob/doc-followup/doc/dependencies.md)
👍 theStack approved a pull request: "test: remove scanning check on `wallet_importdescriptors`"
(https://github.com/bitcoin/bitcoin/pull/31893#pullrequestreview-2623718174)
ACK 405dd0e647e4ed0402320917a06128eb69504655
(https://github.com/bitcoin/bitcoin/pull/31893#pullrequestreview-2623718174)
ACK 405dd0e647e4ed0402320917a06128eb69504655
💬 jonatack commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959835437)
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the directory of the wallet to be loaded, either absolute or relative to the \"wallets\" directory. The \"wallets\" directory is set by the -walletdir option and defaults to the \"wallets\" folder within the data directory."},
```
- be more clear with the distinction between the wallet directory and the wallets directory
- legacy wallet loading will soon be disabled: https://github.com/bitc
...
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1959835437)
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "The path to the directory of the wallet to be loaded, either absolute or relative to the \"wallets\" directory. The \"wallets\" directory is set by the -walletdir option and defaults to the \"wallets\" folder within the data directory."},
```
- be more clear with the distinction between the wallet directory and the wallets directory
- legacy wallet loading will soon be disabled: https://github.com/bitc
...