💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1198824899)
[Leaved](https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554385101) as is.

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198831873)
Would suggest documenting this in `src/kernel/notifications_interface.h`. Maybe:

```c++
//! The fatal error notification is sent to notify the user and start
//! shutting down if an error happens in kernel code that can't be recovered
//! from. After this notification is sent, whatever function triggered the
//! error should also return an error code or raise an exception.
```

💬 ryanofsky commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198840110)
I suggested adding the bool as way to clean up the `MaybeCompleteSnapshotValidation` interface which seemed unnecessarily complicated. Could have gone with a mock notifications object instead but adding the bool was simpler.

Maybe more ideally we could drop the bool by just having the test call `BOOST_CHECK(ShutdownRequested())` and `AbortShutdown()` after it calls `MaybeCompleteSnapshotValidation`. This would make the test more realistic and add better coverage, but I didn't check if that wo
...

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1554410798)
holding off on touching for nits, can do in follow-up

💬 pinheadmz commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1554411729)
Before I re-ACK, are you gonna squash any commits?

💬 fanquake commented on pull request "random: drop syscall wrapper usage for getrandom()":
(https://github.com/bitcoin/bitcoin/pull/27699#issuecomment-1554416619)
Guix Build:
```bash
15e52e32e8ad2574fa5a1bd5e7699f33b7084a63d4ed3951a9b9f22632dd57fb guix-build-f33211a4fe56/output/aarch64-linux-gnu/SHA256SUMS.part
1c981a6d7cce696f4b562443ef53f401dfa6edc4704154f453ee4f66b8fc72d6 guix-build-f33211a4fe56/output/aarch64-linux-gnu/bitcoin-f33211a4fe56-aarch64-linux-gnu-debug.tar.gz
c8d0b81e5ad5f504650690c9b0192338b22a09aeead9cc1a48054e228704f1a8 guix-build-f33211a4fe56/output/aarch64-linux-gnu/bitcoin-f33211a4fe56-aarch64-linux-gnu.tar.gz
4e59bc9cbe2499fd
...

💬 MarcoFalke commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1198846834)
(If this nit is taken, it would also be good to add a log statement, so that if the loop is failing, one can derive easily in which iteration it failed. Generally I think any, or almost any comment in a functional test case can be a (debug)log statement to aid debugging in case of failure)

⚠️ Sjors opened an issue: "Include torrent in binary download verification "
(https://github.com/bitcoin/bitcoin/issues/27702)
The binary verification script currently covers bitcoin(core).org but not the torrents. We should add an option to download and verify those as well.

💬 MarcoFalke commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554434967)
lgtm ACK 342a0c865d8b4b00a088af7b70b1ee0df1864f5c

This just adds a duplicate entry in the normal case, because the symlink from configure is already followed by python, see https://docs.python.org/3/library/sys.html#sys.path, so the duplicate should be harmless.

Didn't check what Cmake does. I presume it copies?

💬 sipsorcery commented on pull request "msvc: Provide `ObjectFileName` explicitly":
(https://github.com/bitcoin/bitcoin/pull/27687#issuecomment-1554438083)
ACK b8ed95127b23873db973b2ef4f68d9f04e9c23ae.

Same object file name change has to be done in other places in the msvc build and doesn't affect the final build artifcat.

🚀 fanquake merged a pull request: "msvc: Provide `ObjectFileName` explicitly"
(https://github.com/bitcoin/bitcoin/pull/27687)

💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554443734)
> Didn't check what Cmake does. I presume it copies?

We use [`file(CREATE_LINK ... COPY_ON_ERROR)` ](https://cmake.org/cmake/help/latest/command/file.html#create-link).

👋 fanquake's pull request is ready for review: "25.0 Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27686)

💬 MarcoFalke commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554471723)
Any reason to change it? I presume this would lead to copies instead of hard links when the build dir is on a different storage device? symbolic links could be used to follow to a different storage device.

💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1554489167)
> Before I re-ACK, are you gonna squash any commits?

Done!

💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1554510214)
> Also, I think we dropped support for non-witness compact block relay, so I guess you can drop `txid` from your map and only use `wtxid`?

I believe that there should be some older versions of our software that support witness-compact-block-relay but don't support wtxidrelay, so I think @ajtowns patch is correct to index on both.

💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554516472)
> Any reason to change it? I presume this would lead to copies instead of hard links when the build dir is on a different storage device?

1. The `SeCreateSymbolicLinkPrivilege` [policy](https://learn.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/create-symbolic-links) is disabled by default for non-administartors on Windows.
2. Even with the `SeCreateSymbolicLinkPrivilege` is enabled, this change is still required on Windows.

> symbolic links could be use
...

🤔 josibake reviewed a pull request: "25.0 Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27686#pullrequestreview-1434354654)
ACK https://github.com/bitcoin/bitcoin/pull/27686/commits/9ded442dffb827d8762c0becce9d10fa283d1a3a

looks good, although we probably don't need to credit `merge-script` as a contributor

💬 josibake commented on pull request "25.0 Final Changes":
(https://github.com/bitcoin/bitcoin/pull/27686#discussion_r1198915559)
lol credit where credit is due, i guess?

💬 MarcoFalke commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554519947)
Anyway, should be unrelated to this pull request.