💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2657659979)
Addressed @sipa 's feedback and rebased on the latest version of #30901.

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1955183145)
Fixed.

💬 fjahr commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2657674233)
tACK 18cc0a55595f9dc1f2e561743201a05754996b64

Again, I am not an expert on cmake but I don't see any downsides in the latest changes and I can confirm it works. I am using this latest version in #28792 now. And I find the change on function names and args is making things a bit nicer.

💬 hebasto commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1955190524)
Would you mind elaborating on your suggestion?

💬 achow101 commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955192539)
This function basically duplicates `GetRNDRRS()`; it should instead be refactored to avoid duplicating.

📝 TheCharlatan opened a pull request: "init: Take lock on blocks directory in BlockManager ctor"
(https://github.com/bitcoin/bitcoin/pull/31860)
This ensures consistent directory locking for users of the kernel library by disallowing mistakenly creating multiple BlockManagers that might write into the same block or undo files.

The change here makes `LockDirectory` more strict: It now returns an error even if it is called again on the same directory from the same process. However from the description in https://github.com/bitcoin/bitcoin/pull/12422 , where this feature was introduced, it is not immediately clear what its usage was at t
...

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955196258)
Yeah, given that the file isn't committed its size doesn't matter that much. And it may be that `constexpr` evaluating a huge amount of data is non-trivial for the compiler, unsure. This isn't that important.

📝 fjahr opened a pull request: "RFC: Generated headers with ""_hex user-defined literal"
(https://github.com/bitcoin/bitcoin/pull/31861)
This is based on a [suggestion by sipa](https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1934116502) in #28792.

The generated header could be a lot smaller when using the ""_hex user-defined literal defined in `util/strencodings.h`. Primarily this would be nice for an embedded asmap file but since we already generate these headers it's a conversation that doesn't have to be tied to the asmap PR.

I am looking for feedback on:
- Do people take issue with including `util/strencoding
...

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1955205625)
Opened #31861 so it can be discussed there :)

💬 achow101 commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2657705795)
I am able to replicate @hodlinator's errors with cross built binaries.

💬 jonatack commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2657714870)
> During designing the BIP, we decided that LIMITED NODES should always cut off at HEAD-288 to avoid getting fingerprinted by leaking the prune depth.

Maybe the BIP and documentation here should clarify that limited nodes are defined by both the limited bit being set and by the node network bit not being set, as described in https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2654830209.

💬 theuni commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2657722669)
Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.

💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2657724719)
> > Is this failure introduced by this PR, or does it occur on the master branch as well?
>
> I think the _way_ cross-built binaries fail was new thanks to your introduced "0. Baseline, no errors."-test.

Right.

Even with an additional commit that fixes another [check](https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2651918799) in this test, some of the subsequent checks reveal their flaws. Fixing these issues will require removing non-portable behaviour in the wallet code. I'
...

📝 hebasto converted_to_draft a pull request: "qa: Fix `wallet_multiwallet.py`"
(https://github.com/bitcoin/bitcoin/pull/31410)
Fixes https://github.com/bitcoin/bitcoin/issues/31409.

The first commit prevents possible false positives by ensuring that each condition potentially causing the "Error scanning" log message is tested separately.

The second commit disables a problematic check on Windows.

💬 hebasto commented on pull request "cmake: Revamp handling of data files":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2657729422)
> Hmm, something here has broken the dependency flattening. I'm seeing a stall again due to the file generation.

What CMake version?

🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2616014706)
Thanks for the reviews!

Rebased 0e4ee158cadc3eb8f6af1b33440ae9a95fe19487 -> edb8a72226b362ee60e4184fd8f56d1e588ce5b6 ([`pr/wrap.18`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.18) -> [`pr/wrap.19`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.19), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.18-rebase..pr/wrap.19)) due to conflict with #31818

---

re: https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2612338457

> Moving the python chan
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955118691)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952926320

Thanks, applied suggestion

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955114378)
https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952940524

> This is equivalent to [...] because `try_exec()` would only return if `search_system_path` is `true`

IMO, this code should work correctly even if `try_exec()` returns true when it succeeds. There's no reason a future implementation of `try_exec()` might not return true when it succeeds or that someone reading this code should automatically know try_exec() doesn't just act like a normal function and return true when it s
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955091370)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952885840

(note: renamed this variable to `fallback_os_search` in the latest push)

> This means that the variable search_system_path will be wrongly set to true in this case.

It will be set to true, but I don't think it will be "wrongly" set to true. This is a heuristic deciding whether or not to let operating system look for executables if we can't find them ourselves. It's very possible different behaviors may be more or l
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1955128191)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1953063825

Thanks! Applied