🤔 janb84 reviewed a pull request: "doc: correct topology requirements in submitpackage helptext"
(https://github.com/bitcoin/bitcoin/pull/33630#pullrequestreview-3350421480)
ACK 3d222825642bfb052ce40cbc1c69318a0d8835bf

PR updates helptext of submitpackage to be up 2 date with the reent changes in #31385 . New help text reflects changes in #31385 and is clear.

Thanks for incorporating my nit!

👍 instagibbs approved a pull request: "doc: correct topology requirements in submitpackage helptext"
(https://github.com/bitcoin/bitcoin/pull/33630#pullrequestreview-3350431854)
ACK 3d222825642bfb052ce40cbc1c69318a0d8835bf

🚀 fanquake merged a pull request: "doc: correct topology requirements in submitpackage helptext"
(https://github.com/bitcoin/bitcoin/pull/33630)

💬 fanquake commented on pull request "doc: correct topology requirements in submitpackage helptext":
(https://github.com/bitcoin/bitcoin/pull/33630#issuecomment-3415826380)
Backported to `30.x` in #33609.

🤔 rkrux reviewed a pull request: "wallet: Expand MuSig test coverage and follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33636#pullrequestreview-3350082420)
Concept ACK 73a5d9315340942f030e6586c640b9240498f8ac

Adds more functional tests that are preferred.
I also like the last commit 73a5d9315340942f030e6586c640b9240498f8ac quite a lot, thanks for adding it.

💬 rkrux commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2439794359)
In c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"

`Miniscript` pattern or `Multipath` pattern?

💬 rkrux commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2440002764)
In https://github.com/bitcoin/bitcoin/commit/c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"

Nit: can rename this function to `test_success_case` now.

💬 rkrux commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2440229322)
In https://github.com/bitcoin/bitcoin/commit/c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"

I noticed some redundancy in both the success and failure cases and I tried to refactor this file based on the following points. Please find full diff below and consider extracting out commonalities.

1. Separated the loop in the `do_test` func so that wallet creation and expectation calculations are not together.
2. Because of the above separation, extracted `create_wa
...

💬 rkrux commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2439990998)
In c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"

Nit: extra blank line

💬 rkrux commented on pull request "wallet: Expand MuSig test coverage and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33636#discussion_r2440231846)
In https://github.com/bitcoin/bitcoin/commit/c7c9dc2d8634bc79e76783be113f8d42cee63c6e "test: Add musig failure scenarios"

I don't think this comment is necessary because the tests pass even if the failure cases run first.

💬 fanquake commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3415896069)
Backported to `30.x` in #33609.

💬 sipa commented on pull request "refactor: optimize block index comparisons (1.4-7.7x faster)":
(https://github.com/bitcoin/bitcoin/pull/33637#discussion_r2440314209)
Mind using `bench.batch(n * n).units("cmp")` or so here, to put the output units in perspective?

💬 fanquake commented on pull request "doc: Add troubleshooting note about Guix on SELinux systems":
(https://github.com/bitcoin/bitcoin/pull/32442#issuecomment-3415995854)
Related upstream issue: https://codeberg.org/guix/guix/issues/3576

💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor (if `internal=false`) & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-3416218823)
> lgtm crACK [664657e](https://github.com/bitcoin/bitcoin/commit/664657ed134365588914c2cf6a3975ce368a4f49)
>
> Good find! Though I find the last paragraph in the PR description slightly confusing:
>
> > If you remove label from external descriptor import object - it will import no problem:
> > Even tho it should NOT, as the descriptor is ranged.
>
> If the label is not present at all, the ranged descriptors can/should be imported. Instead, I find the text in this comment to be helpful
...

💬 fanquake commented on pull request "build: Move CMAKE_SKIP_INSTALL_RPATH from CMake to Guix script":
(https://github.com/bitcoin/bitcoin/pull/33470#issuecomment-3416250460)
Can you please fix the PR description and commit message? `CMAKE_SKIP_INSTALL_RPATH` is removed from the build system, and `CMAKE_SKIP_RPATH` (which does more than `CMAKE_SKIP_INSTALL_RPATH`) is added to the Guix build. Can you also remove the (what looks like) AI generated content from the PR description and commit message. i.e the bullet pointed list of changes, for a 2 line diff.

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2440527930)
Note that nothing is written to disk here, this is all happening in-memory. It doesn't look like the actual from-disk behaviour here is easy to test, because that's relying on `BlockManager` internals, so this is probably the closest we can test. (`nStatus` being a non-const public member while `m_dirty_blockindex` being private is... not great)

I think it makes sense to add this as a separate test case, it's not really related to `invalidateblock` and I find targeted tests easier to understa
...

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2440542622)
> so if it's just readability that is an advantage now, I have a slight preference for the current approach

Okay, no big deal either way. I'm not sure the performance difference is going to show up in any meaningful way so I generally prefer readability until the need for performance improvement is shown (or obvious), but up to you and either is fine.

💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2440539684)
> because we're iterating through block indices in increasing order of heights

Yes you're right, I hadn't factored this in. Thank you for pointing out my mistake, the logic looks correct indeed.

> makes sense to test this migration behaviour. done in https://github.com/bitcoin/bitcoin/commit/cfadc8038c08f9804c81af7950164483761f1db5.

Thanks, great to have more test coverage!

💬 sipa commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2440626085)
I think this is resolved now, with the merge of #32313?

💬 sipa commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2440648496)
I think the comment is a bit misleading. When calling `FindNestBlocks` (= at the end of this function), LastCommonBlock must be an ancestor of BestKnownBlock, but this isn't the case *during* `FindNextBlocksToDownload` (one of its functions is making sure LastCommonBlock is set to something satisfying that property).