💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1778767304)
Removed, thanks!

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778769226)
There aren't any Windows newlines anywhere in log lines, and this pull requests isn't changing anything about that. So I'll leave this as-is for now.

💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2379495804)
`03f6cc2b4a...70c2f13f83`: fix CI failure, and address suggestions

> Can you put sockman.h in libbitcoin_common

Done.

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778770906)
Seems fine to document twice. I don't really see the risk or downside. Leaving as-is for now.

💬 fanquake commented on issue "win64-cross CI timeout after 2h ":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2379500101)
Maybe the documentation is not correct, or `ctest` is buggy.

Cross compiling master for Windows, and running `ctest --test-dir build --extra-verbose`, I see:
```bash
ctest --test-dir build --extra-verbose
Internal ctest changing into directory: /root/ci_scratch/build
UpdateCTestConfiguration from :/root/ci_scratch/build/DartConfiguration.tcl
UpdateCTestConfiguration from :/root/ci_scratch/build/DartConfiguration.tcl
Test project /root/ci_scratch/build
Constructing a list of tests
Do
...

💬 mzumsande commented on pull request "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/30984#issuecomment-2379505903)
> I was unaware of this. I'm happy to cherry-pick these two commits into one of the upcoming Erlay PRs, especially once the network messaging bits have been included.

Sounds good to me. I still think that this (and anything else that is user-facing) should make it into master/releases as soon as Erlay is functional (not necessarily activated by default) but not before.

💬 brunoerg commented on pull request "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/30984#issuecomment-2379530391)
> This looks similar to #26602 / #27797 - I don't think any Erlay PR has been merged since then, so the reasoning from back then should still apply?

I completely forgot my own past PR haha. But one of the main motivation is to improve testing which currently relies on logs.

💬 brunoerg commented on pull request "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/30984#issuecomment-2379532585)
> Sounds good to me. I still think that this (and anything else that is user-facing) should make it into master/releases as soon as Erlay is functional (not necessarily activated by default) but not before.

Sounds good to me as well. Closing this for now.

✅ brunoerg closed a pull request: "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/30984)

🤔 stickies-v reviewed a pull request: "[28.x] backports and finalize (or rc3)"
(https://github.com/bitcoin/bitcoin/pull/30959#pullrequestreview-2334037773)
code LGTM 98745e03ffc9f69f901b827e19e4d8d645a27112

Verified all backport commits are clean and make sense, and that I'm getting the same manpages. I think `doc/release-notes.md` still needs to be updated though?

⚠️ maflcko opened an issue: "ci: Intermittent issue in feature_fee_estimation.py in sanity_check_rbf_estimates (Block sync timed out after 2400s)"
(https://github.com/bitcoin/bitcoin/issues/30990)
A quick skim of the log looks like there are two blocks at the same height, but I haven't looked too closely yet.

https://cirrus-ci.com/task/6299307458953216?logs=ci#L4251

💬 mzumsande commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2379597880)
Concept ACK

> In #29618 and here, I don't see the motivation to forbid v1 connections.

Well, to gain the benefits of BIP324. For example if you are concerned about someone inspecting the traffic to your node it really doesn't matter how many BIP324 peers you have if you have just one unencrypted connection. You could also run tor-only in this case, but that has its own disadvantages and `-onlynet=onion` shares the exact same downside that if everyone did it, the network would split. If som
...

📝 ismaelsadeeq opened a pull request: "test: enable running independent functional test methods"
(https://github.com/bitcoin/bitcoin/pull/30991)
- Some test methods in the functional test framework are independent and do not require any prior context or setup in `run_test`.
- This commit adds a new option for running these specific methods within a test file, allowing them to be executed individually without running the entire test suite.
- Using this option reduces the time you need to wait before the test you are interested in starts executing.
- The functionality added by this PR can be achieved manually by commenting out code, but
...

💬 stickies-v commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1778860008)
Is there a reason to set `check_ratio`? It doesn't seem necessary?

```suggestion
CTxMemPool::Options mempool_opts{};
```

🤔 stickies-v reviewed a pull request: "init: Remove retry for loop"
(https://github.com/bitcoin/bitcoin/pull/30968#pullrequestreview-2334139707)
Strong concept ACK!

🤔 l0rinc requested changes to a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2333963688)
I have concerns about the translations, using raw format instead of adjusting the validator, and I think we could sneak in a fix for unterminated positionals.

<details>
<summary>Patch</summary>

```patch
Index: src/bitcoin-cli.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
--- a/src/bitcoin-cli.cpp (revision 8845a566
...

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778758009)
is this bug still possible now? If it is, consider updating the `strprintf` example

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778754066)
nit: we could avoid mentioning compile-time twice:
```suggestion
- *Rationale*: Tinyformat handles string parameters.
Using a string literal or `constexpr const char*` ensures
format strings are validated at compile time.
```

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778767590)
I'm not sure I understand how this change is supposed to work.

`"Copyright (C) %i-%i"` can be translated by checking the exact string from https://github.com/bitcoin/bitcoin/blob/master/src/qt/bitcoinstrings.cpp#L280, but how is `"Copyright (C) 2009-2024"` be supposed to have a translated counterpart?

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1778855186)
Unrelated:
just realized that we should eventually add `"%1"` passes, since `while ('0' <= *it && *it <= '9') {` can go beyond the string and passed accidentally - but seems to fail in tinyformat, see: https://godbolt.org/z/nbTKnrqch.
Should I add a new PR for that of do you want to add that case here?