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

💬 hodlinator commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1767504341)
Okay, so `FRESH` and `DIRTY_FRESH` will merge into one. :+1:

💬 pinheadmz commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1767508661)
on second thought, this behavior does make sense if a user is crazy enough to pass `IP:port` and then a second `port`