💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2654378982)
> I'm unable to apply the code signatures. E.g. for arm64:

They need to be committed (locally, committed by anyone with any message).

> I've run a build, but don't yet see everything matching:

It looks like the binaries match, but not the `SHA256SUMS.part` files.

👍 ryanofsky approved a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660#pullrequestreview-2612568019)
Code review ACK 8102f3d8e0b6715433eeda78b9b57a83109fc45e. Looks like #31620 made some overlapping changes, but this PR still seems very useful.

From PR description:

> Before #31620, failure to establish RPC connections would result in an attempt to stop the node via RPC, triggering knock-on exceptions. This would muddy the waters for developers looking for root causes of test failures. This PR used to solve that issue in a more roundabout way than aforementioned PR, but now merely adds tes
...

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953030408)
In commit "qa refactor: wait_for_rpc_connection - Treat OSErrors the same" (8fa04cb13a4bf62a42b18e7c0c79151d13cb494b)

Various thoughts:

- I think "initialization phase" comment that was removed provided more context and was more understandable than the new "Suppress these in favor of" comment.
- I don't see why it is useful to list later outcomes that can happen after these errors are suppressed, especially when the list seems to contains every imaginable outcome.
- I don't understand wh
...

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953084710)
In commit "qa: Avoid calling stop-RPC if not connected" (8102f3d8e0b6715433eeda78b9b57a83109fc45e)

This change seems good, but unclear what motivation is. Is it supposed to be a refactoring that makes code more robust, or were there actually any cases where CannotSendRequest exceptions were caught and suppressed? If so what were those cases? Commit description should say what goals of this change are and if CannotSendRequest is an actual or theoretical change in behavior.

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953080253)
In commit "qa: Only log node stopping info if we have any" (9d0e6a1b2aa5be186550658ff85e5ad643554338)

Would be helpful if commit description stated motivation for this change. After looking at the `stop_nodes()` method implementation it seem like this is a logging-only change and point is just to remove stopping log statements when there are no nodes. But it would be good if actual intent of change were explicit.

💬 ryanofsky commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1953060817)
In commit "qa: Add feature_framework_startup_failures.py" (2226e1489101f363114ce474d7f5e56ec50c9c6a)

Maybe give these arguments a prefix like `--internal-` so it obvious they are internal, and to avoid risk of silent conflicts if real test arguments with the same names are added later.

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2654397674)
> > I've run a build, but don't yet see everything matching:
>
> It looks like the binaries match, but not the `SHA256SUMS.part` files.

Ah, I think it's probably that the ` bitcoin-096525e92cc2-codesignatures-<commit>.tar.gz` file is the one that's different in the `SHA256SUMS.part` files as I did not use the same codesignatures commit that @pinheadmz made. I was untarring and committing the signatures into my local repos.

👍 laanwj approved a pull request: "depends: add missing Darwin objcopy"
(https://github.com/bitcoin/bitcoin/pull/31840#pullrequestreview-2612710149)
Code review ACK 3edaf0b4286a771520b7e5b0b5064eca713ff0ad

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953116997)
It's clearer using `* 1024` IMO

👍 theuni approved a pull request: "depends: add missing Darwin objcopy"
(https://github.com/bitcoin/bitcoin/pull/31840#pullrequestreview-2612728849)
utACK 3edaf0b4286a771520b7e5b0b5064eca713ff0ad

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953122891)
It's to disable pruning during IBD.

I'm not seeing a use case to set this lower than `-prune` (we already prune extra during IBD when we prune), only higher (to avoid flushing).

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953123976)
Merge it into master for testing. Rebasing for no reason is a bad practice.

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953126552)
There's no such thing as orphan blocks. Stale blocks are implicitly included in the more general "blocks".

Anyway, this is just a move.

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953130763)
Outside the scope of this PR

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953132732)
It's a new PR, why base it on an old state? I ran the test locally and they're passing, but rebasing the PR assures that CI also runs against the latest changes.

👍 theuni approved a pull request: "cmake: Copy `cov_tool_wrapper.sh.in` to the build tree"
(https://github.com/bitcoin/bitcoin/pull/31722#pullrequestreview-2612746737)
utACK e3c015276962192355e16ca91391b8bc8e188291

💬 luke-jr commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953134231)
idk, not sure why it matters

💬 l0rinc commented on pull request "Add -pruneduringinit option to temporarily use another prune target during IBD":
(https://github.com/bitcoin/bitcoin/pull/31845#discussion_r1953134261)
Looks like we should update MIN_DISK_SPACE_FOR_BLOCK_FILES in that case in a separate PR.

🤔 vasild reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2612338457)
Approach ACK 0e4ee158ca.

> I wonder if python test framework changes here are scaring reviewers from this PR?

Moving the python changes to another PR would probably speed the review of this one and probably delay the review of the other one (with the python changes).

💬 vasild commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1952926320)
Is only used in this file:
```suggestion
static void ExecCommand(const std::vector<const char*>& args, std::string_view wrapper_argv0)
```