🤔 vasild reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2595012158)
Approach ACK 99b7b857d3b8b9ce9bd7501e2480583369740c55

I think it would be best if all sources are build with the same flags and the same build mechanism is used by devs, CI and depends. I also do not see a harm of an extra option which is off by default, to use an external libmultiprocess, if that makes libmultiprocess' devs life easier.

💬 vasild commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942468885)
pico nit: in the commit message of b75e13e6ecce1b2b406ef83099200df42acf37e0 `scripted-diff: s/WITH_MULTIPROCESS/ENABLE_IPC/ in cmake`:

```diff
- a find_package is call is made
+ a find_package call is made
```

💬 vasild commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942473099)
`s/git submodule/git subtree/`

💬 vasild commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942477051)
The comment says "when ENABLE_IPC is enabled" but the code is `if(WITH_LIBMULTIPROCESS)`. Shouldn't that be `if(ENABLE_IPC AND WITH_LIBMULTIPROCESS)`?

💬 vasild commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942473641)
I find it could be confusing to have the same option `WITH_LIBMULTIPROCESS` before and after this PR with different semantics. Maybe `WITH_EXTERNAL_LIBMULTIPROCESS` is a better name for the new one?

💬 vasild commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942612485)
Commit 17329d17ef2f640c5f03e36351f1e19f43acf108 `lint: Add exclusions for libmultiprocess subtree` looks suboptimal because it adds exceptions for the entire `src/ipc/libmultiprocess/` subtree for a whole class of issues. Because there are a few specific issues currently. Even if they are ok, it means new issues will sneak in `src/ipc/libmultiprocess/` unnoticed.

It would be better to either resolve the issues and not exclude `src/ipc/libmultiprocess/` from linters or add exceptions for the p
...

💬 vasild commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942670069)
```suggestion
package sources, or for testing local changes that are not available to
```

💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2636453499)
@theuni
> Wait, I take that back. It seems like we could still get 2 separate `cs_main`s here: one in the lib and the other in the binary. ACK retracted while I investigate.

To clarify this discussion, which binary you are referring to—`bitcoind` or `bitcoin-chainstate`? I assume the latter, as the former uses the static kernel library.

💬 maflcko commented on pull request "rpc: add cpu_load to getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/31672#issuecomment-2636466716)
> I guess you mean that message processing is single threaded - the `b-msghand` thread. I agree it could change in the future.

I left the comment, because if the work is done in a different thread, this feature will silently break. I guess that is fine if the work is minimal, or code could be added to do the additional accounting if the work is sustantial.

💬 maflcko commented on issue "bitcoind loglevel=info too noisy":
(https://github.com/bitcoin/bitcoin/issues/31796#issuecomment-2636476646)
If you don't want the logs to end up in the journal at all, you can configure it so. If you want to disable logs completely, you can do so as well. If you want to filter out info logs, you can do so as well.

You can also modify the log level locally of the log that you don't want to see (Bitcoin Core is open source).

However, I don't think modifying the log level in the source code here is the correct fix, because (as mentioned) this only happens during IBD and other people expect the log.

Us
...

✅ pinheadmz closed an issue: "bitcoind loglevel=info too noisy"
(https://github.com/bitcoin/bitcoin/issues/31796)

💬 Jaysinh146 commented on issue "When opening or autoloading wallets there should be clear messages about rescanning in progress and wallets' names.":
(https://github.com/bitcoin-core/gui/issues/259#issuecomment-2636542781)
Hey, I would love to work on this issue. Can I take it?

💬 maflcko commented on pull request "test: add validation for gettxout RPC response":
(https://github.com/bitcoin/bitcoin/pull/30226#issuecomment-2636543820)
lgtm ACK 723440c5b8eb3a815c80bfb37ad195b5448b25ed

👍 l0rinc approved a pull request: "refactor: Use std::span over Span"
(https://github.com/bitcoin/bitcoin/pull/31519#pullrequestreview-2595295815)
I have recreated the changes locally and compared it against your change and reviewed the diffs one-by-one.
Based on my understanding, your changes are accurate, nicely transitioning to the new type in careful commits, you have thought of every corner case.

Rebased the change against latest master, all tests are passing locally as well and I'm getting the same scripted diff results (rewritten slightly for mac).

<details>
<summary>Span->std::span</summary>

```bash
gsed -i "s#\<Span\>#
...

💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942699465)
nit: for consistency:
```suggestion
/** Fill a byte span with random bytes. This overrides the RandomMixin version. */
```

💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942642531)
Is this still accurate, now that we're always explicitly `const`-ing?
```
... but for const unsigned char member types
```

💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942719114)
Reviewed it in more detail, `git diff --name-only HEAD~6..HEAD` basically only contains `doc/developer-notes.md` in addition to the previous commit - which doesn't contain a copyright header. The other skipped files don't contain any dates, so it's a correct change, nicely done.

💬 l0rinc commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1942700947)
nit:
```suggestion
template <typename T, size_t N> constexpr auto UCharSpanCast(std::span<T, N> s) -> std::span<std::remove_pointer_t<decltype(UCharCast(s.data()))>> { return {UCharCast(s.data()), s.size()}; }
```

💬 hebasto commented on issue " Cannot figure out how to use std::atomic error for MacOS Sequoia 15.2":
(https://github.com/bitcoin/bitcoin/issues/31579#issuecomment-2636564824)
@egruper

> I can reproduce, attaching also build/CMakeFiles/CMakeConfigureLog.yaml.

Thank you for details provided!

The error message in the log file states: `'atomic' file not found`.

On your system, with the command-line tools installed, the `atomic` header should be located in the `/Library/Developer/CommandLineTools/usr/include/c++/v1` directory. If it is not, please try reinstalling the command-line tools.

💬 maflcko commented on pull request "func test: Expand tx download preference tests":
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2636597881)
ACK 846a1387280fa584f70ccb1f5d0198339b065528 🍕

<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: ACK 846a1387280fa584f70ccb1f5d
...