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 reference unlock by same public key but different address #698

Merged
merged 10 commits into from
Feb 28, 2024

Conversation

muXxer
Copy link
Contributor

@muXxer muXxer commented Feb 23, 2024

My take on #697.

Currently it is impossible to unlock an implicit account creation address and an ed25519 address which are derived from the same public key in the same transaction. This is because we disallow to have the same public key in different signatures, but we also fail to check correctly for a valid reference unlock, if the underlying public key is the same, but the address is different.

This PR changes the logic in a way that the public key that unlocked the identity gets tracked, and in a reference unlock it is checked that the specific derived address for that public key matches.

The PR introduces the concept of a SignerUID in the Signature and AddressSigner interface to identify unique signatures signers (in out case public keys).

The AddressSigner for the TransactionBuilder now needs to be passed in the constructor instead of the Build method. This breaking change was necessary because otherwise the workscore for transaction payload was wrong in certain edge cases, because the formerly used EmptyAddressSigner had no knowledge about the SignerUIDs and therefore didn't construct the ReferentialUnlocks properly. The EmptyAddressSigner was therefore removed.

The PR also removes the unused Key interface from the chain ID and other IDs, because the Key method of the underlying Address is used everywhere instead, which is more precise.

jkrvivian
jkrvivian previously approved these changes Feb 27, 2024
@muXxer muXxer force-pushed the fix/ref-unlock-by-same-pubkey-but-different-address branch from e90904a to 3bfaa08 Compare February 27, 2024 15:14
@muXxer muXxer force-pushed the fix/ref-unlock-by-same-pubkey-but-different-address branch from 3bfaa08 to 60f54ff Compare February 27, 2024 15:23
@muXxer muXxer dismissed jkrvivian’s stale review February 27, 2024 16:59

The review was done before a big refactor of the code :/

vm/vm.go Outdated Show resolved Hide resolved
vm/vm.go Outdated Show resolved Hide resolved
signature.go Outdated Show resolved Hide resolved
signature_ed25519.go Outdated Show resolved Hide resolved
unlock.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cyberphysic4l cyberphysic4l left a comment

Choose a reason for hiding this comment

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

Just a few small renames and comments suggested, but the logic changes seem quite minimal now that it's cleaned up so much and all looks good to me.

Copy link
Member

@alexsporn alexsporn left a comment

Choose a reason for hiding this comment

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

lgtm, makes it simpler to read 👍

@muXxer muXxer merged commit 25491bf into develop Feb 28, 2024
4 checks passed
@muXxer muXxer deleted the fix/ref-unlock-by-same-pubkey-but-different-address branch February 28, 2024 10:43
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