💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1943163414)
```Suggestion
// until it is placed after all its ancestors.
```

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1943228435)
a921ae75c2f7162b3b617ddeccb84e3e60728cf8

Should we add a test that completes a cycle then asserts it's not acyclic to add coverage that way?

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1943260292)
comment clarity: "it has no effect" meaning the dependency addition?

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1943239737)
c89d147209c91bb0464321f5bc733a4eeab0dea0 in commit message:
"... and so for." and so on?

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1943200510)
in release builds this will result in `j--` wrapping around to max value of uint32_t, causing an OOB access? We should probably just stop trying to fix things if it happens?

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1943247961)
> This is only allowed when it is empty

Question: how is this enforced?

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1943352763)
c89d147209c91bb0464321f5bc733a4eeab0dea0

could mention nullptr -> "unlinked"

🤔 furszy reviewed a pull request: "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)"
(https://github.com/bitcoin/bitcoin/pull/29640#pullrequestreview-2612547860)
Code review ACK 177d07f

📝 fanquake opened a pull request: "depends: set `CMAKE_*_COMPILER_TARGET` in toolchain"
(https://github.com/bitcoin/bitcoin/pull/31849)
According to the CMake docs, this is the correct way to setup a toolchain file for cross-compilation using Clang. See https://cmake.org/cmake/help/latest/manual/cmake-toolchains.7.html#cross-compiling-using-clang

Internally it looks like CMake will only take this variable into account if it detects the compiler to be Clang, so this shouldn't effect other builds, but in the case of our Apple cross builds, we'd end up with a duplicated `--target=$ARCH-apple-darwin` on the compiler line, given w
...

💬 theuni commented on pull request "build: simplify by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1953081930)
@hebasto Looking at this again, is there any reason [this needs to be an object library](https://github.com/bitcoin/bitcoin/blob/master/cmake/minisketch.cmake#L43)? It triggers the bug that necessitates the workaround here. I'm wondering if rather than using our hack or the (better) workaround suggested upstream, if we should just make it an archive library and avoid the issue altogether?

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1953089601)
Other than the dependency itself, I don't think so. The `pypropject-build-system` is part of python itself, and IIRC `python-poetry-core` is pure python as well with no dependencies.

💬 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