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

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2663965111)
> It might be worth trying to notarize the binaries (instead of only the GUI bundle). This [forum thread](https://forums.developer.apple.com/forums/thread/721117) suggests it can be done by temporarily zipping them and sending them Apple in that form.

The binaries can be notarized, but the notarization cannot be stapled, which is why I elected to not notarize them. This means that anyone who runs those binaries will be phoning home to apple, but I guess that will happen regardless of notariza
...

💬 achow101 commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2663967294)
> rfm?

I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0

🤔 zaidmstrr reviewed a pull request: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2621909024)
Hey, I'm reviewing this PR, as the description says that we can test each target separately, (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`) which is fine, but for the combined test I can't find any fuzz target there like `coinselection`.

💬 Crypt-iQ commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2664024685)
I meant to reply but guess I never did. The point about blocks-only nodes is a good one. In the happy path, they aren't sent compact blocks since they never send `SENDCMPCT` to the peer again to set the high-bandwidth mode. But they can still be sent unsolicited compact blocks and because they can't reconstruct the blocks, it lets them be fingerprinted if anything is in their mempool. In most cases I don't think we can send `GETDATA` to fetch txns from blocksonly nodes since they never `INV` us
...

💬 brunoerg commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2664030173)
> Hey, I'm reviewing this PR, as the description says that we can test each target separately, (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`) which is fine, but for the combined test I can't find any fuzz target there like `coinselection`.

I just updated the description. I removed the `coinselection` target, so there are 3 ones.

💬 mahmoudeA commented on issue "Bitcoin Core-[test] : Text overflows during the sync process.":
(https://github.com/bitcoin-core/gui/issues/847#issuecomment-2664040590)
تم اصلاح المشكلھ