🤔 hodlinator reviewed a pull request: "refactor: Implement missing error checking for ArgsManager flags"
(https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2316714013)
`git range-diff master 1e37bcf 41bdf3d`

Adds code disallowing combining `ALLOW_BOOL` with `ALLOW_INT` and `ALLOW_STRING`. Removes now invalid tests and adds 2 `BOOST_CHECK_THROW()`s.

Curious to hear maflcko's thoughts now that this change was made (a month ago). Happy to see if I can shake some stuff out first through.

## Making the applying of arguments testable

hodlinator:

> > If the C++ code were strictly organized with pairs of free functions with the first calling AddArg() an
...

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1767576075)
nit: Remove double spaces
```suggestion
throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_BOOL flag is incompatible with DISALLOW_ELISION "
"(boolean arguments should not require argument values)", arg_name));
}
if ((flags & ALLOW_INT) && (flags & ALLOW_STRING)) {
throw std::logic_error(strprintf("Bug: bad %s flags. ALLOW_INT flag is incompatible with ALLOW_STRING "
"(any val
...

💬 hodlinator commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771310361)
Feels like this is encouraging `BOOL | STRING` usage and should be broken out too if we are postponing that support.

💬 willcl-ark commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2368164951)
> > set `CCACHE_COMPILERCHECK` to `content` mode, which passes the compiler check when the hash of the compiler is the same (the default is `mdate` which will change with each container we create)
>
> Are you sure? At least on Ubuntu Noble `mdate` is set correctly for `clang` (and does not change with each container). I presume the same is true for Debian and GCC. So, if the `modify` is different, the content would be different as well (and vice-versa). See:
>
> ```
> # stat /usr/bin/clan
...

💬 maflcko commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771467223)
> I think your advice to split up this PR is basically the same advice you are giving in #25665 to split up that PR, and my resistance in both PRs has been counterproductive.

It is certainly useful to have a working end-result that reviewers can look at, than not to have one. However, given an end-result, splitting up stuff for review (where it makes sense on a case-by-case basis) should have no downside, because reviewers can confirm that they agree with the overall goal and that the stuff i
...

💬 andrewtoth commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2368303170)
I really think there's too much variance when syncing with untrusted peers.

💬 maflcko commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2368341651)
> Curious to hear maflcko's thoughts now that this change was made (a month ago). Happy to see if I can shake some stuff out first through.

This needs rebase and is still in draft, which is why I haven't taken another look for now. Though, I may take another look later.

👍 ryanofsky approved a pull request: "depends: Fix build with `MULTIPROCESS=1` in Guix environment"
(https://github.com/bitcoin/bitcoin/pull/30940#pullrequestreview-2322355196)
Code review ACK 06b4c339e89e593d951a90cd2d1bce944acf3bf7

>> This causes CMake to search for package configurations in the native subdirectory first.
>
> This is the correct behaviour as far as we are concerned right?

I think the problem here is not that that cmake is searching the native directory first, but that it is searching the native directory only for the libmultiprocess runtime library, which is cross compiled, and can't be found in the native prefix.

The fix in d8e3afc3352a27
...

💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368400799)
> 2. I don't understand why "In the Guix environment, `${BASEPREFIX}/${HOST}/native/bin` is added to the `PATH` environment variable," according to the description. Setting this seems indiscriminate, like a sledgehammer approach, something that would cause the guix build to behave differently from normal depends builds and lead to confusing issues like this one.

I believe it was required when the macOS toolchain was built in depends. We can investigate this issue separately, can't we?

💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368431723)
My Guix build:
```
aarch64
8868809b50ea77b47c45c1cdf802e85540d9428f182773b1cd7e5d73750dce87 guix-build-06b4c339e89e/output/aarch64-linux-gnu/SHA256SUMS.part
fccf8cf14ca3d3ee7812f254d10eb17e62c507f5b2606c6b099c5d00ed5d3446 guix-build-06b4c339e89e/output/aarch64-linux-gnu/bitcoin-06b4c339e89e-aarch64-linux-gnu-debug.tar.gz
2d7cf4959e238fae666476db1cd11112c77708a58bb9d5cd1e0c7986c8c477a6 guix-build-06b4c339e89e/output/aarch64-linux-gnu/bitcoin-06b4c339e89e-aarch64-linux-gnu.tar.gz
22d7016c
...

💬 willcl-ark commented on pull request "test: re-bucket long-running tests":
(https://github.com/bitcoin/bitcoin/pull/30879#issuecomment-2368445072)
> Should feature_assumeutxo also be moved up a bit? According to [corecheck.dev/tests](https://corecheck.dev/tests) it is one of the top-20 slowest tests. Also, according to the CI runs in this pull (Asan + macOS) it is in the tail.

Added feature_assumeutxo to the < 2m bucket

💬 maflcko commented on pull request "test: switch MiniWallet padding unit from weight to vsize":
(https://github.com/bitcoin/bitcoin/pull/30718#issuecomment-2368460763)
lgtm ACK 7d1c8d0aa3ebe30d7633caf9bc4765927e4d6e08 🔋

<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: lgtm ACK 7d1c8d0aa3ebe30d
...

💬 l0rinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1771575039)
Was merged since (added you as co-author), you should be able to remove this now

💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368530170)
> 3. Indirectly related to this issue: I don't understand why depends toolchain is setting `set(CMAKE_FIND_USE_CMAKE_ENVIRONMENT_PATH OFF)`. I use a `shell.nix` file to set up a build environment for bitcoin and this setting make it hard to use depends because it makes cmake ignore the CMAKE_INCLUDE_PATH / CMAKE_PREFIX_PATH / CMAKE_LIBRARY_PATH variables constructed in the nix environment to allow cmake to find dependencies.

I vaguely recall that while working on the CMake staging branch,
...

💬 brunoerg commented on pull request "fuzz: fuzz connman with non-empty addrman + ASMap":
(https://github.com/bitcoin/bitcoin/pull/29536#issuecomment-2368551774)
Rebased

💬 Sjors commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368574744)
Demo branch `MULTIPROCESS=1` and without `x86_64-w64-mingw32`:

```
x86_64
e31ab0eeff88048301f113731a9b829df63e7db2c6a83c3a54e14b6f423aa4a8 guix-build-d8ec933456bc/output/aarch64-linux-gnu/SHA256SUMS.part
77f4a9481b4ce7df26549c6001384e066a1de3cb1b81ba950c29cc41b0b5c058 guix-build-d8ec933456bc/output/aarch64-linux-gnu/bitcoin-d8ec933456bc-aarch64-linux-gnu-debug.tar.gz
ee471d630ea0eb71c2040a3b9e408c4f1709ee29d2da4ecbc1931d93b67e5495 guix-build-d8ec933456bc/output/aarch64-linux-gnu/bitcoi
...

💬 ryanofsky commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2368602184)
Thanks for @hodlinator and @maflcko for the comments.I'll go ahead and update this PR in case marco is interested in looking at it since dropping support for flag combinations (https://github.com/bitcoin/bitcoin/pull/16545#pullrequestreview-2256044988).

In the meantime though I'm slowly working on the branch in #22978. Compared to this PR, that approach will be a larger change. This PR adds only ~100 lines if you exclude tests and documentation by only adding flags while not change existing c
...

💬 willcl-ark commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2368602687)
ACK 42edb2f4a5900e0b2451a7446d58962c0bf0095d

I agree with @l0rinc that this seems conceptually nicer, and also see a much smaller time diff since 30888:

```log
# master @ 33adc7521cc8bb24b941d959022b084002ba7c60
________________________________________________________
Executed in 181.29 secs fish external
usr time 38.26 mins 214.00 micros 38.26 mins
sys time 1.49 mins 55.00 micros 1.49 mins


# this PR rebased on master @ 33adc7521cc8bb24b941d959022
...

💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368636422)
> I don't understand why it is switching from _DIR variables to _ROOT variables. According to https://chatgpt.com/c/66f16a25-c068-800a-bba6-36d030d42869 both approaches seem roughly equivalent, but I don't understand if there was another reason to switch from _DIR to _ROOT.

1. https://cmake.org/cmake/help/latest/policy/CMP0074.html
2. `<PackageName>_ROOT` has top priority for the [`find_package()`](https://cmake.org/cmake/help/latest/command/find_package.html) command.
3. `<PackageName>_ROO
...

💬 fjahr commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771685550)
fixed