💬 dergoegge commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1378645903)
```suggestion
CMutableTransaction dup_tx;
dup_tx.vin.emplace_back(uint256{}, 0);
dup_tx.vin.emplace_back(uint256{}, 0);
Package package_with_dup_tx{MakeTransactionRef(dup_tx)};
BOOST_CHECK(IsConsistentPackage(package_with_dup_tx));
package_with_dup_tx.emplace_back(create_placeholder_tx(1, 1));
BOOST_CHECK(IsConsistentPackage(package_with_dup_tx));
```

`dup_tx` is otherwise unused and does not conflict with itself.

💬 dergoegge commented on pull request "refactors for subpackage evaluation":
(https://github.com/bitcoin/bitcoin/pull/28758#discussion_r1378634066)
This can be dropped from the header, nothing is using it externally.

💬 fanquake commented on issue "Release schedule for 26.0":
(https://github.com/bitcoin/bitcoin/issues/27758#issuecomment-1788759649)
v26.0rc1 was basically dead on arrival due to an issue with the macos codesigned binary. This has been fixed, and a number of other bugfixes were also backported. [v26.0rc2](https://github.com/bitcoin/bitcoin/releases/tag/v26.0rc2) has now been tagged.

📝 dergoegge opened a pull request: "Improve peformance of CTransaction::HasWitness (28107 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/28766)
This addresses https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1364961590 from #28107.

👍 maflcko approved a pull request: "Use serialization parameters for CTransaction"
(https://github.com/bitcoin/bitcoin/pull/28438#pullrequestreview-1707872957)
lgtm, left some style questions.


lgtm ACK e5cb04009095c21517be71c0bf21cde4ed0490a7 📿

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trus
...

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378613214)
This compiles, so I guess it is fine. Two notes:

* This forces bip152-v2-only in the serialize code. I guess it is fine, given https://github.com/bitcoin/bitcoin/pull/20799.

* question: If `TransactionCompression` was ever implemented it wouldn't support serialization without witness? So wouldn't it be cleaner to just implement the `TransactionCompression` formatter as one that calls `(Un)SerializeTransaction` with the witness params set?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378584333)
review note: I presume reviewers are supposed to set `--function-context` and verify each time that `SERIALIZE_TRANSACTION_NO_WITNESS` was never embedded into the `CDataStream` in each context.

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378615106)
style nit: Forgot to add `{}` when touching this line?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378584497)
??? What is this?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378647959)
You'll have to remove `PROTOCOL_VERSION` now. Also, `GetSerializeSize` could drop the int param completely?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378648420)
any reason for the c-style cast? Why not remove it completely?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378628768)
Cautofile -> AutoFile?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378666698)
`ConsumeDeserializable` is never called with witness deserialization disabled. I wonder if it makes sense to offer a shorter version with the param set as default?

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378633900)
Not sure if the comments adds value to someone reading the lines of code here?

⚠️ DuneCruzade opened an issue: "IOS SONOMA"
(https://github.com/bitcoin/bitcoin/issues/28767)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

The .dmg file for M1, from bitcoincore.org dowload page, is not working. Can´t install/run bitcoin core in my mac.


### Expected behaviour

would like to run bitcoin core on my mac

### Steps to reproduce

nothing happens.

the arm64 (.dmg) just dont run

### Relevant log output

none

### How did you obtain Bitcoin Core

Compiled from source

### What version of Bitcoin Core are yo
...

💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#discussion_r1378693720)
I changed to the newer streams in places where I noticed it was obviously sensible, but didn't try to be thorough about it, since being complete required extra changes like #28451. The fact that `SERIALIZE_TRANSACTION_NO_WITNESS` no longer exists means that any previous (explicit) embedding of it will have been touched by this changeset, fwiw.

💬 dergoegge commented on pull request "Improve peformance of CTransaction::HasWitness (28107 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/28766#issuecomment-1788831718)
Can someone please re-run the "CI / macOS 13 native" job, failure seems unrelated.

👍 willcl-ark approved a pull request: "wallet: Cleanup accidental encryption keys in watchonly wallets"
(https://github.com/bitcoin/bitcoin/pull/28724#pullrequestreview-1707986305)
ACK aaa6b709d164052c8b2496cbedccf8ccf48c4aa2

I left a few comments, but they're mainly thoughts I had and questions for my own understanding.

Also checked the test manually with `--legacy-wallet` and `--descriptors` and lightly inspected the dumps to ensure the records were being removed:

<details>

<summary>log</summary>

```
$ test/functional/wallet_encryption.py
2023-11-01T11:51:31.078000Z TestFramework (INFO): PRNG seed is: 196471712041506435
2023-11-01T11:51:31.07900
...

💬 willcl-ark commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1378657380)
I notice that nothing in this block requires any locks (even though `cs_wallet` will be locked at this stage in this function).

For my own understanding, is this because the db operations, i.e. writing to the on-disk file, have their synchronisation handled by the db's transaction system?

I see for example in `LoadEncryptionKey` that we lock `cs_wallet` to load the keys _into_ the wallet, which makes intuitive sense. But it feels that conversely there should be something asserting the lock
...

💬 willcl-ark commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1378684726)
Perhaps a comment to that effect would be useful here as it's not clear directly from the code?