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 GAS distribution logic, part1 #1364

Merged
merged 8 commits into from
Sep 23, 2020
Merged

Update GAS distribution logic, part1 #1364

merged 8 commits into from
Sep 23, 2020

Conversation

fyrchik
Copy link
Contributor

@fyrchik fyrchik commented Aug 27, 2020

Related #1348 .
Implement distribution to NEO holders and validators.

pkg/core/native/native_neo.go Outdated Show resolved Hide resolved
if !n.votesChanged.Load().(bool) {
return nil
}
pubs, err := n.GetValidatorsInternal(ic.Chain, ic.DAO)
pubs, err = n.GetValidatorsInternal(ic.Chain, ic.DAO)
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to split GetValidatorsInternal to not call GetCommitteeMembers() twice (it has some cost to it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed (GetCommitteeMembers is not cached)

Copy link
Member

Choose a reason for hiding this comment

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

But I don't see any change wrt this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added committee caching.

pkg/core/native/native_neo.go Outdated Show resolved Hide resolved
@roman-khimov
Copy link
Member

Linter still isn't happy.

@codecov
Copy link

codecov bot commented Aug 31, 2020

Codecov Report

Merging #1364 into master will increase coverage by 6.56%.
The diff coverage is 76.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1364      +/-   ##
==========================================
+ Coverage   70.29%   76.85%   +6.56%     
==========================================
  Files         219      219              
  Lines       16890    17012     +122     
==========================================
+ Hits        11873    13075    +1202     
+ Misses       4295     3063    -1232     
- Partials      722      874     +152     
Impacted Files Coverage Δ
pkg/core/util.go 82.27% <ø> (+3.01%) ⬆️
pkg/core/native/native_neo.go 66.32% <71.53%> (+9.41%) ⬆️
pkg/core/blockchain.go 73.80% <72.09%> (-0.66%) ⬇️
pkg/core/native/contract.go 93.33% <81.25%> (-6.67%) ⬇️
pkg/core/native/native_gas.go 66.66% <100.00%> (+2.38%) ⬆️
pkg/core/native/native_nep5.go 73.88% <100.00%> (+3.75%) ⬆️
pkg/core/native/policy.go 70.50% <100.00%> (+0.32%) ⬆️
pkg/core/state/native_state.go 82.02% <100.00%> (+8.25%) ⬆️
pkg/crypto/keys/publickey.go 82.77% <0.00%> (-1.67%) ⬇️
pkg/rpc/server/server.go 78.34% <0.00%> (+0.62%) ⬆️
... and 23 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 1ff1cd7...c5cdaae. Read the comment docs.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

Looks good now, but I'll let it hang for a while until we finish our TPS improvements (this one may affect it).

@roman-khimov
Copy link
Member

Please, rebase it. It's outdated a little wrt C# implementation, but it's a nice starting point.

@roman-khimov
Copy link
Member

I don't see changes from neo-project/neo#1884 there (like PostPersist). Same thing with neo-project/neo#1917.

@fyrchik
Copy link
Contributor Author

fyrchik commented Sep 22, 2020

@roman-khimov changes in neo-project/neo#1917 are overriden by ones from neo-project/neo#1920 .

I wanted to do postPersist later, though nothing stops from doing it now.

Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

We still have duplicate AER problem, but let's create a separate issue for that.

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

2 participants