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

chore: indexed merkle tree emit two changelog entries at init #901

Merged

Conversation

ananas-block
Copy link
Contributor

Issue:

  • in indexed Merkle tree the changelog indices between the Merkle tree changelog and the indexed changelog are different by 1
  • this pr emits a second indexed changelog for consistency

@ananas-block ananas-block requested a review from vadorovsky as a code owner June 28, 2024 03:23
@vadorovsky
Copy link
Contributor

vadorovsky commented Jun 28, 2024

I don't get it. Why exactly is having different changelog indices an issue? Eventually they are going to differ anyway, since CMT changelog and indexed changelog have different modulus.

Is it perhaps about the changelog index not being an even number after each update? I still don't get why would it be a problem though.

@ananas-block
Copy link
Contributor Author

in the forester we need to derive both the changelog and the indexed changelog from the sequence number we get from the indexer.
On current main we need to changelog = indexer_request.root_index % 1400 indexed_changelog = indexer_request.root_index -1 % 256
The -1 feels wrong in a similar way you describe.
The reasoning was that emitting two changelogs here is consistent with concurrent Merkle tree which emits a changelog at intialization and then one at append. However, we just emit one indexed changelog in indexed Merkle tree so actually the right way in my opinion would be to remove the changelog from concurrent Merkle tree. That however makes unit tests fail hence the intuition to just make it consistent and avoid footguns in the forester which is more likely to change in the future than the indexed Merkle tree.

@vadorovsky
Copy link
Contributor

On current main we need to changelog = indexer_request.root_index % 1400 indexed_changelog = indexer_request.root_index -1 % 256 The -1 feels wrong in a similar way you describe.

I see.

That could be easily avoided if we included changelog indices in events and made Photon aware of them. But sadly, it's way too late to do that. Lesson for the future to not rely on seq for everything and to think of including more information in events.

Copy link
Contributor

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

approving with sadness 😬

@ananas-block
Copy link
Contributor Author

thanks!
agreed its ugly

@vadorovsky vadorovsky merged commit d714d9a into main Jun 28, 2024
13 checks passed
@vadorovsky vadorovsky deleted the jorrit/chore-changelog-length-consistency-indexed-merkle-tree branch June 28, 2024 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants