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.

Sign up for GitHub

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

Jump to bottom

[ETCM-174] Sorting of checkpoint signatures #738

Merged
merged 1 commit into from
Oct 26, 2020
Merged

[ETCM-174] Sorting of checkpoint signatures #738

merged 1 commit into from
Oct 26, 2020

Conversation

pslaski
Copy link
Contributor

@pslaski pslaski commented Oct 15, 2020

Description

Sort checkpoint signatures lexicographically to prevent having the same checkpoint block with a different hash (different order of signatures).

Remember about changes on OBFT node side.

Important Changes Introduced

  • added validator of lexicographical order of checkpoint signatures

@pslaski pslaski requested review from rtkaczyk and ntallar October 15, 2020 09:48
@pslaski pslaski mentioned this pull request Oct 15, 2020
Merged
@rtkaczyk rtkaczyk added BREAKS PROTOCOL Affects Node2Node interactions in a way that makes them not interoperate don't merge labels Oct 15, 2020
@rtkaczyk
Copy link
Contributor

What if we sorted the sigs ourselves in CheckpointBlockGenerator? Then we wouldn't require any changes on OBFT side, right?

@pslaski
Copy link
Contributor Author

pslaski commented Oct 16, 2020

that's right, will do the update

@pslaski
Copy link
Contributor Author

pslaski commented Oct 16, 2020

waiting for #728

@lemastero
Copy link
Contributor

After margin [ETCM-141] scalafmt . This PR can be updated by git merge etcm-174-validate-lexicographically-signatures-fmt

Changes:
+55 −23
+62 -23

@pslaski pslaski force-pushed the etcm-174-validate-lexicographically-signatures branch 3 times, most recently from e3c7ca3 to 8a69185 Compare October 22, 2020 10:32
ntallar
ntallar requested changes Oct 23, 2020
View reviewed changes
Copy link

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

Only a really minor change and LGTM! Should we document this change on our checkpointing spec?

src/main/scala/io/iohk/ethereum/consensus/validators/BlockWithCheckpointHeaderValidator.scala Show resolved Hide resolved
@pslaski
Copy link
Contributor Author

pslaski commented Oct 23, 2020

I will update docs after merge

@pslaski pslaski force-pushed the etcm-174-validate-lexicographically-signatures branch from 8a69185 to 8ce96d7 Compare October 23, 2020 14:31
@rtkaczyk rtkaczyk removed BREAKS PROTOCOL Affects Node2Node interactions in a way that makes them not interoperate don't merge labels Oct 23, 2020
@ntallar ntallar added the BREAKS PROTOCOL Affects Node2Node interactions in a way that makes them not interoperate label Oct 23, 2020
ntallar
ntallar approved these changes Oct 23, 2020
View reviewed changes
Copy link

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

LGTM!

 .\\            //.
. \ \          / /.
.\  ,\     /` /,.-
 -.   \  /'/ /  .
 ` -   `-'  \  -
   '.       /.\`
      -    .-
      :`//.'
      .`.'
      .' BP

@pslaski pslaski force-pushed the etcm-174-validate-lexicographically-signatures branch from 8ce96d7 to c515106 Compare October 26, 2020 07:44
@pslaski pslaski merged commit cd5ae33 into develop Oct 26, 2020
@pslaski pslaski deleted the etcm-174-validate-lexicographically-signatures branch October 26, 2020 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntallar ntallar ntallar approved these changes

@rtkaczyk rtkaczyk rtkaczyk approved these changes

Assignees
No one assigned
Labels
BREAKS PROTOCOL Affects Node2Node interactions in a way that makes them not interoperate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pslaski @rtkaczyk @lemastero @ntallar