-
Notifications
You must be signed in to change notification settings - Fork 87
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
Incremental decommits off-chain #1223
Incremental decommits off-chain #1223
Conversation
|
||
-- | Infix version of 'difference'. | ||
(\\) :: UTxO' out -> UTxO' out -> UTxO' out | ||
a \\ b = difference a b |
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.
this is not used.
feels weird to have to define this function twice.
i would keep only the infix version and define difference
in the where clause.
wdyt?
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.
We thought infix version might come in handy. We can certainly remove it if you want.
@@ -377,6 +377,42 @@ spec = parallel $ do | |||
|
|||
waitUntil [n1] $ GetUTxOResponse testHeadId (utxoRefs [2, 42]) | |||
|
|||
it "can request decommit" $ |
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 this test is redundant as it is being covered by the below.
wdyt?
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 so too so we could remove 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 only did a surface review and did not dive deeply in the decommit logic. It overall makes sense to me but take my comments with a grain of salt.
@@ -66,7 +67,7 @@ spec = do | |||
NetworkEffect ReqSn{} -> True | |||
_ -> False | |||
) | |||
let snapshot1 = Snapshot testHeadId 1 mempty [] | |||
let snapshot1 = Snapshot testHeadId 1 mempty [] mempty |
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.
Seems like we want a smart constructor here :)
@@ -121,7 +122,7 @@ spec = do | |||
|
|||
it "sends ReqSn when leader and there are seen transactions" $ do | |||
headState <- runEvents bobEnv simpleLedger (inOpenState threeParties simpleLedger) $ do | |||
step (NetworkEvent defaultTTL alice $ ReqSn 1 []) | |||
step (NetworkEvent defaultTTL alice $ ReqSn 1 [] Nothing) |
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.
and another smart constructor there I think?
6a551d2
to
bda2115
Compare
c00dd75
to
f04183a
Compare
97b2a69
to
815c2f1
Compare
43a575f
to
eefba04
Compare
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
Script summary
Cost of Init Transaction
Cost of Commit TransactionThis is using ada-only outputs for better comparability.
Cost of CollectCom Transaction
Cost of Close Transaction
Cost of Contest Transaction
Cost of Abort TransactionSome variation because of random mixture of still initial and already committed outputs.
Cost of FanOut TransactionInvolves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.
End-To-End Benchmark ResultsThis page is intended to collect the latest end-to-end benchmarks results produced by Hydra's Continuous Integration system from the latest Please take those results with a grain of salt as they are currently produced from very limited cloud VMs and not controlled hardware. Instead of focusing on the absolute results, the emphasis should be on relative results, eg. how the timings for a scenario evolve as the code changes. Generated at 2024-01-24 12:04:18.035711122 UTC Baseline Scenario
Baseline Scenario
|
f04183a
to
4381adb
Compare
eefba04
to
1051875
Compare
4381adb
to
b873b97
Compare
ce1ce0a
to
54d84a8
Compare
Test Results421 tests 414 ✅ 17m 2s ⏱️ Results for commit 73d5576. ♻️ This comment has been updated with latest results. |
834735d
to
14d950d
Compare
b873b97
to
cc4aba5
Compare
Remove some fixmes but also add one more in HeadLogic to remind us to change the response to tx instead of UTxO.
Once we figure out exact type we want to return we will resolve FIXME in the api schema
Add HeadParameters to DecrementTx Stub out the decrementTx with appropriate params Rename maybeEmitDecommitApproved to maybeEmitDecrementTx
decrementTx is missing the head redeemer for decrement so this is moving into on-chain territory a bit. TODO: Add Decrement redeemer TODO: How to construct the correct UTxO hash Only emit DecrementTx if leader
Construct the Decrement redeemer using provided signatures and utxoHash using the UTxO from confirmed Snapshot.
We want better observability when something goes wrong
It makes sense to have one type for all possible invalid outcomes.
643c00b
to
fb69705
Compare
We don't need to keep it in DecrementTx since we already have it as part of a Snapshot
56e6d40
to
0dc0e7b
Compare
We only need to emit ReqSn if we are the leader, other events should be published
When we receive ReqDec it doesn't make sense to see DecommitAlreadyInFlight if we didn't request the decommit in the first place.
Since we hash the utxo to decommit separately we also need another argument to the verification function. Also rename Decrement redeemer field to match the others
fc080ff
to
7dba1f8
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.
I was VERY close to approve this. Only the head logic was slightly off. It's a must change as I think we introduce these changes with this PR and make it behave similar to NewTx
/ TxInvalid
behavior (+ avoid boolean blindness tech debt right away).
On ReqDec if there is a decommit tx present in state just issue RequirementFailure DecommitTxInFlight
a831ed1
to
32c8fb5
Compare
This PR introduces changes related to off-chain decommit protocol logic.
Now user has two options to request a decommit of some UTxO from the open Head:
/decommit
endpointDecommit
Decommit happens in sequence and you can't ask for a decommit while another one is in flight.
It is the user's responsibility to craft a decommit transaction which should be applicable to the local ledger state on
L2
.If the transaction applies then the
ReqDec
network effect is issued and the nextSnapshot
that is to be signed specifieswhich
UTxO
is to be decommitted from the Head.On next
AckSn
all nodes need to agree/sign the snapshot that excludes the decommitUTxO
from the ledger state.Once everybody signs the snapshot decommit tx is issued and upon observing it on
L1
it is removed from the local state and another decommit can be processed.