💬 maflcko commented on pull request "test: Remove 0.16.3 test from wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/30920#issuecomment-2361566377)
> I'm trying to look for it to try and reproduce but am having trouble finding it

I think the logs were nuked in the meantime, which is a Cirrus bug :(

IIRC it may have been a shutdown error of the v0.16.3 software. However, I don't think looking at the CI failure matters, because the test doesn't make much sense and a more realistic test already exists.

💬 maflcko commented on pull request "test: Remove 0.16.3 test from wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/30920#issuecomment-2361584037)
cc @Sjors from 89a28e02fa46f3d5eb07ab02aa34aa95c6fcee11

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361591097)
> It's possible to be connected to a peer that supports the v2 service using the v1 protocol.

Thanks, am I missing something? When calling it on a node configured as `v2transport=0`:

```
Bitcoin Core client v28.99.0-18fcca480165 - server 70016/Satoshi:28.99.0(jon)/

<-> type net serv mping ping send recv txn blk hb addrp addrl age asmap id version
in onion nwl2 487 487 13 0 1 0 14 70016/Satoshi:27.0.0/
in oni
...

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361608191)
(The proposal currently only returns service flags that are present. Could optionally add a "1" if the p2p_v2 flag is unset.)

💬 sipa commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361621928)
I meant this as a comment on:

> remove the "v" transport protocol column, as that info is provided in the new peer services column

They represent different information. The "v" transport protocol column currently conveys whether or not you are *right now* speaking to that peer through the v1 or v2 protocol.

Peer services are about what a peer supports, not what protocol you're speaking with it.

If you run with -v2transport=0, the transport protocol will be v1 with all peers, includes ones th
...

👍 tdb3 approved a pull request: "test: Introduce ensure helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2316167311)
ACK dcd66472600f1676e2e450245924ab6e1a837d39

Nice addition. Compliments `wait_until` nicely.

(As expected) saw little difference in both individual and total runtimes of tests on master (141s) vs pr branch (141s), (at least within typical run-to-run difference).

### Master (e46bebb444dfff89813333dc1926a57c9a3e8aab):
| test | status | duration(seconds) |
|---|---|---|
| feature_assumeutxo.py | Passed | 32 |
| feature_minchainwork.py | Passed | 10 |
| mempool_unbroadcast.py | Passed
...

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361627003)
Ah! Thanks.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361652277)
Ah! Indeed, thank you.

```
<-> type net serv v mping ping send recv txn blk hb addrp addrl age asmap id
in onion 1 * . 0 95
in onion 1 368 556 17 37 * . 6 68
in onion nwl2 1 376 551 0 1 0 30 13 14
in onion nwl2 1 380 430 0 3 24 12 38
...

💬 marcofleon commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2361663097)
@maflcko thanks for the review! Your comments make sense to me, I'll address them in the followup.

I just added some inputs for this target to qa-assets. Here is a [coverage report](https://marcofleon.github.io/coverage/p2p_headers_presync/).

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361685482)
@sipa make it make sense to only return the "v" column if the server node is `-v2transport=0`?

💬 sipa commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361691952)
@jonatack I don't think so. It's possible that the service flag and transport type differ even with `-v2transport=1`.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2361715093)
@sipa Thank you. Dropped the commit that removed the "v" column.

💬 Sjors commented on pull request "test: Remove 0.16.3 test from wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/30920#issuecomment-2361722871)
Since we can longer create a legacy (bdb4) wallets and since older Bitcoin Core versions can't load descriptor (sqlite3) wallets, it should be safe to drop certain older nodes from the _downgrade_ tests.

Will study this PR later.

💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1767332507)
> > Long-term, I think it would make more sense to stop intentionally doing incomplete lines (since that introduces a race condition if other threads are also logging), and just add a newline if it's missing... _shrug_
>
> Fixed in #30485 (but not yet removing the `m_started_new_line` to avoid a merge conflict here). I'll do it in a follow-up

Removed the dead `m_started_new_line` in https://github.com/bitcoin/bitcoin/pull/30929

⚠️ Sjors opened an issue: "cmake: multiprocess guix build broken"
(https://github.com/bitcoin/bitcoin/issues/30931)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

Unable to make a guix build with MULTIPROCESS=1.

### Expected behaviour

To be able to.

### Steps to reproduce

Checkout https://github.com/bitcoin/bitcoin/commit/e821f0a37a026fa0480c7f6f6c938da7c77e0d52 (or master).

In guix/libexec/build.sh below `make -C depends` add `MULTIPROCESS=1` (see #30483 for why). Commit it.

Then do:

```
MULTIPROCESS=1 HOSTS=
```

The build will
...

💬 l0rinc commented on pull request "util: refactor: add and use run-time safe tinyformat::try_format":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767355895)
thanks for clarifying

👍 l0rinc approved a pull request: "util: refactor: add and use run-time safe tinyformat::try_format"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2316356184)
utACK 6dc6e4583c02f0d21dffeb6b2c81358b49a1f340

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1767021502)
nit: would it make sense to add a bool comment for these params?
```suggestion
void BaseIndex::FatalErrorf(util::ConstevalFormatString</*force_trailing_newline=*/false, sizeof...(Args)> fmt, const Args&... args)

```

🤔 l0rinc reviewed a pull request: "log: Enforce trailing newline at compile time"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2315860535)
concept ACK

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1767019782)
nice