bitcoin
OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)
This pull request contains a the same implementation of OP_CHECKTEMPLATEVERIFY (BIP119) as @jamesob's Covenant Tools, an implementation of OP_CHECKSIGFROMSTACK(VERIFY) and of OP_INTERNALKEY.
There are no testnet or mainnet activation parameters proposed in this pull request. I am deeply uninterested in the details of activation semantics.
This combination of changes allows for the implementation of a variety of layer two proposals and improvements. Including, but not limited to, Lightning Symmetry, Point-Time-Locked-Contracts, Timeout Trees, and unidirectional non-interactive channels.
For anyone interested, I also have a branch with these same changes based on the latest release.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage
For detailed information about the code coverage, see the test coverage report.
Reviews
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | michaelfolkson |
| Concept ACK | jonatack, moonsettler, hsjoberg, niteshbalusu11, 1440000bytes, chrisguida |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #29547 (kernel: chainparams updates for 27.x by fanquake)
- #29280 (Implement OP_CHECKTEMPLATEVERIFY by reardencode)
- #29270 (Implement OP_CHECKSIGFROMSTACK(VERIFY) by reardencode)
- #29269 (Add OP_INTERNALKEY for Tapscript by reardencode)
- #29247 (Reenable OP_CAT by 0xBEEFCAF3)
- #29221 (Implement 64 bit arithmetic op codes in the Script interpreter by Christewart)
- #29134 (Compressed Bitcoin Transactions by TomBriar)
- #29050 (Add OP_TXHASH and OP_CHECKTXHASHVERIFY opcodes by stevenroose)
- #29039 (versionbits refactoring by ajtowns)
- #28806 (rpc: Add script verification flags to getdeploymentinfo by ajtowns)
- #28710 (Remove the legacy wallet and BDB dependency by achow101)
- #28334 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 by ajtowns)
- #27433 (getblocktemplate improvements for segwit and sigops by Sjors)
- #26840 (refactor: importpubkey, importprivkey, importaddress, importmulti, and importdescriptors rpc by KolbyML)
- #26201 (Remove Taproot activation height by Sjors)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
This should be opened to bitcoin-inquisition rather than this repo at this stage? I thought that was the whole point of bitcoin-inquisition.
I'm interested in why you think LN-Symmetry would be better implemented not using APO but perhaps that discussion can be had elsewhere.
This should be opened to bitcoin-inquisition rather than this repo at this stage? I thought that was the whole point of bitcoin-inquisition.
Let's focus on code review. There is no strict process suggesting that code first flow through inquisition.
I'm interested in why you think LN-Symmetry would be better implemented not using APO but perhaps that discussion can be had elsewhere.
Happy to discuss on delving
In commit 0ac5cf9141441ea539d2f345d0d516d24a2f77bd
$ ./src/test/test_bitcoin -t transaction_tests
Running 7 test cases...
test/transaction_tests.cpp:192: error: in "transaction_tests/tx_valid": mapFlagNames is missing a script verification flag
Edit: 4371ffefef2 two commits later appears to contain the missing change. Can wait until next need to push for something else.
It seems too early in the process to open a pull request like this. It's also possible to open a pull request against your own repo-fork of Bitcoin Core, or Inquisition as suggested above.
For comparison, the first Taproot pull request to this repo #17977 was made in early 2020, which was after about year of in person workshops studying the proposal. The BIP's themselves were first proposed in 2018. The first work-in-progress taproot branch, not a pull request, is also from around 2018.
Then again, being able to use the full Bitcoin Core CI can help in getting the code in good shape, which in turn is useful in improving the proposal. You can run that locally too in Docker. That said, a few extra (draft) PR's for soft-fork proposals is not a significant drain on (paid for by someone) CI capacity.
Typically if a pull request depends on another pull request we mark it as draft. That's not the case here, but it does depend on two BIP proposals that were made only two days ago. We could also use some other tag to indicate that the underlying proposal is still in the concept stage.
-
A few days back Ava Chow had mentioned on twitter that there is no pull request open for CHECKTEMPLATEVERIFY:
https://x.com/achow101/status/1742427508488729044 https://x.com/achow101/status/1742555598003048753
-
There were some misunderstandings about unaddressed issues related to previous pull request:
https://x.com/JeremyRubin/status/1742582215354019956
-
Since @reardencode was already working on CHECKSIGFROMSTACK and INTERNALKEY to be used in combination with CHECKTEMPLATEVERIFY, I think it makes sense to open this pull request in bitcoin core repository for code review.
-
There have been several meetings and workshops for CHECKTEMPLATEVERIFY: https://utxos.org/workshops/. I am not sure if CHECKSIGFROMSTACK and INTERNALKEY need workshops or LNHANCE however they could be done with the pull request open as well.
-
For comparison, https://github.com/bitcoin/bitcoin/pull/29050 was opened recently and still looks in conceptual stage with unaddressed comments on BIP, incomplete implementation and still open for code review in bitcoin core repository instead of inquisition.
Then again, being able to use the full Bitcoin Core CI can help in getting the code in good shape, which in turn is useful in improving the proposal. You can run that locally too in Docker. That said, a few extra (draft) PR's for soft-fork proposals is not a significant drain on (paid for by someone) CI capacity.
Agree
A few days back Ava Chow had mentioned on twitter that there is no pull request open for CHECKTEMPLATEVERIFY:
There is a pull request open (in draft) that contains CTV in addition to other opcodes and sighash flags (ie proposed consensus changes).
You don't have to be an expert in combinatorics to realize that continuing down this path results in multiple pull requests to this repo from different authors with different combinations of opcodes and sighash flags depending on the PR author's individual preference. That was one of the major motivations for bitcoin-inquisition where multiple opcodes and sighash flags can be activated on the default signet not creating noise in this repo and allowing both review and experimentation with new opcodes and sighash flags that may or may not ever get activated on mainnet. If for some reason bitcoin-inquisition isn't to the PR author's tastes (I"m not aware of any problems so far) the same can be done on a custom signet with a different fork of this repo.
I totally agree with Sjors. The approach taken here of avoiding steps other soft fork proposals have gone through and just assuming shortcuts can be taken is a massive backwards step and I suspect won't achieve anything except creating more unnecessary noise in this repo.
Hi Sjors, thanks for your thoughts!
It seems too early in the process to open a pull request like this. It's also possible to open a pull request against your own repo-fork of Bitcoin Core, or Inquisition as suggested above.
I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
CTV had (a year of?) workshops and review with Jeremy Rubin about 2 years ago. OP_INTERNALKEY is a binary in terms of the concept - either we're OK with it or we're not, no discussion, so no reason to delay the code review. CSFS does have a few points that could be discussed (and weren't discussed years ago on the mailing list because Taproot wasn't active at the time) and I'd love to discuss those on my bips PR. Fortunately they'd be just a few lines of code and tests to change whichever direction we go on them.
Edit to add: CTV is already in inquisition, btw, and is the bulk of this PR.
For comparison, the first Taproot pull request to this repo #17977 was made in early 2020, which was after about year of in person workshops studying the proposal. The BIP's themselves were first proposed in 2018. The first work-in-progress taproot branch, not a pull request, is also from around 2018.
Taproot was a new witness program version, new script system, and new signing system. It is obviously not appropriate to follow the same path here as there.
Typically if a pull request depends on another pull request we mark it as draft. That's not the case here, but it does depend on two BIP proposals that were made only two days ago. We could also use some other tag to indicate that the underlying proposal is still in the concept stage.
I disagree that the underlying proposal is at the concept stage. CTV conceptually was settled years ago, INTERNALKEY (as I said) is a simple binary decision with no room for conceptual discussion really, and CSFS does have a few minor points that can be incorporated here easily if my decisions do not match those of the broader dev community.
I'd very much appreciate further code review here.
From the contributing guidelines
After conceptual agreement on the change, code review can be provided.
I can only speak for myself obviously but Concept NACK from me for this repo at the current time. This pull request should be opened to bitcoin-inquisition or a fork of Core with its own custom signet consensus rules.
From the contributing guidelines
After conceptual agreement on the change, code review can be provided.
I can only speak for myself obviously but Concept NACK from me for this repo at the current time. This pull request should be opened to bitcoin-inquisition or a fork of Core with its own custom signet consensus rules.
You are clearly commenting in bad faith. But in the off chance you want to pretend to be consistent, here is an other PR for you to litter with your unsolicited non-technical negativity:
https://github.com/bitcoin/bitcoin/pull/28550
@moonsettler: That PR is in draft, was opened to bitcoin-inquisition simultaneously and doesn't seem to have conceptual agreement at the current time either. Everyone opening their own pull request to this repo with their own favorite combination of opcodes and sighash flags wastes people's time. They will have to come to this pull request and tell you exactly what Sjors and I have already told you. But if you insist that is what they will have to do.
Hi @michaelfolkson, can you enumerate a concrete dispute with my proposals (on their respective PRs) or my code here? Otherwise, I'm not sure your comments are contributing to the discussion. You've registered your Concept NACK.
@reardencode: No, registering my Concept NACK is sufficient. I'll leave it to others to rehash what Sjors and I have already advised and you can choose to ignore them too.
I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
(Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
CTV had (a year of?) workshops and review with Jeremy Rubin about 2 years ago. OP_INTERNALKEY is a binary in terms of the concept - either we're OK with it or we're not, no discussion, so no reason to delay the code review. CSFS does have a few points that could be discussed (and weren't discussed years ago on the mailing list because Taproot wasn't active at the time) and I'd love to discuss those on my bips PR. Fortunately they'd be just a few lines of code and tests to change whichever direction we go on them.
Edit to add: CTV is already in inquisition, btw, and is the bulk of this PR.
Why not focus on getting OP_CTV (close to) merged first, before adding more to it?
The first PR #21702 was closed by the author. Presumably it's been worked on more over at Inquisition?
There's also a draft PR #28550 for vaults (superseeding #26857), which again includes OP_CTV. Are those exactly the same commits? If so, then why not focus on reviewing and improving (the OP_CTV part of) that PR?
I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
(Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.
As I mentioned on my post on delving it's not clear to me how one reaches consensus on changes to bitcoin's consensus rules. People have also said that they need to see well reviewed and ready code before we can begin reaching consensus, so I have made ready code and asked for review.
Why not focus on getting OP_CTV (close to) merged first, before adding more to it?
I had originally intended to do just that actually, but was given the feedback privately that many folks were not interested in reviewing any ideas for consensus changes that did not enable LN-Symmetry. So here I've made a combined PR which enables LN-Symmetry (with fewer bytes on chain than APO, it turns out).
The first PR #21702 was closed by the author. Presumably it's been worked on more over at Inquisition?
The code hasn't changed except for rebasing (and adding what turned out to be unnecessary caching in response to an accidentally flawed benchmark by @jamesob) since 2021. Getting CTV ready is not an available task.
There's also a draft PR #28550 for vaults (superseeding #26857), which again includes OP_CTV. Are those exactly the same commits? If so, then why not focus on reviewing and improving (the OP_CTV part of) that PR?
See above, the CTV part hasn't changed since 2021. That PR includes VAULT and SIGHASH_ANYPREVOUT, this PR includes CHECKSIGFROMSTACK and INTERNALKEY, and those other parts are what need attention. This set of changes is much more limited in scope than CTV+APO+VAULT - not adding deferred checks, and not modifying existing signature checking code paths.
Thanks so much for your time in responding Sjors. What path do you personally recommend for gaining consensus on changing bitcoin's consensus rules? I've heard many things from many different people and this combined proposal with ready code and BIPs is my current best attempt to move toward consensus. (Happy to continue this discussion on delving or here at your discretion)
Concept ACK
This PR seems to be the minimal feature set to get:
- LN-Symmetry,
- much better PTLCs, and
- all of the powerful UTXO-sharing protocols that CTV enables
Regarding LN-Symmetry, this update will FINALLY enable symmetric channel updates and eliminate the risk of losing all your money due to restoring an old backup. It also massively simplifies the channel state machine, makes on-chain resolution in the case of uncooperative channel closes much less painful, and makes watchtowers much more efficient. There are still some mempool-pinning problems with LN-Symmetry, but my understanding is that these can be fixed down the road with additional mempool work in bitcoind and more tx hashing modes added to CTV (or other, more expressive covenants like TXHASH). Regardless, enabling the LN-Symmetry update mechanism should renew developer interest in the Lightning Network and enable much more capital to be deployed therein.
CTV+CSFS can also substitute for APO to allow much better PTLCs as described in this gist, which should provide a massive boost in privacy and efficiency for Lightning payments.
Regarding CTV, the feature I'm most interested in is Non-Interactive Lightning Channels (which requires some kind of UTXO-sharing mechanism such as darkpools or Ark), which can make the onboarding experience for new bitcoiners as magical as it was for new bitcoin users a decade ago, without resorting to custodial wallets. Namely, it allows a new bitcoin user to, for instance, send some fiat to a vendor, then receive a lightning payment in a commitment vTXO that is attached to their public key. If the pool coordinator tries to cheat, they can still do an on-chain transaction to claim their UTXO. So basically this allows new bitcoin users to defer on-chain fees until they've done enough transactions to justify an on-chain tx, instead of having to immediately spend an on-chain fee ($10? $100??) before they've even gotten a chance to try bitcoin.
I'm unaware of anyone who thinks CTV is unsafe, and I'm unaware of anyone who would be against a fork that activates LN-Symmetry, as long as it doesn't enable drivechains like APO allegedly does. (BitVM may enable drivechains anyway, so this point may be moot).
CSFS seems to be the only vector this new featureset could potentially have unforeseen consequences, but it's been running on both Liquid and BCH for years now, so its effects should be reasonably well understood at this point.
We've worked hard getting Lightning to where it is today (and LN-Symmetry has been a long time coming!), and it's still got a long way to go, but I'm hopeful that this proposal will energize devs to continue building on Lightning and make it into the future of payments it was always meant to be.
Force pushed to deconflict opcodes with VAULT, add deployment info output, and slightly improve tx_invalid tests.
Converted this to a draft while I work through AJ's review on my similar PR to inquisition. Will port those changes over here and undraft when the time comes.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
Debug: https://github.com/bitcoin/bitcoin/runs/20473421681
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
- Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
- Is it no longer relevant? ➡️ Please close.
- Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
Is it still relevant?
yes!
Did the author lose interest or time to work on this?
opening this PR against master may not really make sense all things considered. i don't suppose anyone is under the delusion that it will be merged.
here is a branch that will be maintained for an activation client: https://github.com/lnhance/bitcoin/tree/lnhance-26.x