💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2379174442)
Another option that I'm looking at is simply to replace and extend `txindex` and create 2 indexes:

- txid (32 bytes) -> tx position (8 bytes) (same as txindex)
- tx position (8 bytes) + output index (4 bytes) -> spending tx position (8 bytes)

No more collisions, and we can return the full tx and not just its hash, but not as efficient as this version (which can also return the full tx)l

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

```
[10:48:14.556] 135/136 Test #1: util_test_runner ..................... Passed 550.41 sec
[12:17:58.568]
[12:17:58.568] Timed out!
```

https://cirrus-ci.com/task/5511579989442560?logs=ci#L2605

💬 maflcko commented on pull request "fuzz: fuzz connman with non-empty addrman + ASMap":
(https://github.com/bitcoin/bitcoin/pull/29536#issuecomment-2379194976)
review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc 🏀

<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 552cae243a1b
...

💬 ryanofsky commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2379204971)
re: https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2378828326

> We could start with seperate binaries for multiprocess, e.g.:

This makes sense. Initially I was thinking all the binaries had to live in the same place, but this is not true. We could leave all existing binaries where they are in `bin/` and just add 3 new files to releases when multiprocess option is turned on:

- `bin/bitcoin`
- `libexec/bitcoin-node`
- `libexec/bitcoin-gui`

Implementing this might be as
...

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