✅ 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.

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1771350856)
Just for context, the timeout inside wait_until is scaled by the timeout-factor, but the test really should measure real (wall-clock) time here, not something mocked or scaled.

I know I suggested https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1761224717, and this looks more like (2) mocktime should ideally be used. However, that requires changing real code. I'd say it would also be fine to leave this as-is for now, but up to you.

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1772993081)
```suggestion
def ensure_for(self, *, duration, f, **kwargs):
return ensure_for_helper_internal(duration=duration, predicate=f, **kwargs)
```

style-nit: Seems fine to have the default arg only in one place, since it is supposed to be the same in all places.

(Same for all other places)

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1772997396)
`attempts=` is unused and I fail to see a use-case, so it may be best to remove

🤔 tdb3 reviewed a pull request: "Mining interface: getCoinbaseMerklePath() and submitSolution()"
(https://github.com/bitcoin/bitcoin/pull/30955#pullrequestreview-2324804236)
Concept ACK