📝 hebasto opened a pull request: "guix: Drop no longer needed `PATH` modification"
(https://github.com/bitcoin/bitcoin/pull/30989)
I don't see a reason why this would be necessary in the master branch @ d812cf11896a2214467b6fa72d7b763bac6077c5.

Additionally, from https://github.com/bitcoin/bitcoin/pull/30940#pullrequestreview-2322355196:
> 2. I don't understand why "In the Guix environment, `${BASEPREFIX}/${HOST}/native/bin` is added to the `PATH` environment variable," according to the description. Setting this seems indiscriminate, like a sledgehammer approach, something that would cause the guix build to behave di
...

💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2379355822)
@ryanofsky
> 2. I don't understand why "In the Guix environment, `${BASEPREFIX}/${HOST}/native/bin` is added to the `PATH` environment variable," according to the description. Setting this seems indiscriminate, like a sledgehammer approach, something that would cause the guix build to behave differently from normal depends builds and lead to confusing issues like this one.

Addressed in https://github.com/bitcoin/bitcoin/pull/30989.

👍 theStack approved a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2333878604)
Code-review ACK a4e1ba722be5dbd71a3d21e5da82aee5409d22bd

Thanks for following up!

> Yeah, now that all the other PRs got merged, things changed a bit. Aside from the consistency reasons, this PR is important for people running on HDDs. See https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1952211224, which shared a pretty nice time improvement. Going from a 30 minutes migration to one that took only 80 seconds.
Still, that was a long time ago. Will give it a new run on an HDD an
...

💬 sdaftuar commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2379410875)
> I'm not sure I fully follow your point regarding rule 6. Currently, can't the dummy transaction one creates to circumvent rule 2 simply have a really low fee rate? That would basically also create a situation akin to rule 2 not existing?

Yes, that's true, and I think this is what @glozow was getting at as well:

> (2) it doesn't do its job within the RBF rules of helping us check that replacements have a better mining score / would confirm faster

My point is merely that if we look at a
...

💬 fanquake commented on issue "win64-cross CI timeout after 2h ":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2379431744)
> It is just 2 hours of nothing (no logs, etc):

This is also odd, because the `ctest` default global timeout, should be 25 minutes: https://github.com/Kitware/CMake/blob/master/Tests/CMakeLists.txt#L331:
> Use 1500 or CTEST_TEST_TIMEOUT for long test timeout value,
> whichever is greater.

As far as I'm aware, we aren't setting `CTEST_TEST_TIMEOUT`, or increasing this limit in any way, so the process should already be getting killed after that amount of time?

🤔 l0rinc reviewed a pull request: "log: Enforce trailing newline at compile time"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2333834045)
Thanks for automating these!
I left a few naming notes, a few additional test recommendations and an Assume -> assert suggestion based on its description.

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778709023)
This was already tested in `ConstevalFormatString_NumSpec` - maybe we could move the comment or remove that example

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778678560)
nit: if you edit again (the spirit of @hodlinator is haunting me here):
```suggestion
BOOST_CHECK_EXCEPTION(WrongFmt::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason{error});
```

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778695202)
nit: given that we're removing `\n` specifically in https://github.com/bitcoin/bitcoin/blob/fabf6dcdd1b1e1250936444a80ef25fdf737e2f2/src/wallet/scriptpubkeyman.h#L259 - should we document what we expect to happen with Windows newlines (which also end with `\n`, but we're not removing `\r` and adding back the `\n` later):
```C++
PassFmtNewline<0>("\r\n");
```

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778728317)
given that `PassFmtNewline<1>("\n")` would already fail - and this is an test for a missing newline, not invalid parameter count -, consider the following:
```suggestion
FailFmtWithError<0, util::TrailingNewlineCheck>("", err_newline);
```

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