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

Fix prepare request in recovery message #765

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

ZhangTao1596
Copy link
Collaborator

@ZhangTao1596 ZhangTao1596 commented Sep 21, 2022

ValidatorIndex is uninitialized and unassigned in PrepareRequestMessage of RecoveryMessage. This will cause prepare request reverify failure when recovery.

@roman-khimov
Copy link
Contributor

It can be set on the receiver side as well, somewhere in GetPrepareRequestPayload:

internal ExtensiblePayload GetPrepareRequestPayload(ConsensusContext context)
{
if (PrepareRequestMessage == null) return null;
if (!PreparationMessages.TryGetValue(context.Block.PrimaryIndex, out PreparationPayloadCompact compact))
return null;
return context.CreatePayload(PrepareRequestMessage, compact.InvocationScript);
}

That's what NeoGo currently does in https://github.com/nspcc-dev/neo-go/blob/5eb4ba772f4d0e507d8a77699b3856f340c36fe8/pkg/consensus/recovery_message.go#L205

@vncoelho
Copy link
Member

I agree with @roman-khimov , this will also be better for mainnet compatibility at this moment.

@vncoelho
Copy link
Member

vncoelho commented Sep 21, 2022

@ZhangTao1596, I am double checking your commit, in fact, it will not affect payload serialization/de-serialization itself.
As we see, currently, it is not assigned and keeps null?

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.

@superboyiii, if possible try an extended test.

We use to have Unit Tests for consensus, that we created sometime ago.
@erikzhang, do you know why they were not ported for neo-modules?

That Akka tests can cover several cases, such as this.

@superboyiii
Copy link
Member

@superboyiii, if possible try an extended test.

We use to have Unit Tests for consensus, that we created sometime ago. @erikzhang, do you know why they were not ported for neo-modules?

That Akka tests can cover several cases, such as this.

I will.

Copy link
Member

@superboyiii superboyiii left a comment

Choose a reason for hiding this comment

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

Tested, it works well.

@superboyiii superboyiii merged commit 2b9aa13 into neo-project:master Sep 27, 2022
@ZhangTao1596 ZhangTao1596 deleted the fix-recovery branch September 28, 2022 00:45
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

5 participants