-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add OP_VAULT (BIP 345) #1421
Add OP_VAULT (BIP 345) #1421
Conversation
2ca1fd1
to
b30e37c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK! Looking forward to the merge into inquisition and prototyping with these opcodes.
bip-vaults.mediawiki
Outdated
** (This validates that the recovery has been authorized.) | ||
* else (if the recovery is allowed to be unauthorized): | ||
** If the spending transaction has more than two outputs, the script MUST fail and terminate immediately. | ||
** If the spending transaction has two outputs, and the output not the recovery output is not an ephemeral anchor, the script MUST fail and terminate immediately.<ref>'''Why can unauthorized recoveries only process a single recovery path?''' Because there is no signature required for unauthorized recoveries, if additional outputs were allowed, someone observing a recovery in the mempool would be able to rebundle and broadcast the recovery with a lower fee rate.</ref> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ephemeral anchors come up very unexpectedly here.... so this is how the fee is paid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this is it
from feedback by Gleb and Joost.
33f0ed6
to
9124f29
Compare
Instead of implicitly detecting whether or not an OP_VAULT/OP_UNVAULT spend is a recovery spend by scanning outputs for matching scriptPubKeys, explicitly indicate recoveries by requiring a witness stack element that is either -1 in the case of no recovery OR corresponds to an output index that is the recovery output.
Can this get a number assigned? |
e5f2aa7
to
0204c9a
Compare
Thanks to Vojtěch Strnad for most of this.
Since constraints on unauthorized recovery transaction structure exist only to avoid pinning, make them a matter of policy and not consensus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing section on backward compatibility
Similar BIPs, like BIP-0065, lack this section. I'm not sure what I would include in it because this just tightens validation rules around OP_SUCCESS187 and OP_SUCCESS188. |
In my book, it is perfectly fine (if it applies) to say that there are no backwards compatibility issues, especially if it is stated why that is the case (e.g. "because the op-codes in question are OP_SUCCESSes"). Not having a backwards compatibility section means the reader has to determine whether there are any, by themselves. |
Can this get a number assigned please? Is there anything blocking? |
Looking at this over the next few days. Please hold on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question: when I discussed a similar idea some years back -- https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-February/015793.html -- I was told that it was not going to work, because it essentially incentivizes the attacker to kill the victim, in order to guarantee that the victim won't "revoke" or otherwise take back their coins within the "grace" (?) period. I admit I'm honestly not sure how this proposal addresses that particular concern.
Didn't realize you had proposed something along these lines - will add to references.
I want to be clear that I think most of the benefit of using vaults comes in addressing remote attacks, where someone compromises a key remotely from live infrastructure or has backdoored some hardware that you would otherwise be relying on. I think the potential benefit for individual users is plain, and if you ask basically any industrial custodian, they would be likely to tell you of the potential value of this capability. I thought the provable time delay was novel and interesting, but I can remove that section if it muddles the use of this proposal. |
transaction relies on the use of either fully-spent fee inputs or an ephemeral | ||
anchor output. This means that vaults which do not use recovery authorization | ||
are essentially dependent on v3 transaction relay policy being deployed. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the interpretation of this that vaults using unauthorized recovery could "pre-load" themselves with varying size UTXOs to pay for fees based on the fee rate at the time of trigger (with the understanding that those UTXOs will have to be fully spent towards fees)? The last sentence makes it sound like unauthorized recovery is unusable without v3 transaction relay being deployed. A bit more clarification here would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - "fully spent fee inputs" there confirms your question.
Reporting back on costing. I've written a benchmark (bitcoin-inquisition/bitcoin@dcf1442) that simulates the relative cost of an OP_VAULT invocation vs. a successful Schnorr verification, and I was surprised to find that the taptweak check (including the necessary hashing) with a max-length control block is actually about as expensive as a good Schnorr verification; in fact, it's about 5% slower!
I did some profiling and generated a flamegraph (full interactive graph here); perhaps as expected, most time is spent on the secp256k tweak_add operation, but I was surprised that a substantial amount of time is spent on As mentioned earlier, some of the midstate can probably be cached there. But to be conservative, I've costed OP_VAULT at 60 (vs. 50 for successful Schnorr verification) (bitcoin-inquisition/bitcoin@207b33e). I've verified that all existing testcases pass, and (given requisite witness sizes) I have to bump the cost much higher to encounter any issues running out of budget during expected use. If 60 sounds like a fine cost to everyone, I'll update the BIP. And then I think we're in okay shape to merge? |
@jamesob, interesting. I spend some time digesting the numbers that you wrote. I have a small piece of rust code also produces the similar result. The performance is worse for me because I don't have AVX/sha-ni intrinsic. I am not informed enough to make this choice across different architectures with various CPU features, but at least for my somewhat old system it puts the number closer to 90/95. Given that
I also reproduced with similar results using rust-bitcoin software. https://github.com/sanket1729/rust-bitcoin/tree/vault_bench
It sounds fine to me. I would be great if one of BIP342 authors provide more input on specific number. I think this is in okay shape to merge. Excited to play with this after this is merged in inquisition and might have more feedback based on it. |
@sanket1729 for what it's worth, you can experiment with OP_VAULT pretty conveniently right now on regtest using this repo: https://github.com/jamesob/opvault-demo. I'm hoping it's very easy to set up if you've already got Docker installed on your machine. |
The 50 figure mostly just matches the ratio between MAX_BLOCK_SIGOPS_COST and MAX_BLOCK_WEIGHT. When testing, it seemed that filling a block with the various possible slow opcodes all ended up with similar performance: 3,000,000 "1000 OP_ROLL" took about 4.3s, a block full of "OP_3DUP OP_HASH256 OP_DROP OP_HASH256 OP_DROP OP_HASH256" took about 3.6s, and those were roughly the same as 80k checksigs. Obviously this varies across different hardware. |
Assuming the client caches each Not sure what number this results in exactly, but perhaps this is useful for analysis. |
14d8adc
to
014b832
Compare
I've updated the BIP to give |
e4e09b8
to
eb3fb72
Compare
Co-authored-by: Antoine Poinsot <darosior@protonmail.com>
to implement <code>OP_VAULT</code> and <code>OP_VAULT_RECOVER</code>, | ||
respectively. | ||
|
||
=== <code>OP_VAULT</code> evaluation === |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to review the discussion from bitcoin-dev 2023-03, with a slightly different opcode split.
What we currently have is:
[revault_amount revault_idx] [unvault_idx unvault_pushes unvault_n unvault_script] OP_VAULT
[idx hash] OP_VAULT_RECOVER
where OP_VAULT
does both unvaulting and revaulting, and both do scriptPubKey
checks and deferred amount checks.
What I'd like to propose for consideration is:
[idx amount] OP_REVAULT
[idx amount pushes... n script] OP_UNVAULT
[idx amount sPK] OP_VAULT_RECOVER
()OP_VAULT_FREEZE
with the behaviour that idx amount
specifies the given amount from this input is aggregated into the output at idx (via the deferred checks), and the scriptPubKey
at idx matches (respectively) the scriptPubKey being spent by this input, the scriptPubKey you get by replacing the current tapleaf with the script prefixed by n-pushes of the given n stack items, or the given sPK. If the amount is -1 that represents the remaining value from this input. If the amount is 0, or the amount is -1 but the remaining amount is 0, the operation succeeds. If the operation succeeds, 1
is left on the stack.
I think that's a slightly better split, in that it separates out the "spend/revault" operations into separate opcodes, and also make the "claim the entire remainder" vs "claim and explicit remount" logic available to each vault operation.
It's possible to go further in breaking down the logic:
- you could split out the amount verification and the scriptPubKey verification into different opcodes, but then you need to manually link the
idx
between the two, but that seems to just gets awkward for no real benefit (there's no point restricting an output amount or an output scriptPubKey on their own, after all) - you could split out the "build up a script by prefixing pushes" operation from the "verify a scriptPubKey is made up by replacing the current script with a given script"; but that seems slightly exploitable (as you could be building up a script much larger than 520 bytes)
You can reimplement OP_VAULT as:
[unvault_idx unvault_amt unvault_pushes... unvault_n unvault_script] [revault_idx revault_amount]
(input)OVER TOALT REVAULT DROP UNVAULT DROP -1 FROMALT REVAULT
(script)
I think this enables plausible new behaviour compared to the current spec; eg limiting the amount you can withdraw:
[unvault_idx unvault_amt unvault_pushes... unvault_n unvault_script] [revault_idx revault_amount]
(input)OVER TOALT REVAULT DROP
TOALT DUP 1ADD PICK DUP 0 GT VERIFY <limit> LT VERIFY FROMALT UNVAULT
DROP -1 FROMALT REVAULT
Or a "hodl until day D, but allow consolidations" script:
[sigA]
to spend afterD
, or[idx sigA]
to consolidate to outputidx
A CHECKSIGVERIFY DEPTH IF -1 REVAULT ELSE <D> CLTV ENDIF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OP_VAULT_FREEZE
I assume this is the ranamed "RECOVERY"
If the amount is -1 that represents the remaining value from this input.
Drilling down into this idea as before, it probably conceptually helps to drill down precisely what this case means.
e.g., per input script execution, the remaining value starts at total input value, and simply decrements for every OP_*VAULT
invocation, flooring at 0? I think that still allows most realistic use-cases where certain amounts are locked in at destinations, with the remaining being put somewhere else. Collateral could be done by specifying the value explicitly after whatever -1
vault operation was done.
Here was my attempt at a rework which avoids additional interpreter state, in case you hadn't seen it jamesob#4
I think functionally these are very similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OP_VAULT_FREEZE
I assume this is the ranamed "RECOVERY"
Err, yeah; I started writing that comment making more changes to the behaviour and wanted to be able to redefine VAULT_RECOVER in terms of my suggestion, so gave it a new name to make it clearer which is old and which is new. Then removed those changes, but didn't change the name back.
If the amount is -1 that represents the remaining value from this input.
Drilling down into this idea as before, it probably conceptually helps to drill down precisely what this case means.
e.g., per input script execution, the remaining value starts at total input value, and simply decrements for every
OP_*VAULT
invocation, flooring at 0? I think that still allows most realistic use-cases where certain amounts are locked in at destinations, with the remaining being put somewhere else. Collateral could be done by specifying the value explicitly after whatever-1
vault operation was done.
Yes. At start of script execution, define uint64_t vault_balance = input_amt;
. Every time a vault operation occurs:
- if the amount is
-1
, increment the expected balance for the output at idx byvault_balance
; setvault_balance=0
- otherwise, increment the expected balance for the output at idx by
amount
; setvault_balance = vault_balance - min(amount, vault_balance)
Here was my attempt at a rework which avoids additional interpreter state, in case you hadn't seen it jamesob#4
I think I'd forgotten it so reinvented it from scratch; but yeah, pretty much the same. I guess I think the -1
approach is probably better given you can't do maths in script on amounts more 21.475 BTC; so "10 BTC to A, 20 BTC to B, the rest to C" would be hard to specify, even when all the explicit amounts are representable as a 4B CScriptNum
.
I suppose one other behaviour that could be allowed is "-1 1000 OP_FALSE OP_VAULT_FREEZE" (that is idx=-1, sPK=empty, amount>0) to allow some of the funds to be spent arbitrarily, eg to miner/watchtower fees.
* Queue a deferred check<ref>'''What is a deferred check and why does this proposal require them for correct script evaluation?''' A deferred check is a validation check that is executed only after all input scripts have been validated, and is based on aggregate information collected during each input's EvalScript run.<br /><br />Currently, the validity of each input is (usually) checked concurrently across all inputs in a transaction. Because this proposal allows batching the spend of multiple vault inputs into a single recovery or withdrawal output, we need a mechanism to ensure that all expected values per output can be summed and then checked. This necessitates the introduction of an "aggregating" set of checks which can only be executed after each input's script is evaluated. Note that similar functionality would be required for batch input validation or cross-input signature aggregation.</ref> that ensures the satoshis for this input's <code>nValue</code> minus <code><revault-amount></code> are included within the output <code>nValue</code> found at <code><trigger-vout-idx></code>. | ||
* Queue a deferred check that ensures <code><revault-amount></code> satoshis, if non-zero, are included within the output's <code>nValue</code> found at <code><revault-vout-idx></code>. | ||
** These deferred checks could be characterized in terms of the pseudocode below (in ''Deferred checks'') as<br /><code>TriggerCheck(input_amount, <revault-amount>, <trigger-vout-idx>, <revault-vout-idx>)</code>. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In so far as rehashing the tapleaf path with a replaced script can be expensive, perhaps an implementation recommendation should be made to store the H_TapLeaf()
value after the first successful OP_VAULT
, and afterwards simply compare if the hashed script matches the cached value, without recalculating the merkle root and taproot tweak.
bip-0345.mediawiki
Outdated
** Note: the parity bit of the resulting taproot output is allowed to vary, so both values for the new output must be checked. | ||
* Let the output designated by <code><revault-vout-idx></code> (if the index value is non-negative) be called ''revaultOut''. | ||
* If the scriptPubKey of ''revaultOut'' is not equal to the scriptPubKey of the input being spent, script execution when spending this output MUST fail and terminate immediately. | ||
* Implemetation recommendation: if the sum of the amounts of ''triggerOut'' and ''revaultOut'' (if any) are not greater than or equal to the value of this input, script execution when spending this output SHOULD fail and terminate immediately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"implementation" has two n's in it
corresponding trigger or recovery outputs while preserving their entire input value. | ||
|
||
|
||
== Policy changes == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure the policy changes thoughts really make sense/add very much in this document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree - I think this piece of policy is an important shim in this proposal because it forces awareness of a pinning vector. If we remove this policy requirement, we can add a recommendation somewhere, but I'm not sure of any benefit or possible use of not following that policy constraint.
This section is relatively short and the associated code is too, so I think it would be kind of silly to remove it for no good reason other than BIPs apparently shouldn't talk about policy.
If there is some practical reason not to enforce this policy, I would be very curious to hear it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have suggested just having a separate BIP for the policy approach, but apparently policy is now out of scope for BIPs so... The point of separating would have been that the consensus changes could continue to work even as policy evolves over time, so locking in a description of a policy could become confusing when that policy becomes outdated.
A sample implementation is available on bitcoin-inquisition [https://github.com/jamesob/bitcoin/tree/2023-01-opvault-inq here], with an associated [https://github.com/bitcoin-inquisition/bitcoin/pull/21 pull request]. | ||
|
||
|
||
== Applications == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be better to have this as a separate BIP, separate the consensus spec from the application advice -- presumably once the consensus change is made, application devs will be more interesting in reading about the application advice than the consensus details?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've at least responded to all outstanding feedback.
bip-0345.mediawiki
Outdated
** Note: the parity bit of the resulting taproot output is allowed to vary, so both values for the new output must be checked. | ||
* Let the output designated by <code><revault-vout-idx></code> (if the index value is non-negative) be called ''revaultOut''. | ||
* If the scriptPubKey of ''revaultOut'' is not equal to the scriptPubKey of the input being spent, script execution when spending this output MUST fail and terminate immediately. | ||
* Implemetation recommendation: if the sum of the amounts of ''triggerOut'' and ''revaultOut'' (if any) are not greater than or equal to the value of this input, script execution when spending this output SHOULD fail and terminate immediately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the spelling issue and added an explanatory note to the recommendation, which hopefully makes it more easily understood.
* Queue a deferred check that ensures <code><revault-amount></code> satoshis, if non-zero, are included within the output's <code>nValue</code> found at <code><revault-vout-idx></code>. | ||
** These deferred checks could be characterized in terms of the pseudocode below (in ''Deferred checks'') as<br /><code>TriggerCheck(input_amount, <revault-amount>, <trigger-vout-idx>, <revault-vout-idx>)</code>. | ||
|
||
If none of the conditions fail, a single true value (<code>0x01</code>) is left on the stack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise to abid by cleanstack (in the common case) we'd have to append an OP_TRUE
to each script following the OP_VAULT*
invocation. I figure why not save the byte?
corresponding trigger or recovery outputs while preserving their entire input value. | ||
|
||
|
||
== Policy changes == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree - I think this piece of policy is an important shim in this proposal because it forces awareness of a pinning vector. If we remove this policy requirement, we can add a recommendation somewhere, but I'm not sure of any benefit or possible use of not following that policy constraint.
This section is relatively short and the associated code is too, so I think it would be kind of silly to remove it for no good reason other than BIPs apparently shouldn't talk about policy.
If there is some practical reason not to enforce this policy, I would be very curious to hear it.
A sample implementation is available on bitcoin-inquisition [https://github.com/jamesob/bitcoin/tree/2023-01-opvault-inq here], with an associated [https://github.com/bitcoin-inquisition/bitcoin/pull/21 pull request]. | ||
|
||
|
||
== Applications == |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If many others also feel the applications section should be separated out, I can do that. However I should note that I intentionally bundled them together to avoid, e.g. the bad usability seen in the CLTV/CSV/nLockTime BIPs, which require having three separate BIPs open to fully understand the behavior. I think there is high utility in bundling the most common use of OP_VAULT along with its specification.
transaction relies on the use of either fully-spent fee inputs or an ephemeral | ||
anchor output. This means that vaults which do not use recovery authorization | ||
are essentially dependent on v3 transaction relay policy being deployed. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct - "fully spent fee inputs" there confirms your question.
Is there anything else required for merge here? |
I presume saying "please merge this" is required, if nothing else? |
Pairs with the draft implementation in bitcoin-inquisition/bitcoin#21.