💬 fanquake commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1210341923)
What's the reason for leaving certain mappings out of this list, i.e `#include <boost/operators.hpp>`?

I'm guessing it's because those same includes are recommended for non-test code, and mapping them to Boost Test here would just produce nonsense recommendations for other files? If so, how are you planning on working around that in future? If not, or that isn't a problem, why not do the mapping now?

👍 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-1451069282)
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.

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