💬 vasild commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2315387348)
Here and also in the commit message, this lists 3 categories: `-bind`, `-whitebind`, `onion`. There is an `-onion` config option which is not related to binding. This could be confusing. The intention here is to mean "onion" -> "a subset of -bind if suffixed with =onion". That is, just mentioning `-bind` also covers `-bind=...=onion`. So maybe change this to:

`Please check your -bind and -whitebind settings` or to
`Please check your -bind, -bind=...=onion and -whitebind settings`.

💬 vasild commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2315420542)
> since we already know which container the duplicate came from

That is known inside `CheckBindingConflicts()` but the info is not signaled to the caller of the function. Also, even inside the function, only one of the containers is known, the other is not. E.g. if the 3rd for-loop finds a duplicate, then it is unknown whether the entry was added in the first or the second for-loop.

> I think this should at least be translatable for consistency with nearby init errors

Right, +1 for tran
...

💬 vasild commented on pull request "net: Prevent node from binding to the same `CService`":
(https://github.com/bitcoin/bitcoin/pull/33231#discussion_r2315372807)
Since the new function is used only in this file:

```suggestion
static std::optional<CService> CheckBindingConflicts(const CConnman::Options& connOptions)
```

💬 optout21 commented on pull request "mini miner: enable `Linearize` return package feerates":
(https://github.com/bitcoin/bitcoin/pull/33216#discussion_r2315426655)
nit: Consider removing 'both': it is prone to going stale with future change and it adds not much information

👍 dergoegge approved a pull request: "build: set ENABLE_IPC to OFF when fuzzing"
(https://github.com/bitcoin/bitcoin/pull/33235#pullrequestreview-3175612527)
utACK af4156ab75565acc5a71b68e70da5e2cf407df80

Seems reasonable to do for now while we don't have ipc fuzz tests and don't require the dependency.

🚀 fanquake merged a pull request: "test: Fixup fill_mempool docstring"
(https://github.com/bitcoin/bitcoin/pull/33269)

🤔 hebasto reviewed a pull request: "stabilize translations by reverting old ids by text content"
(https://github.com/bitcoin/bitcoin/pull/33270#pullrequestreview-3175956732)
> stabilize translations...

Concept ACK on this idea.

Could we avoid reintroducing Python scripts into the `translate` build target?

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-3244806630)
`bfe72353c0...5d7b70161d`: rebase due to conflicts

💬 fanquake commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315721583)
Bumped to 20.

💬 fanquake commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315721957)
> I think testnet4 blockchain size is bigger

Not here:
```bash
du -csh testnet4/chainstate/ testnet4/blocks/
945M testnet4/chainstate/
8.0G testnet4/blocks/
8.9G total
```

Will increase to 22 in any case.

💬 vasild commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2315723150)
Note that exception-inside-another-exception will still terminate the program, regardless of the added `noexcept(false)`.

💬 ryanofsky commented on issue "intermittent timeout in mptest unit test":
(https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3244834811)
re: https://github.com/bitcoin/bitcoin/issues/33244#issuecomment-3243976122

> At the same time, it seems to happen frequently in CI, so I prefer my initial suggestion to either rewrite the failing unit test or remove it temporarily.

Sorry, I've been very distracted the past two weeks but I should be able to post a fix for this today.

It'd also be completely reasonable to disable the test, though I'm sure what a good way to disable it is because it's in a subtree.

💬 fanquake commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#discussion_r2315732101)
```bash
du -csh testnet3/chainstate/ testnet3/blocks/
16G testnet3/chainstate/
194G testnet3/blocks/
210G total
```
Can increase to 240.

💬 fanquake commented on issue "ci: tidy job is producing output":
(https://github.com/bitcoin/bitcoin/issues/33256#issuecomment-3244852511)
> So if the output is a problem we could turn it off globally, or apply the patch there which suppresses it locally with a NOLINT annotation

I don't think we should have spurious/undocumented output in the CI, so it'd be good to fix this.

📝 stickies-v opened a pull request: "cmake: make missing Python interpreter behaviour more explicit"
(https://github.com/bitcoin/bitcoin/pull/33278)
We require Python 3.10 for multiple targets, and currently raise a general warning if it is missing, leading to:
1. functional test suite: runtime failure if python missing (as e.g. described in #31476)
2. maintenance targets: targets skipped if python missing
3. macos deploy target: target created, but build fails if python missing

This PR:
- adds a `BUILD_FUNCTIONAL_TESTS` option (default `BUILD_TESTS`, i.e. `ON`) and raises `FATAL_ERROR` if Python is missing and we're building the func
...

💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r2315840032)
Good point! I do not know why I chose lowercase since it also violates the coding style. Maybe it was like that before and I left it as it is even though I touched those lines and the callers.

Append the following as a new commit or amend it into the last one `fuzz: make it possible to mock (fuzz) CThreadInterrupt`?

<details>
<summary>[patch] Uppercase methods of CThreadInterrupt</summary>

```diff
commit 486db14b9ce13a90b69f69dafb59d3d8932498f7 (HEAD -> fuzz_connman)
Parent: 0802398e
...

💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-3244991538)
@brunoerg, this has 2 ACKs and a stale ACK from you, maybe you would like to take a look and re-ACK it?

⚠️ fanquake opened an issue: "build: clang-16 build broken on Debian Bookworm"
(https://github.com/bitcoin/bitcoin/issues/33279)
Our minimum required Clang is 16. Building with Clang-16 on Debian Bookworm is broken due to issue with capnp. See more details here: https://github.com/bitcoin-core/libmultiprocess/issues/199.

```bash
# clang-16 --version
Debian clang version 16.0.6 (15~deb12u1)

cmake --build build
...
In file included from /bitcoin/src/ipc/libmultiprocess/src/mp/util.cpp:6:
In file included from /bitcoin/src/ipc/libmultiprocess/include/mp/util.h:8:
In file included from /usr/include/capnp/schema.h:39:
In fil
...

💬 stickies-v commented on pull request "build: Set AUTHOR_WARNING on warnings":
(https://github.com/bitcoin/bitcoin/pull/33144#issuecomment-3245006172)
> Sounds good.

Thanks for the feedback, I've opened #33278.

> Happy to put this in draft for now

The changes in this/your PR are small, straightforward, can easily be reverted, and offer immediate practical benefit, so I think it makes sense to not block this on my much less straightforward alternative, so my ACK still stands and I'd be happy if this makes progress.

💬 Sjors commented on pull request "kernel: chainparams & headersync updates for 30.0":
(https://github.com/bitcoin/bitcoin/pull/33274#issuecomment-3245015482)
re-ACK 755152ac819a23acf2f9e70316134d74a04d589b