Skip to content
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

types: Move ID output of StateElement #199

Merged
merged 2 commits into from
Oct 29, 2024
Merged

types: Move ID output of StateElement #199

merged 2 commits into from
Oct 29, 2024

Conversation

lukechampine
Copy link
Member

This is an experimental change that gives each element type an ID specific to that type, rather than all elements using types.Hash256 as their ID type. That is, sce.ID is now a types.SiacoinOutputID. This prevents awkward casts in a few places, namely when deriving output IDs like types.SiafundOutputID(sfi.Parent.ID).V2ClaimOutputID()) -- we can now write sfi.Parent.ID.V2ClaimOutputID() instead.

The second commit un-embeds the StateElement field from each element type. Embedding is best avoided unless there are significant benefits, and in this case the only significant benefit was convenient access to the ID field, which is now moot.

The combined effect on JSON encodings is:

old:
{
    "id": "h:9373b2995a6f5d47d63aaf22dbbfe92aa43b84551fe5de1e08341fd02dd100ec",
    "leafIndex": 12,
    "merkleProof": [
        "h:ee261e985aefd3c1f849310dc697b3b4c5d9227b48507d7d4399adefc90708ad",
        "h:f00470b20ca5da851e86ba6793bd967eac56bb7501b1189336b31384c7652d4a"
    ]
    "siacoinOutput": {
        "address": "111a37d4aedbbf671e6ffea289e1bcb8521ab9433fd2e26bce1432696943cbf26612a970021b",
        "value": 10000000000000000000
    },
    "maturityHeight": 0
}
new:
{
    "stateElement": {
        "leafIndex": 12,
        "merkleProof": [
            "h:ee261e985aefd3c1f849310dc697b3b4c5d9227b48507d7d4399adefc90708ad",
            "h:f00470b20ca5da851e86ba6793bd967eac56bb7501b1189336b31384c7652d4a"
        ]
    },
    "id": "scoid:9373b2995a6f5d47d63aaf22dbbfe92aa43b84551fe5de1e08341fd02dd100ec",
    "siacoinOutput": {
        "address": "111a37d4aedbbf671e6ffea289e1bcb8521ab9433fd2e26bce1432696943cbf26612a970021b",
        "value": 10000000000000000000
    },
    "maturityHeight": 0
}

This definitely breaks compatibility, so we'd have to weigh whether it's worth the annoyance.

@n8maninger
Copy link
Member

n8maninger commented Oct 26, 2024

This looks like a job for generics! /s

// A StateElement is a generic element within the state accumulator.
type StateElement[T ~[32]byte] struct {
	ID          T         `json:"id"` // SiacoinOutputID, FileContractID, etc.
	LeafIndex   uint64    `json:"leafIndex"`
	MerkleProof []Hash256 `json:"merkleProof,omitempty"`
}

@lukechampine
Copy link
Member Author

does the encoding still work? Because honestly I think I'd be ok with that

@lukechampine
Copy link
Member Author

Update: the encoding does work, but after fixing up 90% of the code, I hit this...

func (au ApplyUpdate) UpdateElementProof(e *types.StateElement) {
	au.eau.updateElementProof(e)
}

Go doesn't support generic methods, so we'd have to define a top-level function (ApplyElementProof?) for this, which sucks for various reasons (e.g. you can't define a ProofUpdater interface).

After reflecting on this some more, I think the approach taken in this PR is correct: ID should not be a field of StateElement. The ID itself actually has no bearing on the state accumulator whatsoever, except for its contribution to the leaf hash -- which is parameterized by type anyway.

@lukechampine lukechampine marked this pull request as ready for review October 28, 2024 21:09
@n8maninger n8maninger merged commit 18b4d43 into master Oct 29, 2024
9 checks passed
@n8maninger n8maninger deleted the element-id branch October 29, 2024 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants