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

Update mtrie with some minor fixes and small improvements #1801

Merged
merged 8 commits into from
Jan 13, 2022

Conversation

fxamacker
Copy link
Member

@fxamacker fxamacker commented Jan 4, 2022

Closes #1802

This PR updates mtrie with minor fixes and small improvements that don't break backward compatibility.

Changes include:

  • Fix returned "rest" data from utils.ReadShortData() (commit 2b84aaa)
  • Fix buffer size in flattener.EncodeStorableTrie() (commit 1fdfa14)
  • Remove unnecessary error conversion in Ledger.Set() (commit f93cb56)
  • Fix comments that didn't match code (commit 4fc66b9)
  • Improve test code (commit d1296e5)

This was done while onboarding and getting familiar with mtrie code.

"rest" should only return rest of data. It was returning data+rest.
Buffer size in flattener.EncodeStorableTrie() was 2 bytes less which can
cause additional buffer allocation.
In Ledger.Set(), PSMT.Update() returns error of type
ledger.ErrMissingKeys.

We don't need to convert error from ptrie.ErrMissingPath type because
ledger.ErrMissingKeys type is being returned.
@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2022

Codecov Report

Merging #1801 (d432c7c) into master (8fcec1e) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1801      +/-   ##
==========================================
+ Coverage   58.08%   58.10%   +0.02%     
==========================================
  Files         576      576              
  Lines       34227    34217      -10     
==========================================
+ Hits        19880    19883       +3     
+ Misses      11754    11741      -13     
  Partials     2593     2593              
Flag Coverage Δ
unittests 58.10% <100.00%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
ledger/common/encoding/encoding.go 57.05% <ø> (ø)
ledger/complete/mtrie/trie/trie.go 50.25% <ø> (ø)
ledger/complete/wal/encoding.go 61.29% <ø> (ø)
ledger/partial/ledger.go 46.51% <ø> (+8.77%) ⬆️
ledger/common/proof/proof.go 64.00% <100.00%> (ø)
ledger/complete/mtrie/flattener/encoding.go 50.00% <100.00%> (ø)
ledger/partial/ptrie/partialTrie.go 64.91% <100.00%> (ø)
consensus/hotstuff/event_loop.go 52.00% <0.00%> (-2.67%) ⬇️
admin/command_runner.go 79.20% <0.00%> (ø)
...sus/approvals/assignment_collector_statemachine.go 47.11% <0.00%> (+4.80%) ⬆️

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 8fcec1e...d432c7c. Read the comment docs.

Copy link
Member

@AlexHentschel AlexHentschel left a comment

Choose a reason for hiding this comment

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

Thanks Faye. Lots of good cleanups. 👍

ledger/common/utils/testutils.go Show resolved Hide resolved
ledger/complete/mtrie/forest_test.go Outdated Show resolved Hide resolved
ledger/complete/mtrie/forest_test.go Outdated Show resolved Hide resolved
ledger/complete/mtrie/trie/trie.go Show resolved Hide resolved
ledger/complete/mtrie/trie/trie.go Show resolved Hide resolved
@fxamacker fxamacker merged commit defb54a into master Jan 13, 2022
@fxamacker fxamacker deleted the fxamacker/tidy-mtrie branch January 13, 2022 23:35
@fxamacker fxamacker self-assigned this Jun 17, 2022
@fxamacker fxamacker added the Execution Cadence Execution Team label Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Execution Cadence Execution Team Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Execution state] Update mtrie with minor fixes and improvments
4 participants