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

Saam/ec #240

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Saam/ec #240

wants to merge 11 commits into from

Conversation

samantehrani
Copy link

@samantehrani samantehrani commented Jan 3, 2025

This PR:

  • implements a new interface for PublicKey/PrivateKey used for signing/verification.
  • support for EC_SECP256k1 wallets (This key type is planned to be introduced in 2.9)
  • Partial/untested implementation for ED_25519 (This key type is NOT planned to be introduced in 2.9)
  • Defines notion of Identifier to be used as Transaction.owner and Block.reward_key.
  • Defines Address/Target to be Sha256(Identifier).
  • Adding support for the new Keys (public/private) to be used with the existing public interfaces, while maintaining backward compatibility.

Identifiers are always if Odd length, except for the existing RSA_65537. Definition is as follow and is backward compatible:
Let:

  • k be a wallet key used for signing/verification.

  • t be the key type of k when t∈T and T={RSA_65537, EC_SECP256K1, ...}

  • Prefix be a unique one byte prefix a given type t. {EC_SECP256K1 = 0x01}

  • PubRaw(k): be the raw public key bytes of given k.

  • PubPadded(k): when PubRaw(k) % 2 = 0 -> PubPadded(k) = PubRaw(K).

  • PubPadded(k): when PubRaw(k) % 2 = 1 -> PubPadded(k) = PubRaw(K) || 0x00 .

  • If t = RSA_65537 -> Identifier(k) = PubRaw(k).

  • If t != RSA_65537 -> Identifier(k)= p ​∥ PubPadded(K).

Erlang implementation support ArweaveTeam/arweave#683.

@rosmcmahon
Copy link
Member

hi @samantehrani can i draw your attention to this comment on your last closed PR #238 (comment)

FYI, it might be worth waiting a bit. You may have noticed there is a nodejs/web duality of crypto libraries in arweave-js. The plan is to switch over to a single webcrypto API. I was waiting until the API was marked stable in nodejs.

i'll be doing this task with priority starting Monday, it may involve dropping support for nodejsv18 LTS which i would prefer not to do, but on the surface it looks like this might not be necessary.
this also references your comment here:
Partial/untested implementation for ED_25519 (This key type is NOT planned to be introduced in 2.9)
better to have no implementation if it doesn't work. this would definitely break nodejsv18 LTS support also for key generation.

FYI 2, alternative key support needs to be enabled in the Arweave protocol.

there are no nodes to test against until after 2.9.1 release.

additionally, i notice some changes to code that was introduced for subtle backwards compatibility with existing permaweb apps. the current arweave-js tests don't cover things like that.

we should connect next week i think, and work towards a solution together?

@samantehrani
Copy link
Author

samantehrani commented Jan 3, 2025

Hello, I am not sure if I understand your point regarding comment in the last PR (whether the removal of the duality needs to happen at the same time of Secp256k1 introduction). Happy to connect next week when you get a chance to better understand your concern.

@samantehrani
Copy link
Author

Happy to remove ED_25519 implementation for later. As for Node 18 LTS support, that won't be impacted as RSA wecrpbto has stable support mark on 18LTS. It should be possible to get rid of the duality codebase with this PR, let's connect so I understand your goals better and try to align the PR.

@rosmcmahon
Copy link
Member

yeah i didn't read thru the whole PR tbh. i see you're using wasm-secp256k1 for the new supported keys.

@samantehrani samantehrani force-pushed the saam/ec branch 2 times, most recently from 7ad7d86 to 39520d8 Compare January 16, 2025 08:55
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.

2 participants