🤔 furszy reviewed a pull request: "wallet: Write best block to disk before backup"
(https://github.com/bitcoin/bitcoin/pull/30678#pullrequestreview-2321884746)
ACK f20fe33

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1771218843)
I thought of that, but since it has to be different from `base_value`, seems weird to assume it that in a different method instead of the call-site.
```
Base Write Cache Expected Coinbase
CheckAddCoin(base_value, VALUE3, SPENT_DIRTY, VALUE3_DIRTY, false); // VALUE3 + VALUE3_DIRTY -> VALUE3_DIRTY
```
makes more sense than
```
Base Cache Expected Coinbase
CheckAddCoin(base_value,
...

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1771221285)
Added some unit tests yesterday btw, should definitely catch this in I think 2 cases.

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1771249650)
Done, thanks

💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2367971650)
> 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/clang++
File:
...

💬 l0rinc commented on pull request "Drop -dbcache limit":
(https://github.com/bitcoin/bitcoin/pull/28358#issuecomment-2368014082)
> Didn’t #28280 resolve this?

That's why I was checking it, I've rebased it on [9cb9651d92ddb5d92724f6a52440601c7a0bbcf8](https://github.com/bitcoin/bitcoin/commit/9cb9651d92ddb5d92724f6a52440601c7a0bbcf8) before running it with `-prune`.
@andrewtoth is this what you expected to happen, or was I measuring something wrong here?

<details>
<summary>Details</summary>

```bash
# git rev-parse HEAD
6d7f24595b08b8d1eba53e648533bcf87c30b48f

# git log --grep="bitcoin/bitcoin#28280"
commit
...

📝 stratospher opened a pull request: "net: option to disallow v1 connection on ipv4 and ipv6 peers"
(https://github.com/bitcoin/bitcoin/pull/30951)
resolves #29618.

This PR adds a config flag option `-v2onlyclearnet` which disallows v1 connections on ipv4 and ipv6 networks since they're unencrypted. v1 connections are still possible from tor/i2p/cjdns peers since they're encrypted networks.

- v1 outbound connections to peers are not attempted
- v1 reconnections are not attempted (if peer is falsely advertised as v2 peer and we attempt a v2 connection but it fails, we do not attempt a v1 reconnection when `-v2onlyclearnet` is switched
...

💬 stratospher commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2368065103)
opened a possible solution in https://github.com/bitcoin/bitcoin/pull/30951

💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2368086019)
> > Dropped the first refactoring commit
>
> Do you think that was the cause of the indeterminism, or are you still investigating that? (might be a followup)

Another commit with a fix for indeterminism has been pushed.

The PR description has been updated accordingly.

👋 stratospher's pull request is ready for review: "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency"
(https://github.com/bitcoin/bitcoin/pull/26988)

🤔 furszy reviewed a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2321929633)
Code review ACK

Commit 19bc71caf6a43702946f1bc15696fe14afa24fda description says "delay" when it should say "check_interval".

💬 furszy commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771246533)
nit:
`PEP 8: E302 expected 2 blank lines, found 1`

Should add another blank line above and below this function.

💬 furszy commented on pull request "test: Introduce ensure helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771251127)
ultra tiny nit: same code, a bit shorter.
```python3
predicate_source = "''''\n" + inspect.getsource(predicate) + "'''"
while True:
if not predicate():
raise AssertionError("Predicate {} became false within {} seconds".format(predicate_source, duration))
if time.time() > time_end:
return
time.sleep(check_interval)
```

💬 stratospher commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-2368089819)
Rebased. thanks @jonatack! reaching out.

💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2368093192)
@glozow @instagibbs It occurs to me that with the change in this PR, the `Cluster` will really only be used in tests anymore, as `DepGraph` objects can and will be built incrementally using `AddTransaction`, `AddDependencies`, and `RemoveTransactions`.

Because of that, I think it would be simpler to get rid of `Cluster` entirely, and reformulate the tests to all operate on `DepGraph` directly. I can do this in this PR, or as a follow-up. What do you think?

🤔 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
...