💬 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

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

👍 hodlinator approved a pull request: "test: generalize HasReason and use it in FailFmtWithError"
(https://github.com/bitcoin/bitcoin/pull/30921#pullrequestreview-2316578818)
ACK 6c3c619b35cc03e883f9d2b3326f906aedde9ba7

Reuses the existing `HasReason`-idiom in readable way while also being somewhat more efficient memory-wise thanks to the `HasReason`-ctor change.

nit is back: https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765024172

💬 pinheadmz commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1767494576)
I'm seeing something a bit unexpected here which is that the IP address passed to `rpcconnect` is still respected even though the port is ignored / overridden by `rpcport`. This came to my attention in the `interface_bitcoin_cli` test while working on another branch where I'd borked ipv6. This line in the test unexpectedly tried to connect to bitcoin on `::1` even though the port passed to `rpcconnect` was junk:

```
assert_equal(BLOCKS, self.nodes[0].cli("-rpcconnect=[::1]:18999", f'-rpcport
...

💬 laanwj commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1767502902)
Would be nice to add an assertion / check here to make that fileOutPos is within range of unsigned int before casting it, to avoid silent truncation. But yes, out of scope of this PR as the conversion isn't introduced here.