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

💬 MarcoFalke commented on pull request "ci: Ensure git is installed before usage":
(https://github.com/bitcoin/bitcoin/pull/27718#issuecomment-1557239940)
The warning is harmless. If `git` isn't installed, we can be sure that `base-install-done` is not `true`

💬 MarcoFalke commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200535398)
This seems like something that should be improved. Though, as it would be a behavior change, it may be better to do it separate from this refactoring change?

👍 instagibbs approved a pull request: "test: p2p: check misbehavior for non-continuous headers messages"
(https://github.com/bitcoin/bitcoin/pull/27712#pullrequestreview-1436656563)
ACK a97c59f12d50d11d8859f4bbfb9fcf66de667ca0

💬 instagibbs commented on pull request "test: p2p: check misbehavior for non-continuous headers messages":
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200531137)
absolutely don't have to take, I just like using this pythonism

```suggestion
block_headers = [from_hex(CBlockHeader(), self.nodes[0].getblockheader(block_hash, False) for block_hash in block_hashes]
```

💬 instagibbs commented on pull request "test: p2p: check misbehavior for non-continuous headers messages":
(https://github.com/bitcoin/bitcoin/pull/27712#discussion_r1200536118)
do we want to assert anything about not disconnecting, or the total ban score?

💬 hebasto commented on pull request "ci: Ensure git is installed before usage":
(https://github.com/bitcoin/bitcoin/pull/27718#issuecomment-1557248002)
> The warning is harmless. If `git` isn't installed, we can be sure that `base-install-done` is not `true`

You mean, checking for git binary, `command -v git`?

💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1200543291)
I'm more worried about the enforcement question. I'm nearly positive that this assumption is not true now, and even if so there's nothing other than review keeping it true in the future.

I'll work on generating a more current shutdown-bubble-up demo to demonstrate the problem.