💬 achow101 commented on pull request "test: Assumeutxo: snapshots with less work should not be loaded":
(https://github.com/bitcoin/bitcoin/pull/29428#issuecomment-2102949311)
Needs rebase (for some reason Drahtbot isn't detecting this)

💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1595654005)
True, it's only a superset of Rule 3 👍

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1595669798)
In commit "blockstorage: split up FindBlockPos function" (2b6d274af050ea21e39ce59b6a6b3b7fb61e8cbd)

I think this comment is not really accurate in this context. There should be no need to reset `BlockfileCursor::undo_height` here, only to set a new `BlockfileCursor::file_num` value so `MaxBlockfileNum()` returns the right thing. So the comment could be changed to something like "update the cursor so it points to the last file".

Maybe it would also make sense to make this a little more robu
...

💬 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