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.

Sign up for GitHub

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

Jump to bottom

Fix incentive for committee #1884

Merged
merged 11 commits into from
Sep 6, 2020
Merged

Fix incentive for committee #1884

merged 11 commits into from
Sep 6, 2020

Conversation

Tommo-L
Copy link
Contributor

@Tommo-L Tommo-L commented Aug 28, 2020

No description provided.

@Tommo-L Tommo-L mentioned this pull request Aug 28, 2020
Merged
@roman-khimov roman-khimov mentioned this pull request Aug 28, 2020
Closed
@erikzhang
Copy link
Member

neo/src/neo/Ledger/Blockchain.cs

Line 419 in 7032907

if (block.Index > 0)

Remove this if?

@Tommo-L
Copy link
Contributor Author

Tommo-L commented Aug 28, 2020

No, native contracts have not yet been deployed.

@erikzhang
Copy link
Member

What about move the incentive logic to PostPersist()?

neo/src/neo/Ledger/Blockchain.cs

Lines 469 to 474 in cb7a327

using ApplicationEngine engine = ApplicationEngine.Create(TriggerType.System, null, snapshot);
engine.LoadScript(postPersistScript);
if (engine.Execute() != VMState.HALT) throw new InvalidOperationException();
ApplicationExecuted application_executed = new ApplicationExecuted(engine);
Context.System.EventStream.Publish(application_executed);
all_application_executed.Add(application_executed);

We can remove the if condition.

@Tommo-L
Copy link
Contributor Author

Tommo-L commented Aug 28, 2020

I think it's ok. I'll move it.

@Tommo-L
Copy link
Contributor Author

Tommo-L commented Aug 28, 2020

Waiting for #1738, I still need one or two days for testing it.

shargon
shargon previously approved these changes Aug 31, 2020
View reviewed changes
erikzhang
erikzhang requested changes Aug 31, 2020
View reviewed changes
Copy link
Member

@erikzhang erikzhang left a comment

Choose a reason for hiding this comment

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

Waiting for #1738

@erikzhang
Copy link
Member

@Tommo-L #1738 has been merged.

@erikzhang
Copy link
Member

@Tommo-L Please fix the UTs.

@Tommo-L
Copy link
Contributor Author

Tommo-L commented Sep 3, 2020

Ok, I'll fix them.

shargon
shargon reviewed Sep 3, 2020
View reviewed changes
src/neo/SmartContract/Native/Tokens/NeoToken.cs
// Distribute GAS for committee

int index = (int)(engine.Snapshot.PersistingBlock.Index % (uint)ProtocolSettings.Default.CommitteeMembersCount);
var gasPerBlock = GetGasPerBlock(engine.Snapshot);
var committee = GetCommitteeMembers(engine.Snapshot).ToArray();
var pubkey = committee.OrderBy(p => p).ElementAt(index);
var pubkey = GetCommittee(engine.Snapshot)[index];
var account = Contract.CreateSignatureRedeemScript(pubkey).ToScriptHash();
GAS.Mint(engine, account, gasPerBlock * CommitteeRewardRatio / 100);
Copy link
Member

Choose a reason for hiding this comment

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

We set nextValidators before process the transactions and reward the committee after process these transactions, isn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should wait for #1907 and reward every epoch (28 blocks)?

Copy link
Member

Choose a reason for hiding this comment

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

Why #1907 is needed?

Copy link
Member

Choose a reason for hiding this comment

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

Not needed, but ig #1907 it's merged, we can reward them every 28 blocks.

shargon
shargon approved these changes Sep 4, 2020
View reviewed changes
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

we can optimize it later #1884 (comment) after #1907

@shargon shargon merged commit 4066a4f into neo-project:master Sep 6, 2020
@roman-khimov roman-khimov mentioned this pull request Sep 22, 2020
Merged
@danwbyrne danwbyrne mentioned this pull request Dec 18, 2020
Closed
45 tasks
@danwbyrne danwbyrne mentioned this pull request Dec 29, 2020
Closed
4 tasks
ShawnYun pushed a commit to ShawnYun/neo that referenced this pull request Jan 8, 2021
@Tommo-L @shargon @erikzhang
Fix incentive for committee (neo-project#1884)
3dc7126 
* fix incentive for committee

* fix

* Update Blockchain.cs

* Update NeoToken.cs

* fix ut

Co-authored-by: Tommo-L <luchuan@neo.org>
Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Erik Zhang <erik@neo.org>
cloud8little pushed a commit to cloud8little/neo that referenced this pull request Jan 24, 2021
@Tommo-L @shargon
@erikzhang @cloud8little
Fix incentive for committee (neo-project#1884)
84583c5 
* fix incentive for committee

* fix

* Update Blockchain.cs

* Update NeoToken.cs

* fix ut

Co-authored-by: Tommo-L <luchuan@neo.org>
Co-authored-by: Shargon <shargon@gmail.com>
Co-authored-by: Erik Zhang <erik@neo.org>
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Feb 2, 2021
@ixje
https://github.com/neo-project/neo/pull/1884
fe54183
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shargon shargon shargon approved these changes

@erikzhang erikzhang erikzhang approved these changes

@vncoelho vncoelho vncoelho approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Tommo-L @erikzhang @shargon @vncoelho