Implement transaction-level absolute and input-level relative time locks #1339
Conversation
Codecov Report
@@ Coverage Diff @@
## flash-layer #1339 +/- ##
===============================================
+ Coverage 78.61% 80.23% +1.61%
===============================================
Files 115 118 +3
Lines 9870 9990 +120
===============================================
+ Hits 7759 8015 +256
+ Misses 2111 1975 -136
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
AndrejMitrovic
added
the
type-feature
AndrejMitrovic
force-pushed
the
lock-times
branch
from
ce609b2
to
df28212
Compare
AndrejMitrovic
force-pushed
the
lock-times
branch
from
df28212
to
01feb31
Compare
|
This is ready for review. Note that the lock is based on the block height, and not an absolute time. We don't yet have block timestamping. |
AndrejMitrovic
force-pushed
the
lock-times
branch
from
01feb31
to
dbc6df9
Compare
|
@hewison-chris / @linked0 could I get a review of this? |
| /// The UTXO this `Input` references must be at least `unlock_age` older | ||
| /// than the block height at which the spending transaction wants to be | ||
| /// included in the block. Use for implementing relative time locks. | ||
| public uint unlock_age = 0; |
There was a problem hiding this comment.
Oh. Stoa needs to respond to this change.
There was a problem hiding this comment.
Yes, to both changes.
| /// This transaction may only be included in a block with `height >= unlock_height`. | ||
| /// Note that another tx with a lower lock time could double-spend this tx. | ||
| /// Using `uint` instead of `Height` as it's enough to cover 80k years. | ||
| public uint unlock_height = 0; |
There was a problem hiding this comment.
Yeah, here it is. um.. uint
There is a comment above, please read it.
|
@TrustHenry the changes should be fairly simple. Just the addition of two new fields. I think it should be an easy change on the Stoa side? |
I could change it back to
The primary use-case is with hashed time-locked contracts (https://en.bitcoin.it/wiki/Hash_Time_Locked_Contracts), but this requires execution engine support. This is in another PR that I will rebase soon: #1341 |
Structural change itself is simple, but Stoa test codes are not. |
|
The word |
|
Alright, then to keep it simple I will rename it to |
|
Although |
|
|
AndrejMitrovic
force-pushed
the
lock-times
branch
from
dbc6df9
to
a6f3c3c
Compare
AndrejMitrovic
force-pushed
the
lock-times
branch
from
a6f3c3c
to
9068b5d
Compare
AndrejMitrovic
force-pushed
the
lock-times
branch
from
9068b5d
to
4f08e17
Compare
|
@bpfkorea/core I have an idea. Since there's a lot of breaking changes that the flash layer introduces (transaction time locks, Input / Output changes), what do you think about adding a new This way we can avoid breaking Stoa / Faucet until the flash layer is stabilized. I would periodically merge the changes from Then when Mathias returns we could get a final review, and merge Thoughts? |
| // it here right away and force the party to send it again at the right | ||
| // time. However, we run into a risk if we ever implement caching of | ||
| // rejecting them if they're submitted too early. | ||
| if (height < utxo_value.unlock_height + input.unlock_age) |
There was a problem hiding this comment.
I think there's not enough testing on this, but it all looks good.
There was a problem hiding this comment.
Can you explain? The unittests are not enough?
There was a problem hiding this comment.
This is a question,
are you thinking of replacing the utxo_value.unlock_height with the height_lock later?
In my Think Verification methods are confusing when it is a Freezing transaction or a melting transaction.
There was a problem hiding this comment.
This code you're commenting on does not use height_lock. This is the transaction-level lock.
The code instead uses unlock_age, which is an input-level lock. Unlock age is the number of externalized blocks between when a UTXO is first ready to be spent and the current block height.
In my Think Verification methods are confusing when it is a Freezing transaction or a melting transaction.
Yes it's a little bit complicated right now because we have this special freeze transaction.
But we cannot build the flash layer with just freeze transactions. We need transaction-level and input-level time locks too. This is issue #243.
There was a problem hiding this comment.
#243 will be part of my upcoming demo, so this will all be explained there.
The `unlock_height` field is used for a new consensus rule. Transactions may only be included in blocks whose height is >= `unlock_height`. The new field is hashed and therefore changes all UTXOs in the tests. The consensus rules are moved into the next commit to better split the diff.
It sets the `height_lock` of the transaction.
Transactions which are not unlocked yet will not be accepted by the node. In the future this may be relaxed: A node may accept a Transaction with a future unlock height into its pool, but it should never accept it into a block. Note that a transaction with a future unlock height can be double-spent before it's externalized, assuming the proper behavior of the transaction pool (see bosagora#1332). The `unlock_height` feature is primarily useful for implementing HTLCs which will be necessary to support multi-hop payment channels (Flash layer).
The `unlock_age` field of an `Input` specifies the minimum age difference between the referenced externalized UTXO height, and the height at which the spending transaction would be included in a block. This changeset changes the hashing of the `Input`, and therefore consensus rule changes were moved to the next commit to make it easier to review. Note that the Genesis block has not changed as it has no inputs, therefore hashing is unaffected.
Used to set the `unlock_age` in the `Input`.
The `unlock_age` specifies the minimum age difference between the height of the block of the spending transaction, and the unlock height of the UTXO it references. This feature is needed for our Flash layer, where transactions can attach to UTXOs only after the utxo has been externalized. Note that the `unlock_age` is compared to the `Output's` unlock height, and not its externalize height (unlock height is typically +1 of the externalize height, for pay transactions. This way an `unlock_age` of 0 and 1 have separate meanings: `unlock_age` of 0 allows the `Input` to spend the UTXO as soon as its unlock height is reached. `unlock_age` of 1 allows the `Input` to spend the UTXO when there was at least 1 block externalized after the unlock height of the UTXO being spent. Fixed bosagora#243
AndrejMitrovic
force-pushed
the
lock-times
branch
from
4f08e17
to
0608445
Compare
|
I changed the target branch to |
Good idea. |
|
Ok, there's two approvals so I'll merge this and continue with the work. Just to make it clear again, this will not require changes to Stoa / Faucet until next year. |
This will be needed for Eltoo (#1267) and also for multi-hop payment channels.
Note that the execution engine itself is not in charge of checking the block height. The engine only cares about executing opcodes in the scope of the Transaction itself and the Output it is spending - it does not care about block heights. This allows transactions to be cached - otherwise a transaction's script would have to be re-evaluated at each new block height to determine if it's valid. There are many BIPs explaining how this mechanism works, if you're curious.
Fixes #243