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

Use ExtensiblePayload in consensus #2202

Merged
merged 9 commits into from
Jan 10, 2021
Merged

Use ExtensiblePayload in consensus #2202

merged 9 commits into from
Jan 10, 2021

Conversation

erikzhang
Copy link
Member

No description provided.

@erikzhang
Copy link
Member Author

Can someone fix the UT? I can't mock the consensus nodes in Blockchain.extensibleWitnessWhiteList.

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.

I need to understand this concept of ExtensiblePayload.
I can try to fix, but may need some couple of days @erikzhang,3-5.

If someone can fix before it I can double check.

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.

As far as I could investigate, @erikzhang, there is a problem with the RecoveryMessage.

Basically, I commented all tests...aehauheaue
But kept ConsensusService_SingleNodeActors_OnStart_PrepReq_PrepResponses_Commits, which checks a lot of things about the consensus flow.

There is something going on that CommitPayload is not being recovered.

@vncoelho
Copy link
Member

vncoelho commented Jan 6, 2021

I am sending the commit here and will keep uncommenting as soon as we advance.
For now, the debug should focus on making the commit payloads to be 3 after the recovery.

@vncoelho
Copy link
Member

vncoelho commented Jan 6, 2021

                    Console.WriteLine($"\nHI INSIDE RECOVERY IF ...{commitPayloads.Length}");
                    totalCommits = commitPayloads.Length;
                    foreach (ExtensiblePayload commitPayload in commitPayloads)
                        if (ReverifyAndProcessPayload(commitPayload)) validCommits++;
                    Console.WriteLine($"\nVALID COMMITS ...{validCommits}");    
                    
                     HI INSIDE RECOVERY IF ...4
 
                    VALID COMMITS ...0

Something is happening on ReverifyAndProcessPayload

line if (!payload.Verify(context.Snapshot)) return false; is returning false

@erikzhang
Copy link
Member Author

if (!Blockchain.Singleton.IsExtensibleWitnessWhiteListed(Sender)) return false;

The problem is that the mocked validators are not in the whitelist.

@shargon
Copy link
Member

shargon commented Jan 8, 2021

The problem is that the mocked validators are not in the whitelist.

I will try to add them with reflection

@shargon
Copy link
Member

shargon commented Jan 8, 2021

UT Fixed, we should create the module before merge it in order to test it well

@erikzhang
Copy link
Member Author

erikzhang commented Jan 8, 2021

UT Fixed, we should create the module before merge it in order to test it well

We can test it without creating module.

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.

Such a beautiful work, great job @shargon.

@erikzhang
Copy link
Member Author

@neo-project/ngd-shanghai Test?

@erikzhang
Copy link
Member Author

erikzhang commented Jan 10, 2021

I will start to create the module after merging this.

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.

It's good for me, we can test it later

@erikzhang erikzhang merged commit a9fe75d into master Jan 10, 2021
@erikzhang erikzhang deleted the extensible-consensus branch January 10, 2021 11:40
@erikzhang erikzhang added this to the NEO 3.0 milestone Jan 10, 2021
cloud8little pushed a commit to cloud8little/neo that referenced this pull request Jan 24, 2021
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Feb 16, 2021
but we keep the existing Consensus payloads instead of deleting.
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.

3 participants