🤔 theuni reviewed a pull request: "test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX`"
(https://github.com/bitcoin/bitcoin/pull/27717#pullrequestreview-1436480137)
Concept ACK.

I initially complained about the approach [here](https://github.com/hebasto/bitcoin/pull/15#discussion_r1192682955), but it seems it's just following the current convention.

💬 theuni commented on pull request "test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX`":
(https://github.com/bitcoin/bitcoin/pull/27717#discussion_r1200420105)
Shouldn't this (and similar for the other) append `EXEEXT` to `./bitcoin-util` here?

💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200426349)
I guess I think it might be better to not do multiple parallel requests to outbound peers (on the one hand hopefully your outbounds are trustworthy and will respond quickly; on the other hand it's undesirable if listening nodes end up having to send up to 3x as much data when non-listening nodes request blocktxns from all their hb peers), but otherwise this makes sense.

💬 hebasto commented on pull request "test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX`":
(https://github.com/bitcoin/bitcoin/pull/27717#discussion_r1200427628)
I guess not. This line is about parsing of the [`test/util/data/bitcoin-util-test.json`](https://github.com/bitcoin/bitcoin/blob/master/test/util/data/bitcoin-util-test.json) file.

The correct file extension has already been added in the `execprog` variable above, or provided in an environment variable.

💬 theuni commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1557129134)
Nice! Huge concept ACK. Haven't looked at the approach/code yet but will review.

💬 theuni commented on pull request "guix: remove redundant glibc patches":
(https://github.com/bitcoin/bitcoin/pull/27670#issuecomment-1557137398)
Post-merge utACK 3cfe366ec35eebfc0dad89ac7a09c32c45c30ea5. Makes sense to me.

💬 theuni commented on pull request "random: drop syscall wrapper usage for getrandom()":
(https://github.com/bitcoin/bitcoin/pull/27699#discussion_r1200453413)
TIL `[[maybe_unused]]` + `static` could be used for this effect. Neat :)

💬 theuni commented on pull request "random: drop syscall wrapper usage for getrandom()":
(https://github.com/bitcoin/bitcoin/pull/27699#issuecomment-1557155077)
Ping @EthanHeilman. Not that I'm worried, but... curious if any part of what you're working on could verify that this is indeed a noop?

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200499146)
done

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200499309)
took @ajtowns suggestion

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200499376)
done

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200499431)
done

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200499502)
fixed

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200499579)
leaving as-is, reads more naturally to me

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200500047)
added more clarifying comment and an `Assume` for now

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200509401)
> I guess I think it might be better to not do multiple parallel requests to outbound peers

That makes some sense since using an outbound is mostly for the anti-sybil portion of the reasoning. Once we have an outbound hb peer, they're likely highly reliable, and we probably don't need to try so hard for other connections. Maybe even allow only up to two attempts if we already have an outbound in flight.

💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557224231)
responded to all nits

let's keep the issue open for further discussion on bandwidth saving strategies

📝 hebasto opened a pull request: "ci: Ensure git is installed before usage"
(https://github.com/bitcoin/bitcoin/pull/27718)
Fixes https://cirrus-ci.com/task/6690296436621312.

https://api.cirrus-ci.com/v1/task/6690296436621312/logs/build.log:
```
./ci/test/01_base_install.sh: line 11: git: command not found
```

🤔 theuni reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1436636736)
A few high-level conceptual questions that weren't obvious from a quick skim:

What's the threading model of the notification interface? For example, might it be possible that I:
- Receive a fatal error callback before the parent function has completed
- Not yet received the fatal callback before calling another function

I guess your answer to the latter would be "the caller should've received an error as well". See my other comment about my concern there.

💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200518879)
Sorry for the late conceptual review, but I'm worried about how true this is and how well it's enforced? IIRC last time I played with my clang-tidy plugin, there were at least a few places where StartShutdown() was called (maybe in a nested function) but then the function didn't return immediately.