💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2255371157)
> There's also 3 other network types as well as zmq and tor ports.

I’m new to the Bitcoin codebase and these are the ports I know so far:

```yaml
- "8332:8332" # RPC
- "8333:8333" # P2P
```

Could you please suggest which other network ports I should expose in the yaml?

💬 achow101 commented on pull request "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts":
(https://github.com/bitcoin/bitcoin/pull/32473#issuecomment-3156698357)
I would prefer having the cache always active as I find that much easier to reason about.

💬 Crypt-iQ commented on pull request "validation: detect witness stripping without re-running Script checks":
(https://github.com/bitcoin/bitcoin/pull/33105#issuecomment-3156720697)
Replying out of thread to not derail the thread conversation.

> This would make sure we don't weaken any existing standardness or consensus error into a witness-stripped one.

In current master, these are the consensus errors (I think?) that can be returned as witness-stripped:
- SCRIPT_ERR_WITNESS_MALLEATED (changes txid)
- SCRIPT_ERR_WITNESS_MALLEATED_P2SH (changes txid)
- SCRIPT_ERR_WITNESS_PROGRAM_MISMATCH (for P2WPKH, not the case for P2WSH)
- SCRIPT_ERR_WITNESS_PROGRAM_WRONG_LENGT
...

🤔 jonatack reviewed a pull request: "test: remove duplicated code in test/functional/wallet_migration.py"
(https://github.com/bitcoin/bitcoin/pull/33122#pullrequestreview-3089819740)
ACK 6a7c0d3f874942a460bf5b13a107a2b21eb2e572

💬 jonatack commented on pull request "test: remove duplicated code in test/functional/wallet_migration.py":
(https://github.com/bitcoin/bitcoin/pull/33122#discussion_r2255413516)
(nit, only if you have to retouch) generally I believe we've been preferring to use f-strings for new python code

```suggestion
self.log.info(f"Test migrating a wallet with the following path/name: {wallet_path}")
```

💬 w0xlt commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2255424371)
nit
```suggestion
uint32_t m_version{CTransaction::CURRENT_VERSION};
```

💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2255658263)
I think this is fine here since it is in the deploy output folder. It should really never be giving out more than a single zip. It's safe to assume here that the deploy folder will just have the one

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#issuecomment-3157187483)
Addressed or responded to all concerns, thanks for spending the time to review this.
I've extended the fuzzer to use the whole range of possible opcodes, I'm curious to see if it reveals any possible failures.
The consist of tiny, simple changes, other reviewers are welcome.

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255448800)
There was no reordering, it was just inserted after the other helper, `IsPayToWitnessScriptHash` (looks like GitHub got confused).
The `BOOST_CHECK_EQUAL` to `BOOST_CHECK` change is just to test the new helper method in a simpler way, now that we don't have to use `Solver`.
You still think it needs additional explanations?

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255352640)
Done, thanks, kept the rename and added you as a co-author

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255358039)
Added your explanation to the commit message - even though the changes themselves made that obvious, since we've added new comments to the methods

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255644392)
I played with this, but really dislike the new version, mostly for how awkward `CheckSigopsBIP54` has become after it.
But I have kept your comment update and change the ternary, let me know what you think of this instead:
```C++
if (prev_txo.scriptPubKey.IsPayToScriptHash()) {
Assert(prev_txo.scriptPubKey.CountSigOps(/*fAccurate=*/true) == 0);
sigops += CountP2SHSigOps(txin.scriptSig);
} else {
sigops += prev_txo.scriptPubKey.CountSigOps(/*fAccurate=*/true);
}
```

If we
...

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255456847)
In most other cases I used constants, but for the sizes I prefer redundancy and clarity, to make absolutely sure we're not accidentally changing anything here

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255457272)
I prefer the constant here for redundancy

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255462973)
Yes, same reasoning as before - normally I'm all for deduplication, so that the reason is clear + if we change it, we don't have to touch multiple places. But here I want a bit of extra redundancy to make sure that we **do** have to change in multiple places if we **really** want to change this value.

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255465558)
We kinda' depend on pubkey here - if you have a more concrete suggestion, please let me know, all other ones I thought of were uglier than this.

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255459207)
Yes, good point, already did that as part of the other similar comment you had.

💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2255500828)
Even better, I used `CKey dummyKey{GenerateRandomKey(/*compressed=*/true)}`

💬 l0rinc commented on pull request "index: remove unnecessary locater cleaning in BaseIndex::Init()":
(https://github.com/bitcoin/bitcoin/pull/32882#issuecomment-3157260359)
ACK c83741e4033824b4c798fc3e9cdc33cb30bbaf32

please fix the typo in the PR subject: locater -> locator

💬 pablomartin4btc commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2255780830)
nit:
```suggestion
"Creates a wallet file at the specified destination containing a watchonly version "
"of the current wallet. This watchonly wallet contains the wallet's public descriptors, "
"its transactions, and address book data. The watchonly wallet can be imported into "
"another node using 'restorewallet'.",
```