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

💬 1440000bytes commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#issuecomment-1438807616)
> Most users won't know what "signet" means, and it isn't self-explanatory like "testnet"...

Most users wont use it and it just helps devs and power users on windows. They know what signet means. Everyone else clicking on it is same as downloading a malware instead of bitcoin core which is not something this repo needs to solve. First match in search is always "bitcoin core (mainnet)" and even that would open other network if bitcoin.conf has other network set.

So this NACK makes no sense
...

📝 roconnor-blockstream opened a pull request: "Raise PRNG seed log to INFO."
(https://github.com/bitcoin/bitcoin/pull/27137)
Some build infrastructure, such as Nix, will delete failed builds by default, keeping only the log of the failed build.

For flaky tests, it would be very helpful to have the PRNG seed in the default log in order to redo the failed test.

By simply raising the PRNG seed logging to INFO, we can, by default, record the seed in the log of every build.