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

refactor: pubkey compendium stores pubkeys #51

Merged
merged 4 commits into from
Nov 6, 2023

Conversation

wadealexc
Copy link
Collaborator

@wadealexc wadealexc commented Nov 2, 2023

Builds on PR #28 and #43; merge those first! It looks like I'm merging 20 commits, but those are actually commits from the other PRs :) Only the final 2 commits in this PR are relevant.

Functional changes in this PR:

  • adds a new mapping to BLSPublicKeyCompendium : operatorToPubkey, which maps operator addresses to G1Point
  • adds a method BLSPublicKeyCompendium.getRegisteredPubkey(address) which performs a pubkey lookup along with validation that the pubkey exists/isn't the zero pubkey.
    • Validation logic here was previously performed in the BLSPubkeyRegistry
  • Removes the "pubkey" parameter from various functions, like in the BLSPubkeyRegistry and BLSRegistryCoordinatorWithIndices. The pubkey registry is able to look up pubkeys with just an operator address, now.

BLSPubkeyRegistry state variable naming changes (these better match the rest of the contracts):

  • quorumApk -> currentApk
  • quorumApkUpdates -> apkHistory

BLSPubkeyRegistry view function naming changes:

  • getApkIndicesForQuorumsAtBlockNumber -> getApkIndicesAtBlockNumber
  • getApkForQuorum -> getApk
  • getApkUpdateForQuorumByIndex -> getApkUpdateAtIndex
  • getApkHashForQuorumAtBlockNumberFromIndex -> getApkHashAtBlockNumberAndIndex
  • getQuorumApkHistoryLength -> getApkHistoryLength

Recommended review order:

  1. Check out the BLSPublicKeyCompendium changes in b883fd0 to see what new state we're tracking and get some context for the other changes in this commit.
  2. Review the rest of the commit, noting all the places we're no longer passing in pubkeys.
  3. Skim the final commit - this is solely naming-related changes (listed above)

... which means pubkey parameters for various functions/structs can be removed
@wadealexc wadealexc marked this pull request as ready for review November 2, 2023 19:33
Comment on lines +41 to +43
require(
pubkeyHash != ZERO_PK_HASH, "BLSPublicKeyCompendium.registerBLSPublicKey: cannot register zero pubkey"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

seems odd we weren't doing this before. @gpsanant is there something else preventing someone from registering the zero key? Lack of knowledge of the corresponding private key?
guess we could check that the G1 point doesn't equal the zero point, but this is equivalent 🤷

Copy link
Contributor

@samlaf samlaf Nov 3, 2023

Choose a reason for hiding this comment

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

I'm also curious about this. private key for zero point is just 0, so that shouldn't be the problem.
Seems like one could register by using pkG2=sigG1=0 ? I think the check e(sigG1,G2)=e(h(m),pkG2) would pass sincee(0,anything) = e(anything,0) = 0 afaiu (it's a multilinear map after all)

Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't really a substantive check, it's not needed, it doesn't prevent any attacks

Copy link
Contributor

Choose a reason for hiding this comment

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

so it might not prevent an attack per se, but it would prevent weird behavior, right?
the fact that the apk doesn't change when zero is added to or subtracted from it feels like it could lead to some strange effects. For example, any signature would appear as if you both did and didn't sign it, depending on the interpretation in the context of the code.
I get that it's like using a key that everyone else knows, but it also has this special aspect. Seems safer to avoid the possibility of it happening.

Copy link
Collaborator Author

@wadealexc wadealexc Nov 6, 2023

Choose a reason for hiding this comment

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

this isn't really a substantive check, it's not needed, it doesn't prevent any attacks

i think it's more substantive now, since nowhere else checks this. i got rid of the checks against the zero hash in the pk registry in favor of checking it here

Copy link
Contributor

@ChaoticWalrus ChaoticWalrus left a comment

Choose a reason for hiding this comment

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

Loving this PR, thank you!
I have been in favor of this storage change for a while, and this PR really helps make the simplifications it enables clear.
Renaming changes LGTM as well.
I noted one place where we are doing a redundant (and as far as I understand potentially expensive?) check, but we can debate that later / I wouldn't consider resolving the discussion to be blocking for this PR, personally.

@wadealexc wadealexc changed the base branch from m2-mainnet to alex/refactor-quorums November 6, 2023 15:41
@wadealexc wadealexc merged commit 9a060e5 into alex/refactor-quorums Nov 6, 2023
1 check passed
@wadealexc wadealexc deleted the alex/refactor-pubkey-registry branch November 6, 2023 15:52
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