💬 hebasto commented on pull request "msvc: Provide `ObjectFileName` explicitly":
(https://github.com/bitcoin/bitcoin/pull/27687#issuecomment-1554347352)
Friendly ping @sipsorcery :)

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198791887)
Yeah, it may be odd in the meantime, but if it is done for all the lines that are touched anyway and if longer term the ui_interface lines are touched, they can also be switched over?

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198793373)
Ah sure. You didn't want to move them to common, so you moved them to util ahead of the other change.

💬 MarcoFalke commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1554355740)
> Here's a test commit that adds that functionality https://github.com/bitcoin/bitcoin/commit/f26a85059fb4eaf29f9c1652b2b5e14060d79a36

Thanks. Did you want me to push it here (removing the logging), or did you want to open a fresh pull yourself?

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`?

💬 MarcoFalke commented on pull request "[WIP] wallet: standardize change output detection process":
(https://github.com/bitcoin/bitcoin/pull/25979#issuecomment-1554360733)
Needs rebase if still relevant

💬 MarcoFalke commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1554361678)
Looks like CI failed?

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198809696)
The changes here and [here](https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198758293) are done by `git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v`. Should I always drop these format changes that just re-order includes? I already drop a bunch of other changes that re-format moved code, or squash code onto a single line, that would be easier to read with its current hand-written multi line formatting.

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198812663)
> Should I always drop these format changes that just re-order includes?

No, it is just a style nit. Feel free to ignore. I just think that instead of moving one include in the wrong section to another place in the wrong section is worse than just moving it to the right place in the right section in one go.

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198815026)
See the discussion in [this](https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1428804291) and [this](https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1550107951) comment. To summarize: Calling `std::abort` or `exit(0)` would skip over destructors and lead to an unclean exit. Functions calling `fatalError` should already be returning values that would lead to `bitcoin-chainstate` shutting down normally.

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1198817207)
I ran through the scenario in [this comment](https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1553391517). We also already have a bunch of other test-only or debug-only options, so I don't think we are setting a precedent here.

💬 brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1554377481)
> Looks like the tests fail in CI, even on a re-run?

Yes, had to touch `mempool_compatibility`.

Force-pushed:
- Fixed `mempool_compatibility` to stop node1 after calling `send_self_transfer`, not before:
```diff
--- a/test/functional/mempool_compatibility.py
+++ b/test/functional/mempool_compatibility.py
@@ -47,12 +47,12 @@ class MempoolCompatibilityTest(BitcoinTestFramework):
# unbroadcasted_tx won't pass old_node's `MemPoolAccept::PreChecks`.
self.connect_nodes
...

💬 hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554385101)
Dropped non-required refactoring commit.

💬 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.