💬 theuni commented on pull request "depends: add *FLAGS to gen_id":
(https://github.com/bitcoin/bitcoin/pull/31125#discussion_r1815258014)
I wonder if this could be replaced with an actual invocation of the linker instead? Something like:

`bash -c "echo \"int main(){}\" | $CC -v $CFLAGS $LDFLAGS -Wl,-v -xc -o /dev/null -"`

💬 hebasto commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#discussion_r1815258985)
Thanks! Fixed.

🚀 fanquake merged a pull request: "ci: display logs of failed unit tests automatically"
(https://github.com/bitcoin/bitcoin/pull/31148)

👍 fanquake approved a pull request: "util: Remove RandAddSeedPerfmon"
(https://github.com/bitcoin/bitcoin/pull/31124#pullrequestreview-2393116843)
ACK 9bb92c0e7ffe2426b4afb80ddd4b4c96968316b5

👍 dergoegge approved a pull request: "util: Treat Assume as Assert when evaluating at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31150#pullrequestreview-2393133722)
ACK fa69a5f4b76a4e2a02db6c32d9c3311ce5fe29bd

💬 fanquake commented on pull request "depends: Use `CC_FOR_BUILD` for `config.guess `":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2435689061)
@hebasto can you rebase this?

💬 maflcko commented on issue "macOS 13.7 depends build can't find qt":
(https://github.com/bitcoin/bitcoin/issues/31050#issuecomment-2435689719)
> symlink

See also https://github.com/bitcoin/bitcoin/issues/31145

💬 hebasto commented on pull request "depends: Use `CC_FOR_BUILD` for `config.guess `":
(https://github.com/bitcoin/bitcoin/pull/29963#issuecomment-2435700145)
> @hebasto can you rebase this?

Rebased.

💬 fanquake commented on pull request "guix: use GCC 13 to build releases":
(https://github.com/bitcoin/bitcoin/pull/29881#issuecomment-2435706570)
Rebased, and dropped the `[WIP]` from the CI commit.

💬 theStack commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1815309496)
nit, if you retouch:
```suggestion
auto coin = (!mempool || !mempool->isSpent(vOutPoint)) ? view.GetCoin(vOutPoint) : std::nullopt;
```
(slightly easier to read imho; it's no big deal though here, maybe i'm just post-traumatized by the [serialized hash calculation bug](https://github.com/bitcoin/bitcoin/pull/28685#pullrequestreview-1692828827) where missing parantheses in a ternary expression were an issue :P)

💬 theStack commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1815321190)
nit: the `assert` is lost here, but since `std::optional`'s `value` method throws an exception if no value is available, it seems to be fine to not have it (I assume that was intentional for that reason)

👍 theStack approved a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849#pullrequestreview-2393139880)
Code-review ACK 4feaa28728442b0fd29a677d2b170a05fdf967c0

👍 hebasto approved a pull request: "guix: use GCC 13 to build releases"
(https://github.com/bitcoin/bitcoin/pull/29881#pullrequestreview-2393167239)
re-ACK a4e17239ca54ccaa237fa6c824b968ca067228e6, verified with:
```
git range-diff master f9e0010de394ad6ae5eb6a22ead7ebc116a16fd4 a4e17239ca54ccaa237fa6c824b968ca067228e6
```

Only rebased and amended the commit message since my recent [review](https://github.com/bitcoin/bitcoin/pull/29881#pullrequestreview-2363408909).

👍 hebasto approved a pull request: "util: Remove RandAddSeedPerfmon"
(https://github.com/bitcoin/bitcoin/pull/31124#pullrequestreview-2393187592)
ACK 9bb92c0e7ffe2426b4afb80ddd4b4c96968316b5, I have reviewed the code and it looks OK.

💬 darosior commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2435736744)
Rebased after #31148 merge.

👍 pablomartin4btc approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2393191955)
re-ACK 5757fdf0dc74ec9d6bbefba937d6e23b09652605

A small improvement since my last review, adding (also) benchmark name/ test name to the test common dir when `-testdatadir` is not specified.

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1815354171)
I remember this being asked before, but I can't find it - yes, `.value()` already validates the optional.

💬 theuni commented on pull request "guix: use GCC 13 to build releases":
(https://github.com/bitcoin/bitcoin/pull/29881#issuecomment-2435747523)
Concept ACK

💬 marcofleon commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2435764406)
@Eunovo I'm not seeing that reduction in coverage when I run the target. What sanitizers are you using? Also, what does the coverage say if you do `-runs=1`? (instead of letting it run for a while)

💬 instagibbs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1815386949)
I'm working on a test case for master for that case