💬 theStack commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#issuecomment-2369695246)
> Bikeshed: How about `bitcoin-build-config.h` for symmetry with `bitcoin-build-info.h`?

Sounds good to me, done. Both the scripted diff and the patch are now a bit larger, as also some comments/strings have to be adapted, and the .h.in filename needs to be adapted as well, but review should still be simple.

💬 hebasto commented on pull request "depends: Fix build with `MULTIPROCESS=1` in Guix environment":
(https://github.com/bitcoin/bitcoin/pull/30940#issuecomment-2369696567)
My Guix build of the demo [branch](https://github.com/hebasto/bitcoin/commits/240921-guix-mp.DEMO/) except for `x86_64-w64-mingw32`:
```
aarch64
e31ab0eeff88048301f113731a9b829df63e7db2c6a83c3a54e14b6f423aa4a8 guix-build-d8ec933456bc/output/aarch64-linux-gnu/SHA256SUMS.part
77f4a9481b4ce7df26549c6001384e066a1de3cb1b81ba950c29cc41b0b5c058 guix-build-d8ec933456bc/output/aarch64-linux-gnu/bitcoin-d8ec933456bc-aarch64-linux-gnu-debug.tar.gz
ee471d630ea0eb71c2040a3b9e408c4f1709ee29d2da4ecbc193
...

💬 maflcko commented on pull request "test: Use shell builtins in run_command test case":
(https://github.com/bitcoin/bitcoin/pull/30952#issuecomment-2370288142)
review ACK 7bd3ee62f6d6f59ca599e85f81776d282dee1539

Seems fine to replace the `python3` dependency with `sh`, assuming that `sh` is available on more systems (at least on all systems where `ls` was previously available)

📝 KristijanSajenko opened a pull request: "Create devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/30954)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ fanquake closed a pull request: "Create devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/30954)

📝 fanquake locked a pull request: "Create devcontainer.json"
(https://github.com/bitcoin/bitcoin/pull/30954)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2370378310)
> I think I need to rebase this now that https://github.com/bitcoin/bitcoin/pull/30409 is merged

Yes

📝 Sjors reopened a pull request: "Introduce waitFeesChanged() mining interface"
(https://github.com/bitcoin/bitcoin/pull/30443)
This adds `waitFeesChanged()` to the `Mining` interface.

The Stratum v2 protocol allows pushing out templates as fees in the mempool increase. This interface lets us know when it's time to do so.

Without Cluster Mempool however the implementation is "fake", instead returning every time a transaction is added to the mempool. So for now I'm keeping this draft. It's here to provide a complete and stable Mining interface for https://github.com/bitcoin/bitcoin/pull/30437 to build on.

Unlike
...

💬 Sjors commented on pull request "Introduce waitFeesChanged() mining interface":
(https://github.com/bitcoin/bitcoin/pull/30443#issuecomment-2370412347)
(about to rebase after #30409 landed)

✅ Sjors closed a pull request: "Introduce waitFeesChanged() mining interface"
(https://github.com/bitcoin/bitcoin/pull/30443)

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772790585)
Completely agree, but i think it's out of scope for this PR to change this. This function was literally transplanted from net.cpp.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772793740)
i looked into that at thet time but but remember the implementation was ever so slightly different from specified in the RFC, so thought it was more clear to implement it here exactly according to that, instead of relying on an external function that's beholden to bitcoin's requirements. Fine with doing this, though.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772803946)
Thanks, will update

📝 Sjors opened a pull request: "Mining interface: getCoinbaseMerklePath() and submitSolution()"
(https://github.com/bitcoin/bitcoin/pull/30955)
The new `BlockTemplate` interface introduced in #30440 allows for a more efficient way for a miner to submit the block solution. Instead of having the send the full block, it only needs to provide the nonce, timestamp, version fields and coinbase transaction.

This PR introduces `submitSolution()` for that. It's currently unused.

#29432 and https://github.com/Sjors/bitcoin/pull/48 use it to process the Stratum v2 message [SubmitSolution](https://github.com/stratum-mining/sv2-spec/blob/main/
...

💬 maflcko commented on pull request "test: Add missing sync_mempools() to fill_mempool()":
(https://github.com/bitcoin/bitcoin/pull/30948#discussion_r1772883528)
Ok, I've reduced the number of syncs. However, I think it is clearer to do one sync before any eviction and one after all evictions. Otherwise, the fix seems fragile and incomplete, because there could be a tx that wasn't synced and is later evicted (for example the third transaction, if the sync happens after the second one). While this wouldn't lead to test failures right now, it seems brittle. I think all transactions should be added (and removed) on all nodes equally.

💬 TheCharlatan commented on pull request "Mining interface: getCoinbaseMerklePath() and submitSolution()":
(https://github.com/bitcoin/bitcoin/pull/30955#issuecomment-2370589147)
Concept ACK

💬 Riahiamirreza commented on pull request "rpc: show P2(W)SH redeemScript in getrawtransaction #27637":
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-2370635302)
@maflcko Would you please review the PR if it worth? If you think it does not have enough quality or is out of priority just ignore my comment.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772921229)
Maybe the other way around, replace the `CNetAddr` implementation with what you wrote here. But that sounds like a followup.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1772922894)
Agree that fixing satoshi-isms can be done in a followup.

💬 maflcko commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count in GuessVerificationProgress":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1772983265)
I wonder if it is easy to provide an accurate error message. Otherwise it could be a bit confusing for a user that had AU enabled (with 100% progress) as well as pruning, not knowing the exact cause and what they need to do to work around the error. Not sure how easy that is, so up to you.