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

Support alternative transaction signing methods #309

Closed
wants to merge 13 commits into from
Closed

Support alternative transaction signing methods #309

wants to merge 13 commits into from

Conversation

CDDelta
Copy link

@CDDelta CDDelta commented Jul 6, 2021

This PR adds support for signing transactions with the secp256k1 ECDSA and ed25519 EdDSA schemes. This change is entirely backwards compatible with the existing signing method, the process for signing transactions with RSA remains the same.

Transactions can now include an optional signature_type field which can be PS256_65537, ES256K or Ed25519 to specify their signing scheme. Transactions using the new schemes should include the signature_type field as the last leaf used to generate the Merkle root used as the transaction signature message. This should not be done for the pre-existing scheme PS256_65537.

Transactions signed with secp256k1 ECDSA must specify a compressed owner (public key). Uncompressed owners are rejected to avoid confusion with a single EC wallet having two possible addresses.

signature_type field values correspond to those defined in the IANA Jose assignment database and IETF RFC8410 with an extension for PS256 to also define the RSA public exponent.

Copy link
Author

@CDDelta CDDelta left a comment

Choose a reason for hiding this comment

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

Have some questions here.

apps/arweave/src/ar_tx.erl Outdated Show resolved Hide resolved
apps/arweave/src/ar_tx.erl Outdated Show resolved Hide resolved
@CDDelta CDDelta marked this pull request as ready for review July 17, 2021 04:21
@CDDelta
Copy link
Author

CDDelta commented Jul 17, 2021

@ldmberman are there any tests that test out tx inclusion e2e ie. tx creation, /tx endpoint posting, block inclusion? Could you point me to them if there are? I think it would be good to have them updated with the new signature types as well.

@ldmberman
Copy link
Member

@CDDelta there are lots of e2e tests in the codebase, one thing I would do is make ar_wallet:new/0 return an ECDSA or EDDSA wallet and see if the tests pass, but not necessarily commit the change. To name a few places, there are complex e2e tests in ar_multiple_txs_per_wallet_tests.erl and ar_data_sync_tests.erl. It might make sense to update these to use a mix of wallets. Also, perhaps, check ar_node_tests, e.g., wallet_transaction_test_.

@CDDelta
Copy link
Author

CDDelta commented Jul 18, 2021

@CDDelta there are lots of e2e tests in the codebase, one thing I would do is make ar_wallet:new/0 return an ECDSA or EDDSA wallet and see if the tests pass, but not necessarily commit the change. To name a few places, there are complex e2e tests in ar_multiple_txs_per_wallet_tests.erl and ar_data_sync_tests.erl. It might make sense to update these to use a mix of wallets. Also, perhaps, check ar_node_tests, e.g., wallet_transaction_test_.

ar_node_tests was just what I was looking for. I believe everything is in line now. Could you have a look again? :)

Unfortunately, the tests don't all pass here since master itself is broken but it looks like the areas I touched didn't break anything.

@CDDelta CDDelta requested a review from ldmberman July 18, 2021 04:12
@CDDelta CDDelta changed the title Support alternate transaction signing methods Support alternative transaction signing methods Jul 30, 2021
@DougAnderson444
Copy link

Hello Team, wondering what the status of this PR is? Would love to see ed25519 keys used to sign Arweave transactions.

@CDDelta CDDelta closed this by deleting the head repository Apr 27, 2023
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