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

[Access] Fix slice iteration bug in TrieUpdate protobuf conversion #4593

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

peterargue
Copy link
Contributor

There was a bug in the logic that converts ledger.TrieUpdate objects to their protobuf form. The iteration did not properly handle memory reuse by the for loop, resulting the the output containing copies of the last element only.

This PR updates this logic to use the correct element, and improves tests to cover this case.

The previous tests only use a single path in the TrieUpdate, so this case was never exercised.

@codecov-commenter
Copy link

codecov-commenter commented Aug 1, 2023

Codecov Report

Merging #4593 (e74c055) into master (2528190) will decrease coverage by 1.72%.
Report is 476 commits behind head on master.
The diff coverage is 63.25%.

@@            Coverage Diff             @@
##           master    #4593      +/-   ##
==========================================
- Coverage   56.25%   54.53%   -1.72%     
==========================================
  Files         653      915     +262     
  Lines       64699    85352   +20653     
==========================================
+ Hits        36396    46550   +10154     
- Misses      25362    35219    +9857     
- Partials     2941     3583     +642     
Flag Coverage Δ
unittests 54.53% <63.25%> (-1.72%) ⬇️

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

Files Changed Coverage Δ
cmd/bootstrap/transit/cmd/utils.go 31.54% <ø> (ø)
cmd/execution_builder.go 0.00% <0.00%> (ø)
engine/access/rest/middleware/metrics.go 0.00% <0.00%> (ø)
engine/access/rest/routes/execution_result.go 56.75% <0.00%> (ø)
engine/access/rpc/backend/backend_network.go 34.11% <0.00%> (ø)
engine/access/state_stream/engine.go 0.00% <0.00%> (ø)
engine/common/rpc/convert/blocks.go 78.76% <ø> (ø)
fvm/environment/env.go 100.00% <ø> (ø)
model/flow/identifierList.go 22.36% <0.00%> (+0.84%) ⬆️
model/verification/chunkDataPackRequest.go 43.75% <0.00%> (-6.25%) ⬇️
... and 101 more

... and 239 files with indirect coverage changes

for i, path := range t.Paths {
paths[i] = path[:]
for i := range t.Paths {
paths[i] = t.Paths[i][:]
Copy link
Contributor

Choose a reason for hiding this comment

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

The two methods seem similar, and later changes to t.Paths seem to also impact paths (because array element pointers are equal) : https://go.dev/play/p/HD8XhfcFJDa
Maybe I didn't get what the bug is in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the original bug was that the slice (and thus pointer to the underlying array) were reused resulting in the grpc response including a bunch of copies of the last element.

I opted to not make a copy here since there are likely to be a large number of paths, and the underlying data structure should be immutable.

Copy link
Contributor

Choose a reason for hiding this comment

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

By updating slices to arrays, the issue and solution could be reproduced. Thank you!
https://go.dev/play/p/VMUv-aWUf8O

@peterargue peterargue merged commit 6ece1c3 into master Aug 2, 2023
33 checks passed
@peterargue peterargue deleted the petera/fix-execution-data-proto-conversion branch August 2, 2023 18:51
This pull request was closed.
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.

4 participants