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

[StateService] Verification #470

Merged
merged 58 commits into from
Feb 1, 2021

Conversation

ZhangTao1596
Copy link
Collaborator

@ZhangTao1596 ZhangTao1596 commented Jan 18, 2021

Step 2/3 StateService

Since there are too many changes in StateService, I will split it into 3 parts to commit pull request, and this is the 2nd.

  • Storage and P2P message (Merged)
  • Verification
  • More command & RPC

Changes:

  • Add Verification service to get consensus of state root from verifiers.
    • Cache `MaxCachedValidationProcess· Verification processes because block grows without state root.
    • Node use latest state root to compare state.
    • Not consensused state root of height will be dropped when block grows.
  • Add RPC method to exchange consensus message.
  • Add command to start Verification service.

src/StateService/Settings.cs Outdated Show resolved Hide resolved
src/StateService/StatePlugin.cs Outdated Show resolved Hide resolved
src/StateService/StatePlugin.cs Outdated Show resolved Hide resolved
@ZhangTao1596 ZhangTao1596 changed the title [StateService] Validation [StateService] Verification Jan 20, 2021
Comment on lines 1 to 29
using Akka.Actor;
using Neo.Cryptography;
using Neo.Cryptography.ECC;
using Neo.IO;
using Neo.Ledger;
using Neo.Network.P2P;
using Neo.Network.P2P.Payloads;
using Neo.Persistence;
using Neo.Plugins.StateService.Storage;
using Neo.SmartContract;
using Neo.SmartContract.Native;
using Neo.Wallets;
using System;
using System.Collections.Generic;

namespace Neo.Plugins.StateService.Verification
{
public class VerificationContext
{
private const uint MaxValidUntilBlockIncrement = 100;
private StateRoot root;
private ExtensiblePayload payload;
private readonly Wallet wallet;
private readonly KeyPair keyPair;
private readonly int myIndex;
private readonly uint rootIndex;
private readonly ECPoint[] verifiers;
private int M => verifiers.Length - (verifiers.Length - 1) / 3;
private readonly Dictionary<int, byte[]> signatures = new Dictionary<int, byte[]>();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
using Akka.Actor;
using Neo.Cryptography;
using Neo.Cryptography.ECC;
using Neo.IO;
using Neo.Ledger;
using Neo.Network.P2P;
using Neo.Network.P2P.Payloads;
using Neo.Persistence;
using Neo.Plugins.StateService.Storage;
using Neo.SmartContract;
using Neo.SmartContract.Native;
using Neo.Wallets;
using System;
using System.Collections.Generic;
namespace Neo.Plugins.StateService.Verification
{
public class VerificationContext
{
private const uint MaxValidUntilBlockIncrement = 100;
private StateRoot root;
private ExtensiblePayload payload;
private readonly Wallet wallet;
private readonly KeyPair keyPair;
private readonly int myIndex;
private readonly uint rootIndex;
private readonly ECPoint[] verifiers;
private int M => verifiers.Length - (verifiers.Length - 1) / 3;
private readonly Dictionary<int, byte[]> signatures = new Dictionary<int, byte[]>();
using Akka.Actor;
using Neo.Cryptography;
using Neo.Cryptography.ECC;
using Neo.IO;
using Neo.Ledger;
using Neo.Network.P2P;
using Neo.Network.P2P.Payloads;
using Neo.Persistence;
using Neo.Plugins.StateService.Storage;
using Neo.SmartContract;
using Neo.SmartContract.Native;
using Neo.Wallets;
using System;
using System.Collections.Concurrent;
namespace Neo.Plugins.StateService.Verification
{
public class VerificationContext
{
private const uint MaxValidUntilBlockIncrement = 100;
private StateRoot root;
private ExtensiblePayload payload;
private readonly Wallet wallet;
private readonly KeyPair keyPair;
private readonly int myIndex;
private readonly uint rootIndex;
private readonly ECPoint[] verifiers;
private int M => verifiers.Length - (verifiers.Length - 1) / 3;
private readonly ConcurrentDictionary<int, byte[]> signatures = new ConcurrentDictionary<int, byte[]>();

Use concurrent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why use concurrent, it is only in one actor.

Copy link
Member

Choose a reason for hiding this comment

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

VerificationService use Concurrent for his dictionary and it use this class

Copy link
Collaborator Author

@ZhangTao1596 ZhangTao1596 Jan 31, 2021

Choose a reason for hiding this comment

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

I don't think we need concurrent in VerificationService, how do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 94 to 95
signatures.Add(index, sig);
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
signatures.Add(index, sig);
return true;
return signatures.TryAdd(index, sig);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

src/StateService/Verification/VerificationContext.cs Outdated Show resolved Hide resolved
@superboyiii
Copy link
Member

@erikzhang @shargon Please review again.

@ZhangTao1596
Copy link
Collaborator Author

Merge?

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

There is something strange,
Once I activate this plugin the screen do not allow to be stopped on linux server with ctrl+c.
Multiple nodes are running together and the ones with this plugin activated suffers this.

@ZhangTao1596
Copy link
Collaborator Author

ZhangTao1596 commented Feb 1, 2021

There is something strange,
Once I activate this plugin the screen do not allow to be stopped on linux server with ctrl+c.
Multiple nodes are running together and the ones with this plugin activated suffers this.

Did you install oracle plugin? As @ProDog tested #494, orcale plugin will cause this can't exit problem.

{
if (MaxCachedVerificationProcessCount <= contexts.Count)
{
var indexes = contexts.Keys.OrderBy(i => i).ToArray();
Copy link
Member

Choose a reason for hiding this comment

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

Use list?

Copy link
Member

Choose a reason for hiding this comment

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

I think ReadOnlySpan<T> is better.

Copy link
Collaborator Author

@ZhangTao1596 ZhangTao1596 Feb 1, 2021

Choose a reason for hiding this comment

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

List done.

Copy link
Collaborator Author

@ZhangTao1596 ZhangTao1596 Feb 1, 2021

Choose a reason for hiding this comment

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

I think ReadOnlySpan<T> is better.

Use this var indexes = new ReadOnlySpan<uint>(contexts.Keys.OrderBy(i => i).ToArray());?

Copy link
Member

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

            if (MaxCachedVerificationProcessCount <= contexts.Count)
            {
                contexts.Keys.OrderBy(p => p).Take(contexts.Count - MaxCachedVerificationProcessCount + 1).ForEach(p =>
                {
                    if (contexts.TryRemove(p, out var value))
                    {
                        value.Timer.CancelIfNotNull();
                    }
                });
            }

@erikzhang How about this?

Copy link
Member

Choose a reason for hiding this comment

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

Good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

shargon
shargon previously approved these changes Feb 1, 2021
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.

I think that the code it's ok

@vncoelho
Copy link
Member

vncoelho commented Feb 1, 2021

hi @ZhangTao1596, you are correct, I forgot to delete the OraclePlugin just for those clients, makes sense.
I did not test without it, but the analysis is precise.

{
Timeout = TimeSpan.FromMilliseconds(TimeoutMilliseconds),
};
var client = new RpcClient(http_client, url);
Copy link
Member

Choose a reason for hiding this comment

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

We should use ExtensiblePayload with p2p, but it could be done in a different PR

Copy link
Collaborator Author

@ZhangTao1596 ZhangTao1596 Feb 1, 2021

Choose a reason for hiding this comment

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

Yes. Using p2p, We won't need url and avoid inconsistence between urls and public keys.
I have code here https:/ZhangTao1596/neo-modules/tree/state-service-p2p.

@Tommo-L worries that use p2p will put more pressure on p2p. And he proposes add service discovery function.
Once we make agreement, I will create another pr.

Copy link
Member

Choose a reason for hiding this comment

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

If we use service discovery, will it expose the IP address?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can we merge this for now. I will make p2p improvement pr later.

Copy link
Contributor

Choose a reason for hiding this comment

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

use p2p will put more pressure on p2p.

Yes

will it expose the IP address?

Maybe we can encrypt part of the service information so that only the relevant node can decrypt it, like:

service type + service information

state: xxxxxxx 
oracle: yyyyyyy
rpc: 192.168.1.1
...

We can consider it later, after this pr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Tommo-L Can you create a issue of your idea. We can discuss there. Since oracle and state both need that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oracle can't use p2p message for siging!

Copy link
Contributor

Choose a reason for hiding this comment

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

We can discuss here #493

@ProDog
Copy link
Contributor

ProDog commented Feb 1, 2021

Merge?

@erikzhang erikzhang merged commit 1aac793 into neo-project:master Feb 1, 2021
@ZhangTao1596 ZhangTao1596 deleted the state-service-validation branch February 1, 2021 14:56
joeqian10 pushed a commit to joeqian10/neo-modules that referenced this pull request Apr 7, 2021
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.

8 participants