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 the signature verification #46

Merged
merged 6 commits into from
Nov 22, 2024
Merged

Conversation

ptagl
Copy link
Contributor

@ptagl ptagl commented Nov 20, 2024

This pull request introduces a new unit test to check the correct verification of signatures.

The test takes a signed transaction generated on devnet using telos-native-docker and performs the following steps:

  • check the decoding
  • check that the signature matches the computed one (as the private key is known)
  • check that the signature can be verified correctly

In order to make the test work it was necessary to update the constant RECOVERY_ID_ADDITION as the value used by antelope.rs differed from the one used by Wharfkit and Leap.

This change impacts the computation of the signature, meaning that signing the same transaction before and after this change produces two different signatures, but the public key is always the same, making the change retro-compatible.

Some JSON test files used by the MockProvider had to be renamed though (as the mock transaction generated have a different signature now).

@ptagl ptagl requested review from poplexity and MarcoOl94 November 20, 2024 15:28
Copy link
Contributor

@MarcoOl94 MarcoOl94 left a comment

Choose a reason for hiding this comment

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

LGTM!
I would investigate how difficult it would be to introduce a flag / compile option to use the old RECOVERY_ID while keeping some compatibility with the previous signature.

Copy link
Member

@poplexity poplexity left a comment

Choose a reason for hiding this comment

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

LGTM 👍

…ures

The test attempts to deserialize a known transaction generated with a `devnet` node using the `alice` account.
There are now additional checks related to the computation of the signature.

The assertion that was commented out is now working as expected, the issue was that the verification functions were used passing the digest as input while they expect the pre-image message (before it's hashed).
The change is needed now that the constant `RECOVERY_ID_ADDITION` has been modified, as the computed signature is different than before.
@ptagl ptagl force-pushed the pt/transaction_signature_verification branch from ef20af0 to b8445eb Compare November 22, 2024 08:55
@ptagl ptagl merged commit 013e56a into master Nov 22, 2024
4 checks passed
@ptagl ptagl deleted the pt/transaction_signature_verification branch November 22, 2024 08:57
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.

3 participants