💬 sr-gi commented on pull request "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`":
(https://github.com/bitcoin/bitcoin/pull/30984#issuecomment-2379205303)
Concept ACK

💬 hebasto commented on issue "win64-cross CI timeout after 2h ":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2379220357)
We might consider the `--timeout` and `--repeat after-timeout` flags for the [`ctest`](https://cmake.org/cmake/help/latest/manual/ctest.1.html) command.

💬 Sjors commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2379221197)
`bitcoin -m daemon -debug=net` as a format makes sense. It makes it more clear which arguments are passed into `bitcoin-node`.

💬 maflcko 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_r1778590253)
nit in 5d69f2d33b64a025d0b93b81558f2f51bd54b104: I think the `()` are no longer needed to silence the linter and can be removed: `{tfm::format_raw(desc_fmt.c_str(), ...}`

👍 maflcko approved 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-2333689157)




review ACK bb12305737e3c22325930b5bdb04028f1a6f2ef6 💅

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK bb12
...

💬 maflcko commented on issue "win64-cross CI timeout after 2h ":
(https://github.com/bitcoin/bitcoin/issues/30969#issuecomment-2379263873)
> --repeat

Any kind of automated repeat would be horrible in the CI. Some failures (often UB) are real and intermittent. Silently ignoring them makes a good chunk of the CI pointless.



> `timeout`

Yeah, sounds great. In most cases it probably doesn't hurt for the unit tests to follow what the functional tests are doing.

💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1778621747)
I think this error handling makes more sense post #30968.

I rebased this commit on top of #30968 and was able to get the same error handling behavior as before: https://github.com/josibake/bitcoin/commit/85895b870d669175f42d0730d58930bea169e221, was quite happy with the result!

💬 MrSuddenJoy commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2379291361)
@ryanofsky `bin/bitcoin` should (or could) be meta-process of all others that you mentioned. Just `symlink` maybe?

💬 maflcko commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#discussion_r1778636559)
nit in 59b4a4029cce8f2eb09c26a7c8b972df21967d9b: `chain` can't be nullptr, so it would be better to use `&` instead of `*` for self-documenting code.

This should also make the dimmed zebra smaller.

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2379324601)
(rebased, trival)

<!--


```diff
diff --git a/src/logging.h b/src/logging.h
index f465622d0b..9998f85c35 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -256,7 +256,7 @@ inline void LogPrintFormatInternal(std::string_view logging_function, std::strin
/* Original format string will have newline so don't add one here */
log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt;
}
+ if (!log_msg.ends_
...

📝 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);
```