Skip to content
This repository was archived by the owner on Nov 10, 2025. It is now read-only.

Fix prepare request in recovery message#765

Merged
superboyiii merged 1 commit intoneo-project:masterfrom
ZhangTao1596:fix-recovery
Sep 27, 2022
Merged

Fix prepare request in recovery message#765
superboyiii merged 1 commit intoneo-project:masterfrom
ZhangTao1596:fix-recovery

Conversation

@ZhangTao1596
Copy link
Collaborator

@ZhangTao1596 ZhangTao1596 commented Sep 21, 2022 •
edited
Loading

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

@roman-khimov
Copy link
Contributor

roman-khimov commented Sep 21, 2022

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

neo-modules/src/DBFTPlugin/Messages/RecoveryMessage/RecoveryMessage.cs

Lines 89 to 95 in c6d9640

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

vncoelho commented Sep 21, 2022

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 •
edited
Loading

@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?

vncoelho
vncoelho approved these changes Sep 21, 2022
View reviewed changes
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 commented Sep 22, 2022

@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.

@ZhangTao1596 ZhangTao1596 mentioned this pull request Sep 23, 2022
Closed
superboyiii
superboyiii approved these changes Sep 27, 2022
View reviewed changes
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 subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@shargon shargon shargon approved these changes

@vncoelho vncoelho vncoelho approved these changes

@superboyiii superboyiii superboyiii approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@ZhangTao1596 @roman-khimov @vncoelho @superboyiii @shargon