💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813734)
Done

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918813936)
Taken, thanks

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1918814186)
Done

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2596117348)
Forced pushed from ad1bc03245181b00a25ea0182373eddae1c151e1 to fd3103142de8bcb92b5db6a6501c4c66abc12abd see [diff](https://github.com/bitcoin/bitcoin/compare/ad1bc03245181b00a25ea0182373eddae1c151e1..fd3103142de8bcb92b5db6a6501c4c66abc12abd)

Changes
1. Addressed @Sjors comments

💬 glozow commented on pull request "p2p: track and use all potential peers for orphan resolution":
(https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1918820905)
yes, I think it needs to be a `wait_until`

👍 ryanofsky approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2556604025)
Code review ACK 39eabfa433e0bb9d85c67c6f9ce74c1570a21417. Since last review just simplified the test and clarified documentation a bit (thanks!)

💬 ryanofsky commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918817921)
In commit "test: check difficulty adjustment using alternate mainnet" (90b6c189d9b18203e119a5f3478f7b0da7d71392)

Note: this seems ok, but I guess it could be simplified a little since COINBASE_OUTPUT_DESCRIPTOR is unused, and no spending is done anymore.

In case you do want to bring back the spending test that seems fine if it's explained a little. I wasn't necessarily asking to remove it before, but was just confused by why it was there and a little overwhelmed with the details chain gene
...

👋 Sjors's pull request is ready for review: "Add multiprocess binaries to release build (except Windows)"
(https://github.com/bitcoin/bitcoin/pull/30975)

💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2596134377)
The first commit now bumps the multiprocess dependency. @ryanofsky feel free to extract into its own PR if you want to land it faster.

I don't it's necessary to wait for #31375 and #31161 so I've marked this ready for review. But if folks disagree, I can make it draft again.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2596141152)
Rebased with an updated #30901 and implemented @sipa 's suggestion here: https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2511719781 This took me a bit more time than I anticipated and a couple of times I wasn't sure which approach to go with. This is why I have kept this part as a separate commit for now and I am looking for approach ACKs on that last part in particular first before I squash it into the appropriate previous commits.

The most interesting pieces IMO:
- Dual member a
...

✅ l0rinc closed a pull request: "refactor: modernize recent `ByteType` usages and read/write functions"
(https://github.com/bitcoin/bitcoin/pull/31601)

🚀 fanquake merged a pull request: "doc: fix minor typos in comments"
(https://github.com/bitcoin/bitcoin/pull/31673)

💬 mzumsande commented on pull request "test: p2p: fix sending of manual INVs in tx download test":
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2596162834)
> My understanding is that a change like this would make the test fail on master, and pass with the patch in the pull request as it is now. But I haven't tried it, so I may be missing something?

I suspect that TheStack was looking for something that would prevent the underlying issue (sending wrong inv type) in general:

For example we could add an `assert` in `send_message` of `p2p.py` if we try to send an `Inv` of type `MSG_TX` and have `self.wtxidrelay` set (and vice versa). But it seems
...

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918849236)
I'm fine with leaving it out.

I also dropped the `COINBASE_OUTPUT_DESCRIPTOR` constant and moved the descriptor itself to the README.

💬 brunoerg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918868486)
I had same doubt. I understand that the generated address would be watch-only if we do not import as it's done in the test.

💬 brunoerg commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1918877108)
0f0ce579fffe533534dc117c854ebef3f277f18e: nit: No need to set `require_checksum`. It's false by default.

💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1918878124)
Why does `CMAKE_REQUIRED_LIBRARIES` get `LDFLAGS` rather than setting `LINK_OPTIONS`?

💬 theuni commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1918882693)
Ah, I see your comment here: https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1917262536

I think a static lib would work fine as an option though? Or -l/path/to/libfoo.a otherwise?

💬 hebasto commented on pull request "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally":
(https://github.com/bitcoin/bitcoin/pull/31662#discussion_r1918886382)
> I think a static lib would work fine as an option though?

Correct. This is documented by CMake.

👍 ryanofsky approved a pull request: "Add multiprocess binaries to release build (except Windows)"
(https://github.com/bitcoin/bitcoin/pull/30975#pullrequestreview-2556701386)
Code review ACK 12357ee9482dafb021350965fb3c3ea718fa126c assuming CI passes since it is still running currently. I'll go ahead and update #31375 after this but I don't think it needs to be a dependency. It also seems good to bump libmultiprocess version in this PR I generally think it is it is good to batch changes and only bump where needed in cases like this.