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

feature/delete_temp_siks_when_election_ends #1080

Merged
merged 15 commits into from
Sep 4, 2023

Conversation

lucasmenendez
Copy link
Contributor

Now the elections have a flag during creation (tempSIKs:boolean) which if set to true, the temporary SIKs (created using RegisterSIKTx) must be purged when the election ends.

@p4u p4u marked this pull request as ready for review August 24, 2023 13:30
Copy link
Contributor Author

@lucasmenendez lucasmenendez left a comment

Choose a reason for hiding this comment

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

review done!

@p4u p4u force-pushed the feature/delete_temp_siks_when_election_ends branch from 3e2c992 to d1b02d8 Compare August 31, 2023 21:20
lucasmenendez and others added 14 commits September 1, 2023 11:38
…ous API, splitting census proofs generation from siks proof generations. SIK related purges on election with tempSIKs moved to the top of the functions, trying to solve IST and State issue
For the temporary SIK elections, we need Arbo
to implement a Delete() mechanism.

The current Delete method removes the key, leaf and
also all the intermediary nodes that are not needed.

Signed-off-by: p4u <[email protected]>
Fix the upAfterDelete function and unify it with up().
So now also Update and Add methods are removing empty nodes.

Add a test to check the disk size after some operations.

Signed-off-by: p4u <[email protected]>
Signed-off-by: p4u <[email protected]>
Having a second implementation of the db interface is useful
for testing purposes. We can easily switch from pebble to leveldb
in order to test something in a different storage layer.

In addition add a Compact() method to the db interface.
Remove all intermediate nodes from the database once
they are not necessary anymore.

Break up the code into smaller Go files.
When a leaf of the tree is removed, if the neighbour
child is not empty, it should be moved to the upper level
where there are no other leaves.

Add tests to verify the operation.
Since we do not hold old intermediate nodes anymore,
SetRoot(), Snapshot(), Dump() and so on require to have
an existing current intermediate node (or nil to use the
current root). To this end we introduce a new method to
facilitate retrieve the list of existing roots from a level.
The main issue was that on update, if the key and value are the same,
the clean up routine for removing old intermediate nodes, was removing
the current valid intermediate nodes up to the root.

In addition adapt some tests to become compatible with the new impl.
And remove some that are not longer valid.
@p4u p4u force-pushed the feature/delete_temp_siks_when_election_ends branch from ecf9ef8 to 960d9f3 Compare September 1, 2023 10:56
@p4u
Copy link
Member

p4u commented Sep 1, 2023

Finally... everything works as expected :)

@lucasmenendez please make a new review

@p4u p4u force-pushed the feature/delete_temp_siks_when_election_ends branch from a7560bd to 503240b Compare September 1, 2023 21:21
@p4u
Copy link
Member

p4u commented Sep 1, 2023

should not be squashed, the commits are correctly ordered and with correct comments

tree/arbo/batch.go Show resolved Hide resolved
vochain/state/sik_test.go Outdated Show resolved Hide resolved
t.emptyHash = make([]byte, t.hashFunction.Len()) // empty
var err error
t.emptyNode, _, err = t.newIntermediate(t.emptyHash, t.emptyHash)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the empty node is required?

Copy link
Member

Choose a reason for hiding this comment

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

The empty node is used to compare in the up() function if both childs are empty. Precomputing and comparing is faster.

Ensure we produce the same root using both methods.

Also update the dev genesis file.
@p4u p4u force-pushed the feature/delete_temp_siks_when_election_ends branch from 503240b to 753fe1f Compare September 4, 2023 10:17
@p4u
Copy link
Member

p4u commented Sep 4, 2023

Is this ready @lucasmenendez? lets merge it!

@p4u
Copy link
Member

p4u commented Sep 4, 2023

I just created an issue with a couple of optimizations that can be done in the future: #1086

@p4u p4u merged commit e8af275 into main Sep 4, 2023
9 checks passed
@p4u p4u deleted the feature/delete_temp_siks_when_election_ends branch September 4, 2023 12: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