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

Merkle foundations with pre-commit hook #96

Merged
merged 1 commit into from
Jun 19, 2023
Merged

Conversation

Scooletz
Copy link
Contributor

@Scooletz Scooletz commented Jun 16, 2023

This PR introduces a seam for the place where Merkle construct will be created. It is done in a way, so that Verkle construct in a future should be also easy to inject into it as a similar, but separate, implementation. The implementation in the blockchain is not done yet, but it should not prevent users from using the seam.

Effectively, this allows to implement the Merkle construct as

class Merkle : IPreCommitBehavior
{
    public void BeforeCommit(ICommit commit)
    {
      // build the Merkle construct and remember to store the root under `Key.Merkle('')`
    }
}

@Scooletz Scooletz added the ethereum An Ethereum specific work item that requires a good understanding of Eth label Jun 16, 2023
@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Health
Paprika 88% 78%
Summary 88% (1602 / 1820) 78% (403 / 518)

Minimum allowed line rate is 75%

@Scooletz Scooletz marked this pull request as ready for review June 19, 2023 12:10
@@ -417,6 +424,8 @@ private void Set(in Key key, in ReadOnlySpan<byte> payload)
map.TrySet(key, payload);
}

public IKeyEnumerator GetEnumerator() => throw new NotImplementedException("Not implemented yet");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Block is responsible for keeping track of the keys used, could we face performance issues?

@@ -391,7 +396,9 @@ public void SetStorage(in Keccak key, in Keccak address, UInt256 value)
Set(Key.StorageCell(path, address), payload);
}

private void Set(in Key key, in ReadOnlySpan<byte> payload)
public bool TryGet(in Key key, out ReadOnlySpanOwner<byte> result) => throw new NotImplementedException("Not implemented yet");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have a Get method, why do we need a TryGet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a subject to debate what to do on the missing one. I assume for now that you'd like to know that it was not there 👀

Copy link
Contributor

@emlautarom1 emlautarom1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interfaces look good, I was thinking something similar to this.

@Scooletz
Copy link
Contributor Author

Merge at will @emlautarom1

@emlautarom1 emlautarom1 merged commit 8f38acd into main Jun 19, 2023
2 checks passed
@emlautarom1 emlautarom1 deleted the merkle-foundations branch June 19, 2023 14:01
@Scooletz Scooletz mentioned this pull request Jun 26, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ethereum An Ethereum specific work item that requires a good understanding of Eth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants