Implement musti-signing signature verification. #85
Conversation
|
Looks good, but a little bit confused by your reference to arrays, which are fixed length in Go. Are you talking about the ripple binary format ST_ARRAY? Probably should be explicit in your commit message if so. There is no ST_SLICE :-) |
dzmitryhil
force-pushed
the
dzmitryhil/multisigning-signature-verification
branch
from
15bf92a to
214e276
Compare
Sure, updated the commit message. |
@donovanhide Is it good now? |
|
Sorry, not sure if your force-push worked. Can't see any changes to the commit message. |
|
@donovanhide Check it here. |
data/signing_test.go
Outdated
| const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" | ||
| password := make([]byte, 20) | ||
| for i := range password { | ||
| password[i] = letterBytes[rand.Intn(len(letterBytes))] |
There was a problem hiding this comment.
This is obviously a very bad way to generate a password :-) I know it's only a test, but people copy and paste code. Maybe either just pass the password as a fixed string for each signer to genSeed, or use:
https://pkg.go.dev/crypto/rand#Read
and convert the []byte to a string.
dzmitryhil
force-pushed
the
dzmitryhil/multisigning-signature-verification
branch
from
214e276 to
cb4bf49
Compare
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| seed, err := NewSeedFromAddress(seedFromPass.String()) |
There was a problem hiding this comment.
You don't need this second seed. Use the Payload() function.
https://github.com/rubblelabs/ripple/blob/master/crypto/key_test.go#L65
There was a problem hiding this comment.
Could you elaborate on it a bit?
Becase in the example you sent NewECDSAKey(seed.Payload()) the key is generated from the seed, and in the test we generate the Key from the seed seed1.Key(ECDSA) which is similar.
There was a problem hiding this comment.
I guess it's just a bit of pointless backwards and forwards between unnecessary base58 encoding. I wrote all this code 9 years ago and can barely remember half of it!
I don't really have time to do full code reviews on this and other PRs, paying work takes priority :-) I'd just say read all the existing tests, make sure all your tests log the reason for failure with a contextual sentence, expected and result text and use testing.FatalF to do so:
https://pkg.go.dev/testing#T.Fatalf
Happy to merge once done.
If you want a challenge, you could port all the existing tests from go.check to testing.T 👍
There was a problem hiding this comment.
I've added testing.FatalF where it gives more information.
What is regarding to go.check , I might do it in the future, once start touching the code it tests.
data/signing_test.go
Outdated
| tx.Signers[0].Signer.TxnSignature = tx.Signers[1].Signer.TxnSignature | ||
| valid, invalidSigners, err = CheckMultiSignature(tx) | ||
| if valid { | ||
| t.Fatal(err) |
There was a problem hiding this comment.
Why is this same err in all the Fatal calls? You need an appropriate error string for each test failure. The first test should be if err!=nil
There was a problem hiding this comment.
You are right, updated.
dzmitryhil
force-pushed
the
dzmitryhil/multisigning-signature-verification
branch
from
cb4bf49 to
a2a74d6
Compare
* Add multi-signing signature verification function. * Move `ignoreSigningFields` check on top level to fix the incorrect encoding of the empty ST_ARRAY.
dzmitryhil
force-pushed
the
dzmitryhil/multisigning-signature-verification
branch
from
a2a74d6 to
18baf65
Compare
|
@donovanhide Do you want me to change anything else in that PR ? |
|
I left comments here: I don't really have time to review all this and your colleague's work. The changes you are asking for have no effect on my private usage of this library. Might be better for you to fork this library and just do whatever it is you need for your work. I don't really want to deal with other users complaining about broken API's and bugs, there is no upside :-) This library is ancient and not really what I would write today. |
No problem, I'll cancel that PR and create to our fork, just wanted them to be on-track. |
norbertvannobelen
deleted the
dzmitryhil/multisigning-signature-verification
branch
Description
The PR contains logic and tests for the multisig-signing transactions signature verification.
The changes in the
data/encoder.goare required for the signature verification, since with the current logic theignoreSigningFieldsworks incorrectly with the slices/arrays. If we ignore the array, for exampleSigners, it adds the start and end of the array to the decoded transaction, without the array content, which is invalid, and leads to the signature verification failure.