Initialize Consensus at the correct view #779

vncoelho · 2022-12-16T20:19:13Z

close #778

src/DBFTPlugin/Consensus/ConsensusService.cs

vncoelho · 2023-03-21T12:57:02Z

@superboyiii, can you test?
@shargon, can you review.

src/DBFTPlugin/Consensus/ConsensusService.cs

vncoelho · 2023-08-30T12:06:15Z

@dotnet-policy-service agree

vncoelho · 2023-08-30T12:06:32Z

@vncoelho please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@dotnet-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@dotnet-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@dotnet-policy-service agree company="Microsoft"
Contributor License Agreement

@dotnet-policy-service agree

Jim8y · 2023-08-30T12:59:40Z

@vncoelho is this one ready for review?

vncoelho · 2023-08-30T13:05:02Z

Yes

Jim8y · 2024-02-16T01:05:42Z

@shargon how do you think about this pr? @roman-khimov can you please also take a look?

Co-authored-by: Shargon <shargon@gmail.com>

vncoelho

I committed @shargon suggestion, it makes it simpler.

github-actions · 2024-02-16T11:23:54Z

src/DBFTPlugin/Consensus/ConsensusService.cs

@@ -157,7 +157,7 @@ private void OnStart()
                    return;
                }
            }
-            InitializeConsensus(0);
+            InitializeConsensus(context.ViewNumber);


It's good that you've simplified the code by removing the unnecessary if condition. However, it would be better to add a null check for context before using context.ViewNumber to avoid potential NullReferenceException.

Thanks bot, I thought about that in previous discussions here. That why I did not applied the change before.
Perhaps we need to check if its null and then set to 0 for such case.
Can you apply that?

This bot is so polite.

Thanks bot, I thought about that in previous discussions here. That why I did not applied the change before. Perhaps we need to check if its null and then set to 0 for such case. Can you apply that?

@vncoelho

InitializeConsensus(context?.ViewNumber ?? 0);

This bot is so polite.

Well the difference is I have the solution

If it's null then highly likely we're dead already at the line 145, so I don't think it's a problem.

Thanks Cris, can you suggest the change?

Line 145 @roman-khimov is
https://github.com/neo-project/neo-modules/blob/a57e51fa85c03f3f8823bdb7701a95aa0478df5c/src/DBFTPlugin/Consensus/ConsensusService.cs#L145C1-L145C68

I do not see where context.ViewNumber was initialized in a normal use case, without recovery.
Maybe 0 is the default of the C# creation of the context class, but I am not sure.

roman-khimov · 2024-02-16T19:31:22Z

src/DBFTPlugin/Consensus/ConsensusService.cs

@@ -157,7 +157,7 @@ private void OnStart()
                    return;
                }
            }
-            InitializeConsensus(0);
+            InitializeConsensus(context.ViewNumber);


If it's null then highly likely we're dead already at the line 145, so I don't think it's a problem.

vncoelho · 2024-02-16T23:49:46Z

I am going to test, but last time (1 year ago), I thought that @cschuchardt88 suggestion InitializeConsensus(context?.ViewNumber ?? 0); could be needed.

vncoelho · 2024-02-16T23:51:57Z

If we are going to proceed, let's just make sure that ViewNumber could not assume something different here and lead to a crash. I did not yet check it completely.

My comment from Dec,2022

vncoelho · 2024-02-16T23:58:53Z

@shargon

I cannot test against current master, there is some another error in my setup

neo> FATAL [23:55:59.600] 	System.Collections.Generic.KeyNotFoundException
The given key 'Store' was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Neo.Persistence.StoreFactory.GetStore(String storageEngine, String path) in /opt/neoLib/src/Neo/Persistence/StoreFactory.cs:line 48
   at Neo.NeoSystem.LoadStore(String path) in /opt/neoLib/src/Neo/NeoSystem.cs:line 228
   at Neo.Plugins.TokensTracker.OnSystemLoaded(NeoSystem system)
   at Neo.NeoSystem..ctor(ProtocolSettings settings, IStore storage) in /opt/neoLib/src/Neo/NeoSystem.cs:line 137
   at Neo.NeoSystem..ctor(ProtocolSettings settings, String storageEngine, String storagePath) in /opt/neoLib/src/Neo/NeoSystem.cs:line 118
   at Neo.CLI.MainService.Start(CommandLineOptions options) in /opt/neoLib/src/Neo.CLI/CLI/MainService.cs:line 380
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_1(Object state)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()

Unhandled exception. System.Collections.Generic.KeyNotFoundException: The given key 'Store' was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Neo.Persistence.StoreFactory.GetStore(String storageEngine, String path) in /opt/neoLib/src/Neo/Persistence/StoreFactory.cs:line 48
   at Neo.NeoSystem.LoadStore(String path) in /opt/neoLib/src/Neo/NeoSystem.cs:line 228
   at Neo.Plugins.TokensTracker.OnSystemLoaded(NeoSystem system)
   at Neo.NeoSystem..ctor(ProtocolSettings settings, IStore storage) in /opt/neoLib/src/Neo/NeoSystem.cs:line 137
   at Neo.NeoSystem..ctor(ProtocolSettings settings, String storageEngine, String storagePath) in /opt/neoLib/src/Neo/NeoSystem.cs:line 118
   at Neo.CLI.MainService.Start(CommandLineOptions options) in /opt/neoLib/src/Neo.CLI/CLI/MainService.cs:line 380
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_1(Object state)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()
/opt/start_node.sh: line 2:   128 Aborted                 (core dumped) dotnet neo-cli.dll

vncoelho · 2024-02-17T00:11:34Z

@neo-project/core please wait until I can test the PR against current master branch, or, at least, add @cschuchardt88 suggestion.

cschuchardt88 · 2024-02-17T01:49:04Z

I am going to test, but last time (1 year ago), I thought that @cschuchardt88 suggestion InitializeConsensus(context?.ViewNumber ?? 0); could be needed.

Yes it is needed because of line 145 dbftSettings.IgnoreRecoveryLogs that it could be true.

cschuchardt88 · 2024-02-17T01:50:50Z

@shargon

I cannot test against current master, there is some another error in my setup

neo> FATAL [23:55:59.600] 	System.Collections.Generic.KeyNotFoundException
The given key 'Store' was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Neo.Persistence.StoreFactory.GetStore(String storageEngine, String path) in /opt/neoLib/src/Neo/Persistence/StoreFactory.cs:line 48
   at Neo.NeoSystem.LoadStore(String path) in /opt/neoLib/src/Neo/NeoSystem.cs:line 228
   at Neo.Plugins.TokensTracker.OnSystemLoaded(NeoSystem system)
   at Neo.NeoSystem..ctor(ProtocolSettings settings, IStore storage) in /opt/neoLib/src/Neo/NeoSystem.cs:line 137
   at Neo.NeoSystem..ctor(ProtocolSettings settings, String storageEngine, String storagePath) in /opt/neoLib/src/Neo/NeoSystem.cs:line 118
   at Neo.CLI.MainService.Start(CommandLineOptions options) in /opt/neoLib/src/Neo.CLI/CLI/MainService.cs:line 380
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_1(Object state)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()

Unhandled exception. System.Collections.Generic.KeyNotFoundException: The given key 'Store' was not present in the dictionary.
   at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
   at Neo.Persistence.StoreFactory.GetStore(String storageEngine, String path) in /opt/neoLib/src/Neo/Persistence/StoreFactory.cs:line 48
   at Neo.NeoSystem.LoadStore(String path) in /opt/neoLib/src/Neo/NeoSystem.cs:line 228
   at Neo.Plugins.TokensTracker.OnSystemLoaded(NeoSystem system)
   at Neo.NeoSystem..ctor(ProtocolSettings settings, IStore storage) in /opt/neoLib/src/Neo/NeoSystem.cs:line 137
   at Neo.NeoSystem..ctor(ProtocolSettings settings, String storageEngine, String storagePath) in /opt/neoLib/src/Neo/NeoSystem.cs:line 118
   at Neo.CLI.MainService.Start(CommandLineOptions options) in /opt/neoLib/src/Neo.CLI/CLI/MainService.cs:line 380
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_1(Object state)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()
/opt/start_node.sh: line 2:   128 Aborted                 (core dumped) dotnet neo-cli.dll

You do not have the store library for whatever store is in your config.json

Initialize Consensus at the correct view

29c2602

vncoelho commented Dec 16, 2022

View reviewed changes

src/DBFTPlugin/Consensus/ConsensusService.cs Outdated Show resolved Hide resolved

vncoelho mentioned this pull request Dec 21, 2022

Bugs on MoreThanFNodesCommittedOrLost - Logic probably not used for changeview #781

Open

vncoelho mentioned this pull request Dec 29, 2022

[dBFT CONSENSUS]: dBFT 3.0 with double speakers model #680

Closed

Merge branch 'master' into recoverCrashAtCorrectView

9e3877e

shargon reviewed Apr 13, 2023

View reviewed changes

src/DBFTPlugin/Consensus/ConsensusService.cs Outdated Show resolved Hide resolved

vncoelho and others added 2 commits May 4, 2023 20:00

Merge branch 'master' into recoverCrashAtCorrectView

611b396

Merge branch 'master' into recoverCrashAtCorrectView

291df32

Merge branch 'master' into recoverCrashAtCorrectView

f1a7dc0

Jim8y added waiting for review Need Active Pr will be closed after one week if no new activity. labels Feb 12, 2024

Merge branch 'master' into recoverCrashAtCorrectView

f99733e

Jim8y removed the Need Active Pr will be closed after one week if no new activity. label Feb 16, 2024

Update src/DBFTPlugin/Consensus/ConsensusService.cs

a57e51f

Co-authored-by: Shargon <shargon@gmail.com>

vncoelho commented Feb 16, 2024

View reviewed changes

github-actions bot reviewed Feb 16, 2024

View reviewed changes

roman-khimov approved these changes Feb 16, 2024

View reviewed changes

shargon approved these changes Feb 16, 2024

View reviewed changes

Jim8y added the blocked label Feb 17, 2024

Merge branch 'master' into recoverCrashAtCorrectView

d10753c

vncoelho merged commit 58fa510 into master Mar 12, 2024
3 of 4 checks passed

vncoelho deleted the recoverCrashAtCorrectView branch March 12, 2024 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize Consensus at the correct view #779

Initialize Consensus at the correct view #779

vncoelho commented Dec 16, 2022

vncoelho commented Mar 21, 2023

vncoelho commented Aug 30, 2023

vncoelho commented Aug 30, 2023

Jim8y commented Aug 30, 2023

vncoelho commented Aug 30, 2023

Jim8y commented Feb 16, 2024

vncoelho left a comment

github-actions bot Feb 16, 2024

vncoelho Feb 16, 2024

Jim8y Feb 16, 2024

cschuchardt88 Feb 16, 2024 •

edited

Loading

roman-khimov Feb 16, 2024

vncoelho Feb 16, 2024

vncoelho Feb 16, 2024

roman-khimov Feb 16, 2024

vncoelho commented Feb 16, 2024

vncoelho commented Feb 16, 2024

vncoelho commented Feb 16, 2024

vncoelho commented Feb 17, 2024

cschuchardt88 commented Feb 17, 2024 •

edited

Loading

cschuchardt88 commented Feb 17, 2024 •

edited

Loading

Initialize Consensus at the correct view #779

Initialize Consensus at the correct view #779

Conversation

vncoelho commented Dec 16, 2022

vncoelho commented Mar 21, 2023

vncoelho commented Aug 30, 2023

vncoelho commented Aug 30, 2023

Jim8y commented Aug 30, 2023

vncoelho commented Aug 30, 2023

Jim8y commented Feb 16, 2024

vncoelho left a comment

Choose a reason for hiding this comment

github-actions bot Feb 16, 2024

Choose a reason for hiding this comment

vncoelho Feb 16, 2024

Choose a reason for hiding this comment

Jim8y Feb 16, 2024

Choose a reason for hiding this comment

cschuchardt88 Feb 16, 2024 • edited Loading

Choose a reason for hiding this comment

roman-khimov Feb 16, 2024

Choose a reason for hiding this comment

vncoelho Feb 16, 2024

Choose a reason for hiding this comment

vncoelho Feb 16, 2024

Choose a reason for hiding this comment

roman-khimov Feb 16, 2024

Choose a reason for hiding this comment

vncoelho commented Feb 16, 2024

vncoelho commented Feb 16, 2024

vncoelho commented Feb 16, 2024

vncoelho commented Feb 17, 2024

cschuchardt88 commented Feb 17, 2024 • edited Loading

cschuchardt88 commented Feb 17, 2024 • edited Loading

cschuchardt88 Feb 16, 2024 •

edited

Loading

cschuchardt88 commented Feb 17, 2024 •

edited

Loading

cschuchardt88 commented Feb 17, 2024 •

edited

Loading