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

Hash events collection [Verifiable events part 1] #734

Merged
merged 22 commits into from Jun 23, 2021

Conversation

m4ksio
Copy link
Contributor

@m4ksio m4ksio commented May 20, 2021

Adds EventsList type which captures list of events and hashes them, propagating it to the Chunk.
It's the first part of changes to add verifiable events to the system, kept here for ease of review

model/flow/identifier.go Outdated Show resolved Hide resolved
@@ -1078,7 +1078,7 @@ func (e *Engine) saveExecutionResults(
collectionID = flow.ZeroID
}

chunk := generateChunk(i, startState, endState, collectionID, blockID)
chunk := generateChunk(i, startState, endState, collectionID, blockID, result.Events[i].Hash())
Copy link
Member

@ramtinms ramtinms May 31, 2021

Choose a reason for hiding this comment

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

I don't think it is a good idea to do the result.Events[i].Hash() operation here, this is not a negligible operation and directly impact TPS slowdown. We probably should do the hashing operation as part of committer and just pass the final root hash to generateChunk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was planned as a next step, to keep this PR simple

Copy link
Member

Choose a reason for hiding this comment

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

My only concern is if this gets merge into master before the next step and make it to an spork

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can prepare PR on top of this to before merging this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

model/flow/event.go Outdated Show resolved Hide resolved
model/flow/event.go Outdated Show resolved Hide resolved
model/flow/event.go Outdated Show resolved Hide resolved
model/flow/identifier.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jun 1, 2021

Codecov Report

Merging #734 (bb271e5) into master (59672ec) will increase coverage by 0.07%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #734      +/-   ##
==========================================
+ Coverage   56.61%   56.69%   +0.07%     
==========================================
  Files         429      429              
  Lines       25472    25495      +23     
==========================================
+ Hits        14422    14455      +33     
+ Misses       9077     9062      -15     
- Partials     1973     1978       +5     
Flag Coverage Δ
unittests 56.69% <80.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
engine/execution/state/state.go 21.79% <0.00%> (-0.19%) ⬇️
fvm/handler/event.go 57.37% <ø> (ø)
engine/execution/ingestion/engine.go 54.82% <77.77%> (+3.01%) ⬆️
model/flow/event.go 73.68% <78.57%> (+29.23%) ⬆️
storage/badger/events.go 52.94% <80.00%> (+4.55%) ⬆️
engine/access/rpc/backend/backend_transactions.go 56.31% <100.00%> (ø)
engine/execution/computation/computer/computer.go 86.95% <100.00%> (+0.21%) ⬆️
engine/execution/messages.go 100.00% <100.00%> (ø)
model/convert/fixtures.go 100.00% <100.00%> (ø)
engine/verification/match/chunks.go 83.33% <0.00%> (-6.67%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59672ec...bb271e5. Read the comment docs.

# Conflicts:
#	engine/execution/computation/computer/computer.go
Copy link
Contributor

@tarakby tarakby left a comment

Choose a reason for hiding this comment

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

👌

@m4ksio m4ksio merged commit 0079a62 into master Jun 23, 2021
@m4ksio m4ksio deleted the m4ksio/5471-hash-events-collection branch June 23, 2021 19:54
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.

None yet

4 participants