💬 TheCharlatan commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2124300838)
Guix build (aarch64):
```
00becde2dd12878e3b9f50f27899a6a8b752343dade7c71781632715c3001473 guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/SHA256SUMS.part
a685b9cee54014e74639be1e8db2d55b7c008fdb3b31c1c708c364a49b56759a guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/bitcoin-e8c25e8a35e3-aarch64-linux-gnu-debug.tar.gz
d61228158409802e5aef11c39a0da5653a6c7e870d5f500483c32c75f319e8b6 guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/bitcoin-e8c25e8a35e3-aarch64-linux-gnu.tar.gz
49447a196e
...

💬 fanquake commented on pull request "depends: Fetch miniupnpc sources from GitHub":
(https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2124309530)
> website is unavailable now.

Forever? Temporarily? Why not use the other official site?

Also no mention that this is now downloading a bunch of other stuff that we don't use?

💬 hebasto commented on pull request "depends: Fetch miniupnpc sources from GitHub":
(https://github.com/bitcoin/bitcoin/pull/30151#issuecomment-2124320183)
> > website is unavailable now.
>
> Forever?

I don't know.

> Temporarily?

I don't know.

> Why not use the other official site?

Which one? http://miniupnp.free.fr without HTTPS?

> Also no mention that this is now downloading a bunch of other stuff that we don't use?

Mentioned in the PR description.

💬 hebasto commented on pull request "ci: Add mising -Wno-error=maybe-uninitialized to armhf task":
(https://github.com/bitcoin/bitcoin/pull/30144#issuecomment-2124331610)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/207.

💬 maflcko commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#discussion_r1609646449)
I don't have freeBSD to test this, but are you sure it doesn't crash when the dtor is there but empty?

In any case, what is the point of an empty dtor? Might as well remove it, or `modernize-use-equals-default` it?

👍 hebasto approved a pull request: "doc: Correct pull request prefix for scripts and tools"
(https://github.com/bitcoin/bitcoin/pull/30150#pullrequestreview-2070693213)
ACK fa3e1151a28345edff8f371283745bdd647f9a74.

👋 fanquake's pull request is ready for review: "guix: Use LTO to build releases"
(https://github.com/bitcoin/bitcoin/pull/25391)

💬 fanquake commented on pull request "p2p: detect addnode cjdns peers in GetAddedNodeInfo()":
(https://github.com/bitcoin/bitcoin/pull/30085#issuecomment-2124384604)
Backported to 27.x in #30092.

👋 fanquake's pull request is ready for review: "[27.x] Backports and probably finalize"
(https://github.com/bitcoin/bitcoin/pull/30092)

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1609568747)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

Can remove this line.

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1609556427)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

nit: I think it's more correct to leave this as "Reason." "Reasons" implies a single transaction can have multiple reasons for being removed at the same time.

🤔 josibake reviewed a pull request: "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict"
(https://github.com/bitcoin/bitcoin/pull/29680#pullrequestreview-2070549155)
Still reviewing the later commits, but had some initial feedback/questions for the first commit.

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1609633243)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

I think it would be better to pass a dummy `CTxRef` here instead of nullptr.

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1609617090)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

Was a bit surprised that `pthisBlock->GetHash()` isn't returning a member variable and instead is calculating the hash each time its called. Worth mentioning we are adding an extra hash to `ConnectTip`. Probably negligible but wanted to mention it.

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1609583688)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

Don't we also need to `#include kernel/mempool_removal_reasons.h` , for Include What You Use (IWYU)? I'll admit, I'm totally sure what our conventions are on this. cc @TheCharlatan or @maflcko

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1609571510)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

AFAICT, these are unused (I was able to compile this commit fine without them). Maybe leftover from a different approach?

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1609644369)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

Any reason why `conflicting_block_hash` is passed by reference but `replacement_tx` isn't?

Would also be nice if we prevented these objects from be constructed with `nullptrs`. I'm not sure if we have a convention in our codebase around this or other examples to point to, but would be worth looking into.

💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1609666831)
in https://github.com/bitcoin/bitcoin/pull/29680/commits/7debac0b09faf051229a4ead0b57e6658e00ac11 ("Change MemPoolRemovalReason to variant type"):

I haven't finished reviewing the later commits yet, but seems odd to pass both `conflicting_block_hash` and `conflicting_block_height`. Seems like we should be able to only use `conflicting_block_hash`?

👍 theStack approved a pull request: "wallet, tests: Avoid stringop-overflow warning in PollutePubKey"
(https://github.com/bitcoin/bitcoin/pull/30131#pullrequestreview-2070750537)
utACK 2289d4524053ab71c0d9133987cb36412797c1a2

💬 maflcko commented on issue "test: rpc_bind.py --ipv4 fails when run on s390x or arm in debian":
(https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-2124403378)
Possibly unrelated, but `rpc_bind.py --ipv6 | ✖ Failed | 2 s`

With ASan, on GHA: https://github.com/m3dwards/bitcoin/actions/runs/9178831587/job/25239590853#step:5:5070

```
test 2024-05-21T18:27:24.024000Z TestFramework.node0 (DEBUG): RPC successfully started
test 2024-05-21T18:27:24.078000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "
...

💬 maflcko commented on issue "ci: Enable bpfcc-tools":
(https://github.com/bitcoin/bitcoin/issues/29804#issuecomment-2124403920)
Nice. I think you can just temporarily disable rpc_bind, due to https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-2124403378 I presume?