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

Enhancement/voting #130

Merged
merged 16 commits into from
Jan 15, 2018
Merged

Enhancement/voting #130

merged 16 commits into from
Jan 15, 2018

Conversation

erikzhang
Copy link
Member

No description provided.

@erikzhang erikzhang added this to the Oracle milestone Dec 28, 2017
@AshRolls AshRolls requested a review from shargon January 4, 2018 22:32
@AshRolls AshRolls assigned AshRolls and unassigned AshRolls Jan 4, 2018
@AshRolls
Copy link
Member

AshRolls commented Jan 5, 2018

Conflicts currently need resolving since the other merges to master

{
AccountState account = accounts.GetAndChange(output.ScriptHash, () => new AccountState(output.ScriptHash));
if (account.Balances.ContainsKey(output.AssetId))
account.Balances[output.AssetId] += output.Value;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe is faster use TryGetValue
With this way, you call internally get method with ContainsKey then call the get method for += and then the set method

IF([call GET]!=0) { [set][call GET]+[value] }

With TryGetValue only one get is called because don't need the +=
if[call GET to x]!=0) { [set][x]+[value] }

foreach (var group in tx.Inputs.GroupBy(p => p.PrevHash))
{
Transaction tx_prev = GetTransaction(group.Key, out int height);
foreach (CoinReference input in group)
Copy link
Member

Choose a reason for hiding this comment

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

Need check
if (tx_prev == null)

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no need to check because these transactions have been validated before.

{
foreach (ECPoint pubkey in account.Votes)
validators.GetAndChange(pubkey, () => new ValidatorState(pubkey)).Votes += output.Value;
validators_count.GetAndChange().Votes[account.Votes.Length - 1] += output.Value;
Copy link
Member

Choose a reason for hiding this comment

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

account.Votes ever have the same length than GetAndChange().Votes ?

Copy link
Member Author

Choose a reason for hiding this comment

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

The length is always equals to Blockchain.MaxValidators.

if (!validator.Registered && validator.Votes.Equals(Fixed8.Zero))
validators.Delete(pubkey);
}
validators_count.GetAndChange().Votes[account.Votes.Length - 1] -= out_prev.Value;
Copy link
Member

Choose a reason for hiding this comment

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

account.Votes ever have the same length than GetAndChange().Votes ?

HashSet<ECPoint> sv = new HashSet<ECPoint>(Blockchain.StandbyValidators);
DataCache<ECPoint, ValidatorState> validators = Blockchain.Default.GetStates<ECPoint, ValidatorState>();
foreach (ECPoint pubkey in pubkeys)
if (!sv.Contains(pubkey) && validators.TryGet(pubkey)?.Registered != true)
Copy link
Member

Choose a reason for hiding this comment

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

maybe this should be or ?
if (!sv.Contains(pubkey) || validators.TryGet(pubkey)?.Registered != true)

Copy link
Member Author

Choose a reason for hiding this comment

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

It is and.

@AshRolls
Copy link
Member

AshRolls commented Jan 8, 2018

Another merge conflict needs resolving after recent commits to master.

Fixed8 balance = account.GetBalance(GoverningToken.Hash);
foreach (ECPoint pubkey in account.Votes)
{
ValidatorState validator = validators.GetAndChange(pubkey);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a good option would be to check null before, if (validator == null)

Copy link
Member Author

Choose a reason for hiding this comment

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

It can't be null if it has been voted.

@AshRolls
Copy link
Member

AshRolls commented Jan 9, 2018

Restarted CI build that failed, it looked like just a temporary failure.

Copy link
Contributor

@localhuman localhuman left a comment

Choose a reason for hiding this comment

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

If this requires a new sync, we should make sure to offer a starter blockchain for people to download and for the nodes to start with, otherwise the nodes will take
a while to catch up.

@@ -38,7 +38,7 @@ public LevelDBBlockchain(string path)
Version version;
Slice value;
db = DB.Open(path, new Options { CreateIfMissing = true });
if (db.TryGet(ReadOptions.Default, SliceBuilder.Begin(DataEntryPrefix.SYS_Version), out value) && Version.TryParse(value.ToString(), out version) && version >= Version.Parse("1.5"))
if (db.TryGet(ReadOptions.Default, SliceBuilder.Begin(DataEntryPrefix.SYS_Version), out value) && Version.TryParse(value.ToString(), out version) && version >= Version.Parse("2.6.0"))
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this change require all clients to resync the chain?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. They can use chain.acc.

@erikzhang erikzhang merged commit 115a402 into master Jan 15, 2018
@erikzhang erikzhang deleted the enhancement/voting branch January 15, 2018 07:14
Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
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

6 participants