💬 Sjors commented on pull request "Revert "[contrib] verify-commits: Add MarcoFalke fingerprint"":
(https://github.com/bitcoin/bitcoin/pull/27135#issuecomment-1438545680)
Commit signature for fab17f08e24f0db687dc25c5e10eb62293070048 looks good. I'm running a rebased version of #27058 against the proposed new trusted git root.

Calling the commit `Revert` both confusing and omits the rather critical bit about updating the trusted root.

💬 jamesob commented on pull request "OP_VAULT draft":
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1113120278)
Good point. This has been done; see the `limit_recursion` parameter. Thanks to @john-moffett for outlining a particular test case to ensure that recursion is limited to a single call.

💬 jamesob commented on pull request "OP_VAULT draft":
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1113121652)
Removed. Vault opcodes now only exist as OP_SUCCESSx overrides in tapscript.

💬 jamesob commented on pull request "OP_VAULT draft":
(https://github.com/bitcoin/bitcoin/pull/26857#discussion_r1113122540)
Fixed. OP_UNVAULT outputs (and recovery outputs) are now specified explicitly by an index on the witness stack. Thanks for this feedback.

📝 ccdle12 opened a pull request: "2023.02.21 ci prep"
(https://github.com/bitcoin/bitcoin/pull/27136)
null

✅ ccdle12 closed a pull request: "2023.02.21 ci prep"
(https://github.com/bitcoin/bitcoin/pull/27136)

👍 vasild approved a pull request: "p2p: return `CSubNet` in `LookupSubNet`"
(https://github.com/bitcoin/bitcoin/pull/26078)
ACK 9fb86661cf81e27131eb5f8d901397a25e2cd4b4

Thanks!

💬 Sjors commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1438609866)
For testing purposes, I tried rebasing the PR on master, removing @MarcoFalke's key without updating the trusted root and instead adding revsigs:

```
git log --format="%H %GK" --merges $(cat contrib/verify-commits/trusted-git-root)..master | grep -E "CE2B75697E69A548" | cut -c -40
```

It seems to ignore them though, because `contrib/verify-commits/verify-commits.py HEAD~4` fails: `No parent of 75f0e0b607cd7ff7afd56853eb34a2b285b22ad2 was signed with a trusted key!`. I checked that the fi
...

💬 MarcoFalke commented on pull request "Remove MarcoFalke fingerprint, update trusted-git-root":
(https://github.com/bitcoin/bitcoin/pull/27135#issuecomment-1438626295)
Thanks, changed title. Also happy to close this if someone wants to open a better alternative.

👍 MarcoFalke approved a pull request: "ci: A few fixes of `ccache` issues"
(https://github.com/bitcoin/bitcoin/pull/27084)
lgtm

💬 MarcoFalke commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#discussion_r1113188750)
Isn't this already the default?

> true otherwise

https://cirrus-ci.org/guide/writing-tasks/#cache-instruction

💬 MarcoFalke commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#discussion_r1113187573)
Isn't this already the default?

> By default the task name is used as a fingerprint value.

https://cirrus-ci.org/guide/writing-tasks/#cache-instruction

💬 Sjors commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#issuecomment-1438641491)
Ah wait, I'm misunderstanding what `allow-revsig-commits` does. It merely allows a signature fron a revoked key. Not a key that we removed from the trusted list, but a revoked PGP key.

So the only way to verify earlier history is to check out the trusted root commit, copy the most recent `verify-commits.py` and run it again. That way it will use the trusted keys at the time.

I suggest we merge this and then later rethink how we want to handle changing maintainers.

tACK bb86887527d817ee2
...

💬 MarcoFalke commented on pull request "lint: enable E722 do not use bare except":
(https://github.com/bitcoin/bitcoin/pull/25867#issuecomment-1438643927)
Thanks. I've read https://www.flake8rules.com/rules/E722.html and checked that the changes implement the description.

lgtm ACK 61bb4e783b3acc62b121a228f6b14c2462e23315

👍 instagibbs approved a pull request: "script: BIP341 txdata cannot be precomputed without spent outputs"
(https://github.com/bitcoin/bitcoin/pull/27122)
ACK 95f12de92505522a32ba58acd5251c69e602d160

I do wonder if it makes sense to rename `force` to something like `signing_context` or even more drastically split `Init` up into two obvious types `ConsensusInit`/`SigningInit` to make things clearer to future readers.

💬 pinheadmz commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#discussion_r1113216105)
OK, leaving this as one-liner for now

💬 pinheadmz commented on pull request "docs: add ramdisk guide for running tests on OSX":
(https://github.com/bitcoin/bitcoin/pull/27124#discussion_r1113216786)
Thanks for the review! All nits addressed throughout the entire section at ab6f73a1f65bcfd59fea07f3067312a757dba2f8

💬 MarcoFalke commented on pull request "contrib: Improve verify-commits.py to work with maintainers leaving":
(https://github.com/bitcoin/bitcoin/pull/27058#discussion_r1113217413)
Does anyone know which merge strategy this uses? I couldn't find anything at https://git-scm.com/docs/git-merge-tree

See also:

* https://git-scm.com/docs/git-merge#Documentation/git-merge.txt--sltstrategygt
* commit 291e363ce500e492475c4ccd189ea1d031c43613

👍 furszy approved a pull request: "wallet: ensure the wallet is unlocked when needed for rescanning"
(https://github.com/bitcoin/bitcoin/pull/26347)
Tested ACK 2e098439

No longer crashes due the deadlock.

💬 ajtowns commented on pull request "script: BIP341 txdata cannot be precomputed without spent outputs":
(https://github.com/bitcoin/bitcoin/pull/27122#issuecomment-1438699054)
ACK 95f12de92505522a32ba58acd5251c69e602d160

This was directly addressed in the comments earlier; should that be updated too?

> `This only works if spent_outputs was provided as well, but if it wasn't, actual validation will fail anyway.`

The immediately following comment seems wrong too, as the branch is gated by a `!scriptWitness.IsNull()` test?

> `Note that this branch may trigger for scriptPubKeys that aren't actually segwit but in that case validation will fail as SCRIPT_ERR_WIT
...