-
Notifications
You must be signed in to change notification settings - Fork 524
ledger: draft of adding UpdateCommitment header from StateDelta #6497
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
base: master
Are you sure you want to change the base?
Conversation
❌ 86 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
jannotti
left a comment
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 a lot of copying. We seem to find a reason to copy app and account state into another similar struct every time we do anything.
That's not really a strong complaint. It seems like Go pushes in that direction.
| Value []byte `codec:"v"` | ||
| Deleted bool `codec:"d"` |
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.
You don't want to use nil Value as delete? I think we do that quite a bit elsewhere.
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 wasn't sure about nil vs empty for boxes and wanted to double check but it would be fine for the other types
|
@jannotti I was trying to do no copying — just use original types with a pointer recast (except for boxes) but it does become copying once you call Add and Delete on the trie interface |
|
Out of curiosity why do we need this, I thought the digests already had txn state commitments? |
|
@pao-beep it's true that the existing transaction commitment block headers are committing to the ordered sequence of transactions some state about each txn's effects and its logs (the ApplyData and EvalDelta). An "update commitment" could use a trie to provide a key-value based commitment on the final state of all the changes in the block, so you could use the root to prove, e.g.. "acct A had asset holding X at the end of block R" or "app B had at box key Y had value Z." This would be a lot more straightforward than having to know the index position of individual transactions in a block to access or prove commitments about them. |
|
@cce I see, thanks. I guess update commitments are better and faster than state proofs |
|
@pao-beep they are complementary, not better or faster. State proofs commit to any values (transaction content via transaction commitments, other fields) in the batch of 256 rounds of block headers it is attesting. An update commitment would be a new value in the block header that would let a user follow a chain of trust down from a state proof to the corresponding block header for round R, down to an individual modified account/asset/app state at the end of round R. |
|
Thanks |
Summary
This provides a POC of implementing an
UpdateCommitmentblock header, from the changes inledgercore.StateDeltausing the encoding tags indata/basicsand a Merkle commitment implemented by the existingmerklearraypackage. (We could also implement theUpdateCommitterinterface with a Merkle Patricia Trie instead, to allow for KV based lookups and proofs.)Test Plan
Existing tests should pass, need to add new ones.