💬 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

👍 hebasto approved a pull request: "build: scripted-diff: drop config/ subdir for bitcoin-config.h"
(https://github.com/bitcoin/bitcoin/pull/30937#pullrequestreview-2324803577)
ACK 09b0161c4a2502564f50fa2b3ce9b19d8fc40d8b.

💬 hebasto commented on pull request "build: scripted-diff: drop config/ subdir for bitcoin-config.h":
(https://github.com/bitcoin/bitcoin/pull/30937#discussion_r1773075606)
Not directly related to this PR change, but mentioning "Makefile.am" is outdated.

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1773088147)
Wouldn't that have the same conceptual issue, but the other way around? eg locking the bitcoin implementation to RFC6887 specifics.

It didn't feel right, it's different specifications coming from a different place that somehow in a parallel evolution ended up similarly.

Now looking at `SetLegacyIPv6` i see the differences:
- there's the TORV2 case. It just marks the addess as invalid, which it would be anyway, so *that* wouldn't be problematic here.
- there's also the `NET_INTERNAL` ca
...

👍 hebasto approved a pull request: "test: Use shell builtins in run_command test case"
(https://github.com/bitcoin/bitcoin/pull/30952#pullrequestreview-2324850575)
ACK 7bd3ee62f6d6f59ca599e85f81776d282dee1539.

The new command does exactly what is described in the comment above.

Both `sh` and `echo` are expected to be available on systems where `ls` (from the original implementation) is available.