Skip to content

Add signature validation tool + fix checkpointing padding. #824

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

Merged
merged 1 commit into from
Dec 1, 2020

Conversation

picnoir
Copy link

@picnoir picnoir commented Dec 1, 2020

Description

We add a signature validation tool allowing the OBFT team to create a automated integration test checking the signatures issued by Morpho.

We also pad the ECDSA Signature bigInt2Bytes output.

Note: this has been tested using the Morpho integration test suite:

https://hydra.project42.iohkdev.io/log/lak4pp2cy1ych2kzzm5d7lr4dx7jnmv4-checkpointing-node-mantis-integration-tests.drv

unpacking sources
unpacking source archive /nix/store/0zyf6r2rlypf8la9prn6ks1105cz9hww-source
source root is source
patching sources
configuring
no configure script, doing nothing
building
no Makefile, doing nothing
installing
Mantis Integration Tests
  mantis-integration
    Test the keygen against the actual Mantis parser: OK (136.19s)
      +++ OK, passed 100 tests.

All 1 tests passed (136.19s)
@nix { "action": "setPhase", "phase": "fixupPhase" }
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/7aliz8s7ipfzmbhsssrlhbhvrcpjg45v-checkpointing-node-mantis-integration-tests
patching script interpreter paths in /nix/store/7aliz8s7ipfzmbhsssrlhbhvrcpjg45v-checkpointing-node-mantis-integration-tests
checking for references to /build/ in /nix/store/7aliz8s7ipfzmbhsssrlhbhvrcpjg45v-checkpointing-node-mantis-integration-tests...

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 

I can't avoid marking this comment, but it's not a blocker for merging

args match {
case Array(pk, sig, msgHash) =>
Try {
val signature = ECDSASignature.fromBytes(ByteString(decode(sig)))
Copy link

Choose a reason for hiding this comment

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

We should probably move this decoder to ByteStringUtils and not rely here on the jsonrpc methods

@pslaski pslaski force-pushed the million-dollars-tool-felix-pays branch from 6ffbb47 to 247cd77 Compare December 1, 2020 18:05
@pslaski pslaski force-pushed the million-dollars-tool-felix-pays branch from 247cd77 to 81efecd Compare December 1, 2020 18:11
@pslaski pslaski merged commit c75dc4b into develop Dec 1, 2020
@pslaski pslaski deleted the million-dollars-tool-felix-pays branch December 1, 2020 19:10
@@ -174,7 +174,7 @@ case class ECDSASignature(r: BigInt, s: BigInt, v: Byte) {
import ECDSASignature.RLength

def bigInt2Bytes(b: BigInt) =
ByteString(ByteUtils.bigIntToBytes(b, RLength))
ByteUtils.padLeft(ByteString(b.toByteArray).takeRight(RLength), RLength, 0)
Copy link
Contributor

@aakoshh aakoshh Dec 2, 2020

Choose a reason for hiding this comment

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

By the looks of it ByteUtils.bigIntToBytes should have been doing the same thing, except it was lacking padding on the left. Unfortunately it doesn't have any docstrings or tests, so it's difficult to tell what it's supposed to be doing. I know that some private keys generated by Secp256k1 are 33 bytes and some are 32 when converted to a BigInt, which is why I had to change serialization here. If this change made a difference in your case, does it mean you had a BigInt which had a different length? Do you have examples? Should ByteUtils.bigIntToBytes also do padding? Could you add unit tests?

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.

4 participants