💬 brunoerg commented on pull request "net: add ASMap info in `getrawaddrman` RPC":
(https://github.com/bitcoin/bitcoin/pull/30062#issuecomment-2102986141)
cc: @virtu @fjahr

📝 fanquake opened a pull request: "contrib: use ENV flags in get_arch"
(https://github.com/bitcoin/bitcoin/pull/30074)
This isn't an issue right now (because the get_arch check is simple), but becomes one as soon as we want to use `lld` for linking, and need LDFLAGS (otherwise we call `ld` and fail, see it's usage in #21778). So I've split this out for review. It also makes sense to use the same flags for all compilation in these checks.

Also drops some dead code in test-symbol-check.

💬 fanquake commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2102992159)
Redone this to be less convoluted. No-longer dropping our native LLVM Clang in here; just switching to LLD, dropping cctools & libtapi. Will do Clang in a followup, it gets too messy otherwise. Currently based on #29739 and #30074. Can split the other cctools changes if wanted, but they are pretty simple. The qrencode AR change likely needs to be applied globally in depends.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595684897)
done

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595685042)
added comment

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595685102)
done

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595701850)
fwiw that test is a single tx rbf, so even without this line it should pass. With package RBF it might result in multiple `RemoveStaged` calls on same conflicts, though I'm unsure what exactly that would do.

👍 theuni approved a pull request: "build: swap cctools otool for llvm-objdump"
(https://github.com/bitcoin/bitcoin/pull/29739#pullrequestreview-2048381271)
ACK 7f5ac4520d1553170b1053a9ffcd58179386a6d2. Tested `make deploy` on native macOS. Looks good.

💬 theuni commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#discussion_r1595707431)
Seems this is a bugfix regardless?

💬 glozow commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595707676)
Oh yes I was confused. Then I wonder why this is needed...?

💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1595715668)
Should be fixed now, sorry for the delay.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595720234)
In package RBF contexts, the 2nd time `Finalize()` is called in a subpackage, the child tx, it will try and enumerate the `m_all_conflicts` list again, but those transactions were already removed, so we would crash.

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1595723053)
elaborated on the comment a bit more

📝 hebasto opened a pull request: "test, subprocess: Improve coverage report correctness"
(https://github.com/bitcoin/bitcoin/pull/30075)
As a child process uses the [`execvp()`](https://linux.die.net/man/3/execvp) call, an explicit dumping of the collected profile information is required.

Coverage:
- on the master branch:

- with this PR:

💬 josibake commented on pull request "crypto: add `NUMS_H` const":
(https://github.com/bitcoin/bitcoin/pull/30048#discussion_r1595777133)
Oh! I see , I misinterpreted what you were suggesting. This does seem better, will update.

💬 mjdietzx commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1595783449)
> but I don't see how it matters since all participants are equivalent anyway

exactly, all participants are equivalent, and the intention of this `random.sample` is to prove that. maybe it's a little extra to even have this `random.sample`, but participants being equivalent is critical to the intention of this multisig so I thought the randomness was a nice to have

💬 mjdietzx commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1595783876)
Done

💬 mjdietzx commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1595792597)
All participants are equivalent [as discussed in related review comment](https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1595783449). Maybe it's extra and we shouldn't even bother with `random.sample`. If you'd prefer that lmk and I can remove

Why I included it in first place:
- all participants being equivalent is a critical property of this multisig. I know it's clear behavior of `thresh`. But I went ahead and asserted it in this test. Otherwise as signers decay, without a change
...

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1595821023)
Updated as suggested here: https://github.com/theuni/bitcoin/commits/keypairtaptweak/

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595845274)
@glozow pushed some more detailed docs, thanks!