👍 stickies-v approved a pull request: "refactor: Add [[nodiscard]] where ignoring a Result return type is an error"
(https://github.com/bitcoin/bitcoin/pull/27774#pullrequestreview-1451069356)
ACK fa5680b7520c340952239c4e25ebe505d16c7c39

I looked at all sites where `Result` is either of type `void` or some other descriptive/status type, and could not find any other places where the return value can never be ignored.

👍 theStack approved a pull request: "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`"
(https://github.com/bitcoin/bitcoin/pull/26261#pullrequestreview-1451070881)
re-ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b

> Maybe done with nits, and we can have it merged?

Agree!

🚀 fanquake merged a pull request: "refactor: Add [[nodiscard]] where ignoring a Result return type is an error"
(https://github.com/bitcoin/bitcoin/pull/27774)

💬 Sjors commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1210397176)
Which version of Python are you using?

💬 mzumsande commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1210400306)
Not really sure, I've never looked into these options much and don't know about best practices - I think that if you use `-whitebind` with non-local addresses, you'd at least have to make sure that that address is not self-advertised. I guess that this applies more to `-whitebind` than `-whitelist`, so that the preferred approach in case of a non-local connection would be to use `-whitelist`?
fyi @vasild, do you have an opinion on this?

💬 stickies-v commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210393080)
I'm not sure this works? Even though the CLIs have a compatible syntax, I think they operate on their own set of resources. I'm pretty sure `podman ps` right before this line won't show anything, so I think no containers are killed after this line? At least, that's what I get when I try locally spinning up a container with `docker run` and then try to show/kill it with `podman ps` or `podman container kill --all`.

💬 stickies-v commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210393258)
> Do dangling image (layer)s have a label?

Yeah they do, just tested it to confirm.

> guarded via RESTART_CI_DOCKER_BEFORE_RUN

Fair enough, looks like this is unlikely to be hit locally. I don't think renaming to `DANGER_RESTART_CI_DOCKER_BEFORE_RUN` is an improvement, so happy to keep as is. Just thought adding a label and only applying batch operations to our own images is a good practice, e.g. if in the future we remove the `RESTART_CI_DOCKER_BEFORE_RUN` condition, given that it's al
...

💬 dergoegge commented on pull request "refactor: Guard TxRequestTracker by its own lock instead of cs_main":
(https://github.com/bitcoin/bitcoin/pull/26151#issuecomment-1568587465)
Closing this for now, can be marked up for grabs.

https://github.com/bitcoin/bitcoin/pull/26151#discussion_r1003216463 needs to be addressed for this to move forward. Imo, the interface of `TxRequestTracker` should change to internally enforce the `MAX_PEER_TX_ANNOUNCEMENTS` and `MAX_PEER_TX_REQUEST_IN_FLIGHT` limits.

✅ dergoegge closed a pull request: "refactor: Guard TxRequestTracker by its own lock instead of cs_main"
(https://github.com/bitcoin/bitcoin/pull/26151)

💬 MarcoFalke commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210414133)
I already switched all machines to `podman-docker`, because it is not possible to install `docker.io` and `podman-docker` at the same time. So `docker run` is actually just an alias for `podman run`.

💬 Sjors commented on pull request "wallet, bench: Move commonly used functions to their own file and fix a bug":
(https://github.com/bitcoin/bitcoin/pull/27666#issuecomment-1568589827)
utACK 7379a54ec416c8c0a029cc41835a23d42cb6d800

💬 MarcoFalke commented on pull request "ci: Prune dangling images on RESTART_CI_DOCKER_BEFORE_RUN":
(https://github.com/bitcoin/bitcoin/pull/27777#discussion_r1210416393)
Ok, could make sense in a follow-up to apply the label to both places and then unguard the `image prune` from `DANGER_RESTART_CI_DOCKER_BEFORE_RUN`?

💬 jamesob commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1210425053)
@Sjors this is bash :). If he's on macOS, probably some kind of zsh?

💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1210427226)
That's a good point, `whitelist` is harder to attack than `whitebind` because the attacker would have to spoof their origin IP repeatedly to fill up your inbounds. If a user `whitebind`'s a port and an attacker figures out that port numnber, they can trivially evict all your other inbounds

💬 theuni commented on pull request "kernel: Remove shutdown from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1568618485)
> > It also provides an easy way to see when calls to shutdown are made without returning to the calling function.
>
> Can you explain this? Am I correct when guessing that any added EXIT* macro, and not the BUBBLE* macros are indicating a missing return?

@MarcoFalke Right, the `BUBBLE_UP` is kinda like a cast, it's not so interesting. The others introduce conditional returns to pass any errors back up the stack. So the problem cases are ones where we don't already have that behavior.

T
...

✅ dergoegge closed a pull request: "[PoC] Structure aware fuzzing with libprotobuf-mutator"
(https://github.com/bitcoin/bitcoin/pull/26975)

🚀 fanquake merged a pull request: "wallet, bench: Move commonly used functions to their own file and fix a bug"
(https://github.com/bitcoin/bitcoin/pull/27666)

💬 dergoegge commented on pull request "p2p: Prevent block index fingerprinting by sending additional getheaders messages":
(https://github.com/bitcoin/bitcoin/pull/24571#issuecomment-1568632278)
Closing for now, can be marked up for grabs.

Someone should look into implementing the alternative approach I have described here: https://github.com/bitcoin/bitcoin/pull/24571#issuecomment-1475286226.

✅ dergoegge closed a pull request: "p2p: Prevent block index fingerprinting by sending additional getheaders messages"
(https://github.com/bitcoin/bitcoin/pull/24571)

💬 achow101 commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#issuecomment-1568642975)
ACK 5c832c3820253affc65c0ed490e26e5b0a4d5c9b