💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778688603)
nit: in tinyformat the print variant that appends a `\n` is called [printfln](https://github.com/bitcoin/bitcoin/blob/master/src/tinyformat.h#L1081-L1086) -> please consider a similar naming (I'm not necessarily recommending it, just bringing it to your attention):
```suggestion
void WalletLogPrintfln(LogFmt<Params...> wallet_fmt, const Params&... params) const
```

If you disagree, just resolve it, no explanation needed.

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778709371)
we could add a few more corner cases, e.g. `\n\n` and `\n ` -> otherwise the tests would pass for a `contains("\n")` or `count("\n") == 1` as well

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778720770)
> `Assume` should be used to document assumptions when program execution can safely continue even if the assumption is violated.

Shouldn't this be an `assert` instead?

💬 stickies-v 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_r1778745490)
Ah, I wasn't sure why that was. Fuzzer compiles and linter seems happy so updated, thanks!

🤔 mzumsande reviewed a pull request: "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/30984#pullrequestreview-2333964060)
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?

💬 davidgumberg commented on pull request "Don't zero-after-free `DataStream`: ~25% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/30987#issuecomment-2379480304)
## Appendix

<details>

<summary>

### Benchmarks

</summary>

Command being timed:
```bash
./src/bitcoind -daemon=0 -connect=amd-ryzen-7900x-node:8333 -stopatheight=815000 -port=8444 -rpcport=8445 -dbcache=2048 -prune=550 -debug=bench -debug=blockstorage -debug=coindb -debug=mempool -debug=prune"
```

I applied my branch on
[6d546336e800](https://github.com/bitcoin/bitcoin/commit/6d546336e800), which is
"master" in the data below.

Average master time (hh:mm:ss): 48:17:15 (17
...

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778761138)
It does not append a `\n`. It removes one and adds one, the effect of which is a nullopt.

I understand that this is a bit confusing and it would be better if there were no `\n` anywhere in log strings, but this can be done in the future. For example when doing other breaking changes, like switching to `std::format` in C++23 in a few years down the line. Closing for now.

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778763759)
A missing `\n` is a harmless, albeit confusing, stylistic issue in the debug logs. I'd very much say that the program can safely continue and that this is not a fatal error.

💬 sr-gi commented on pull request "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/30984#issuecomment-2379488317)
> 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 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.

💬 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