💬 maflcko commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3156558206)
> `local`: for development (builds from local source)

Developer would want to be able to pick different compile options. I don't think your approach of adding a hardcoded command is useful. Has any developer asked for this?

💬 KaueTech commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#issuecomment-3156572778)
> Developer would want to be able to pick different compile options. I don't think your approach of adding a hardcoded command is useful. Has any developer asked for this?



> Developer would want to be able to pick different compile options. I don't think your approach of adding a hardcoded command is useful. Has any developer asked for this?

Thanks for the feedback!

I understand that developers might want to customize build options — and this setup doesn't remove that flexibility. T
...

💬 l0rinc commented on pull request "test: remove duplicated code in test/functional/wallet_migration.py":
(https://github.com/bitcoin/bitcoin/pull/33122#issuecomment-3156583828)
code review ACK 6a7c0d3f874942a460bf5b13a107a2b21eb2e572

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

💬 pinheadmz commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2255302543)
Español here?

💬 pinheadmz commented on pull request "Add Docker support with multi-arch build and user permissions handling":
(https://github.com/bitcoin/bitcoin/pull/33139#discussion_r2255305327)
This is lannwj's key but this isn't how we "sign" releases any more.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2255308540)
Done as suggested

💬 w0xlt commented on pull request "wallet: prevent accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2255308480)
nit
```suggestion
height_delta = 65535 + (flag if flag is SEQUENCE_LOCKTIME_TYPE_FLAG else 0)
```

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2255308766)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2255309003)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2255309211)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2255310512)
Leaving as-is since we need to pass around the signature in a `std::vector` elsewhere in signing.

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