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

Create consensus plugin #453

Merged
merged 12 commits into from
Jan 14, 2021
Merged

Create consensus plugin #453

merged 12 commits into from
Jan 14, 2021

Conversation

erikzhang
Copy link
Member

No description provided.

@erikzhang
Copy link
Member Author

Wait for neo-project/neo#2212

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.

Looks good, I will test tomorrow.

@erikzhang
Copy link
Member Author

Wait for neo-project/neo#2213

@erikzhang
Copy link
Member Author

Wait for #454

@superboyiii superboyiii mentioned this pull request Jan 11, 2021
36 tasks
@chenzhitong
Copy link
Member

image
Failed to compile (I've updated Neo in NuGet to the latest)

@cloud8little
Copy link
Contributor

how to use it to start consensus?

@erikzhang
Copy link
Member Author

Plugin.GetService<IConsensusProvider>()?.Start(wallet);

Use this code in cli.

Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Neo.Plugins.StateService.Tests", "tests\Neo.Plugins.StateService.Tests\Neo.Plugins.StateService.Tests.csproj", "{149822EC-4E0C-425F-A032-4196B615BFEB}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Neo.Plugins.StateService.Tests", "tests\Neo.Plugins.StateService.Tests\Neo.Plugins.StateService.Tests.csproj", "{149822EC-4E0C-425F-A032-4196B615BFEB}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "dBFT", "src\dBFT\dBFT.csproj", "{90185D3E-4813-4BC1-98FE-26FD34311403}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "dBFT", "src\dBFT\dBFT.csproj", "{90185D3E-4813-4BC1-98FE-26FD34311403}"
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "dBFT2.0", "src\dBFT\dBFT.csproj", "{90185D3E-4813-4BC1-98FE-26FD34311403}"

?

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 project name shouldn't include version number.

{
"PluginConfiguration": {
"RecoveryLogs": "ConsensusState",
"IgnoreRecoveryLogs": false
Copy link
Member

Choose a reason for hiding this comment

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

"StartConsensus": 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.

But how do you pass the wallet?

Copy link
Member

Choose a reason for hiding this comment

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

An encapsulated wallet such as RPCServer DummyWallet?

Copy link
Member

@vncoelho vncoelho Jan 11, 2021

Choose a reason for hiding this comment

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

But perhaps we will need to pass the path and password duplicated here...

Copy link
Member

Choose a reason for hiding this comment

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

What are you suggesting, @erikzhang ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can register the wallet as a service. But maybe we should do it in another pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

will we control the start consensus in the plugin config instead of neo-cli config?

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 so, @cloud8little, control it on neo-cli config will be a little bit confusing in my opinion.
It looks like we need to do this extra effort here to create the wallet as a service as Erik mentioned.

Copy link
Member

@vncoelho vncoelho Jan 13, 2021

Choose a reason for hiding this comment

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

@erikzhang,
another way to do could be to create a function here (called GetStartConsensus()) that return true or false (getting there value from the dBFT config file here..

On the neo-node we check if this plugin exist and call this function.
If true calls

cs IConsensusProvider consensus = Plugin.Plugins.OfType<IConsensusProvider>().FirstOrDefault(); consensus?.Start(wallet);

@ProDog
Copy link
Contributor

ProDog commented Jan 12, 2021

Plugin.GetService<IConsensusProvider>()?.Start(wallet);

Use this code in cli.

Plugin.GetService<T>() is protected.

@vncoelho
Copy link
Member

I tried without success here as well,@ProDog....aheuahea
Perhaps creating the wallet service for opening here will be a good direction to already merge this PR.

@superboyiii
Copy link
Member

Plugin.GetService<IConsensusProvider>()?.Start(wallet);

Use this code in cli.

protectedstatic T GetService<T>() -> protected method can't be used in cli straightly.

@erikzhang
Copy link
Member Author

IConsensusProvider consensus = Plugin.Plugins.OfType<IConsensusProvider>().FirstOrDefault();
consensus?.Start(wallet);

Use this code.

@superboyiii
Copy link
Member

superboyiii commented Jan 12, 2021

I tried it on my local. No error, no consensus, no log.
image
image

src/dBFT/DBFTPlugin.cs Outdated Show resolved Hide resolved
@superboyiii
Copy link
Member

superboyiii commented Jan 12, 2021

It could work now but I will make deeper test on this.
image

@vncoelho
Copy link
Member

vncoelho commented Jan 12, 2021

Working.

image

vncoelho
vncoelho previously approved these changes Jan 12, 2021
@superboyiii
Copy link
Member

superboyiii commented Jan 13, 2021

Test: PASS
I've tried single-cn and multi-cn, on local and online, within straight connection and seed connection, send some txs and make recovery scenario. All these works well.

So, we'll create the start consensus command in another PR, right?

shargon
shargon previously approved these changes Jan 13, 2021
@erikzhang
Copy link
Member Author

Ready to review.

{
walletProvider = GetService<IWalletProvider>();
if (Settings.Default.AutoStart)
walletProvider.WalletOpened += WalletProvider_WalletOpened;
Copy link
Member

Choose a reason for hiding this comment

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

What is this syntax?
WalletProvider_WalletOpened is a function?

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.

@@ -1,6 +1,7 @@
{
"PluginConfiguration": {
"RecoveryLogs": "ConsensusState",
"IgnoreRecoveryLogs": false
"IgnoreRecoveryLogs": false,
"AutoStart": false
Copy link
Member

Choose a reason for hiding this comment

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

Why not True as default? Who use the plugin usually will want it true.

walletProvider.WalletOpened += WalletProvider_WalletOpened;
}

private void WalletProvider_WalletOpened(object sender, Wallet wallet)
Copy link
Member

Choose a reason for hiding this comment

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

What will be the message is wallet closed?

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 will pass the wallet with null value.

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.

Tested and worked as expected.

vncoelho
vncoelho previously approved these changes Jan 13, 2021
@shargon
Copy link
Member

shargon commented Jan 13, 2021

Give me one day more for review

@chenzhitong

This comment has been minimized.

@superboyiii
Copy link
Member

Works well on mine.

@superboyiii
Copy link
Member

superboyiii commented Jan 14, 2021

Works well on mine. Wait for @shargon 's review.
image

@erikzhang erikzhang merged commit 2942a29 into master Jan 14, 2021
@erikzhang erikzhang deleted the dBFT branch January 14, 2021 09:15
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