💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958439049)
In 121c8fb494df4e81566eb0706f7b9500a870ac85 "psbt: Check sighash types in SignPSBTInput and take sighash as optional": I'm a little worried that burying this in PSBT code will lead to a bug one day when a new soft fork allows e.g. a witness version 1 scriptPubKey of 40 bytes. And then instead of expanding `IsPayToTaproot()` a new helper is added and not used here.

I don't have a good suggestion though, other than maybe adding another helper like `CanSighashDefault()`. But that might be forgot
...

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958251025)
In c49cccf9aac2d4e783c633930ceca4534f05c710 "psbt: Return PSBTError from SignPSBTInput": would be useful if the commit describes this behavior change, especially since there's no test. It seems that previously `SignPSBTInput` failures were just ignored, and now they're not. But this wasn't a problem before?

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958525188)
In https://github.com/bitcoin/bitcoin/commit/293fb5d16b51cf259c9a8fe379954d1d9f303029 "psbt: Add sighash types to PSBT when not DEFAULT or ALL": this is a good place for the commit message:

```
// When an atypical sighash type is specified by the user,
// add it to the PSBT so that further signing can enforce sighash type matching.
```

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958481668)
In 121c8fb494df4e81566eb0706f7b9500a870ac85 "psbt: Check sighash types in SignPSBTInput and take sighash as optional": this makes the call to `RemoveUnnecessaryTransactions` with `*sighash_type` below dangerous.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958499227)
In 293fb5d16b51cf259c9a8fe379954d1d9f303029 "psbt: Add sighash types to PSBT when not DEFAULT or ALL" nit: is this really `else`-worthy?

This boolean juggling is really hard to read, try:

```cpp
// Allow SIGHASH_DEFAULT for non-taproot inputs, to support transactions
// that also have taproot inputs. CreateSig() will adjust it.
if (utxo.scriptPubKey.IsPayToTaproot() ? sighash != SIGHASH_DEFAULT :
(sighash != SIGHASH_DEFAULT && sighash != SIGHASH_
...

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958493444)
In https://github.com/bitcoin/bitcoin/commit/121c8fb494df4e81566eb0706f7b9500a870ac85 "psbt: Check sighash types in SignPSBTInput and take sighash as optional" nit: it would be slightly more clear if you move this comment line directly above `if (input.sighash_type && input.sighash_type != sighash)`.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958409095)
In 59186f20f24c8ab14ac34a69c61ed0c40793cea6 "wallet: change FillPSBT to take sighash as optional": I think I get it now, see comment on 293fb5d16b51cf259c9a8fe379954d1d9f303029 "psbt: Add sighash types to PSBT when not DEFAULT or ALL"

📝 yancyribbens opened a pull request: "refactor: remove redundant `for` constructs"
(https://github.com/bitcoin/bitcoin/pull/31891)
Remove redundant for loops. This refactor is a no-brainer.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958557261)
(although maybe just indenting the second line with one extra space would do the trick

📝 fanquake opened a pull request: "build: remove ENABLE_HARDENING condition from check-security"
(https://github.com/bitcoin/bitcoin/pull/31892)
This check is only used in release builds, where hardening should always be enabled. I can't think of a reason we'd want to silently skip these checks if hardening was inadvertently disabled.

🤔 jonatack reviewed a pull request: "refactor: remove redundant `for` constructs"
(https://github.com/bitcoin/bitcoin/pull/31891#pullrequestreview-2621670803)
The test code is from 7488acc64685 in https://github.com/bitcoin/bitcoin/pull/27877 (cc @murchandamus). This change alters the order of adding clone coins, which doesn't appear to matter, but maybe the author had a reason for this order.

💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2663787814)
> This change alters the order of adding clone coins, which doesn't appear to matter, but maybe the author had a reason for this order.

One of the first thing coin-grinder does is to sort the UTXO's by effective value, therefore, the order of the UTXO pool should not matter. Thanks for pointing this out and for taking a look.

💬 NicolaLS commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1958633307)
I'll go through this PR and check what I can add in a follow up PR. Regarding https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2589412550 and the discussion on Linux Kernel I came to the following conclusions:
- it does not make sense to mix runtime deps. that are either built from [depends](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md) or come from the system in the same table as required build deps.
- there should be a separate section for runtime deps. that **d
...

💬 yancyribbens commented on pull request "refactor: remove redundant `for` constructs":
(https://github.com/bitcoin/bitcoin/pull/31891#issuecomment-2663823172)
I updated the commit message adding that the given input order does not matter since coin-grinder sorts first.

💬 adamandrews1 commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#issuecomment-2663858278)
utACK 80e695e

🤔 jonatack reviewed a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2621788527)
Concept ACK

🤔 Sjors reviewed a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2621768049)
Also looked at the last two commits.

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958677916)
In https://github.com/bitcoin/bitcoin/commit/6ec298a335585ef723d113b94959377d3ff65759 "psbt: use sighash type field to determine whether to remove non-witness utxos": any non_witness_utxos? Also we don't know if it's SegWit v0 at this point?

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1958663642)
In 6ec298a335585ef723d113b94959377d3ff65759 "psbt: use sighash type field to determine whether to remove non-witness utxos":

"can only ... if not" is hard to parse, and it seems the code doing the opposite. It also seems to do the opposite of the original behavior.

```
// Only drop non_witness_utxos if sighash_type != SIGHASH_ANYONECANPAY
```

Can you clarify the reason why `SIGHASH_ANYONECANPAY` requires different treatment in the first place?

💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#issuecomment-2663900281)
@DrahtBot is drunk again? Asking me for a review directly after I give a (partial) review.