💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2379079961)
Nice! I'll try to use this for `Sv2Connman` in https://github.com/Sjors/bitcoin/pull/50 and will let you know if anything is missing.

Can you put `sockman.h` in `libbitcoin_common` instead of `libbitcoin_node`? For the Template Provider I'm trying to prevent a circular dependency on the node. I think this can be done by moving a few more things out of net.h. This should do the trick: https://github.com/bitcoin/bitcoin/commit/4dd51b2924860bf10466da080ea4ff7bed1a3e3f

👋 brunoerg's pull request is ready for review: "p2p: rpc: add `tx_reconciliation` to `getpeerinfo`"
(https://github.com/bitcoin/bitcoin/pull/30984)

💬 antonilol commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2379086150)
How hard is it for attackers to deliberately insert collisions? Is it worth to (try to) counteract this?

💬 maflcko commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1778257807)
nit: Arrays are sorted and I don't think it is user-friendly to shuffle them by ID. I think this comment can just be removed.

💬 maflcko commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2379102811)
> Would it be beneficial for users if we started shipping DEB and RPM packages? The package manager would handle the proper installation of multiple executables.

IIRC there were some in the packagin repo, but they were both removed due to being unmaintained, see https://github.com/bitcoin-core/packaging?tab=readme-ov-file#bitcoin-core-packaging

💬 sstone commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-2379167604)
> How hard is it for attackers to deliberately insert collisions? Is it worth to (try to) counteract this?

The index is currently based on suggestions by @hodlinator and @antonilol and the key for a given outpoint is the last 8 bytes of the outpoint's tx hash, represented as an 8 bytes integer, to which we add the outpoint's output index.
=> to generated collisions attackers need to generate transactions which hash to values that have the same first 8 bytes. I guess that would not be that d
...

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