Full transaction format with signer integration #74

koxu1996 · 2024-04-22T13:01:43Z

This PR updates ASN.1 schema to compose full transaction data, and integrate Signer module with it.

Most importantly, this will unblock:

Signatures verification in kairos-server - Verify signatures #7.
Integration with Deposit event - Integrate kairos-tx with smart contract events #73.

Transaction format

The following top-level type was added to kairos-tx:

  Transaction ::= SEQUENCE {
    publicKey PublicKey,
    payload SigningPayload,
    hash PayloadHash,
    algorithm SigningAlgorithm,
    signature Signature,
    ...
  }

It represents full transaction data, including public key, hash (constructed from payload), algorithm (used for signing), and signature.

Notably, kairos_tx::asn::Transaction - similarly to payload - can be deterministically serialized and deserialized from bytes.

Signer integration

Interacting with full transaction is now included in the kairos-crypto package (under tx feature flag, enabled by default). The following methods are available in a signer trait:

Create full transaction - CryptoSigner::sign_tx_payload(&self, payload: SigningPayload) -> Result<Transaction, CryptoError>.
Validate transaction signature - CryptoSigner::verify_tx(&self, tx: Transaction) -> Result<(), CryptoError>.

Those are implemented for Casper signer, with full ed25519 and secp256k1 support.

github-actions · 2024-04-22T13:11:29Z

File	Coverage
All files	`52%`	❌
kairos-test-utils/src/cctl/parsers.rs	`66%`	✅
kairos-server/src/config.rs	`0%`	❌
kairos-server/src/errors.rs	`12%`	❌
kairos-server/src/state.rs	`90%`	✅
kairos-server/src/utils.rs	`22%`	❌
kairos-test-utils/src/cctl.rs	`87%`	✅
kairos-test-utils/src/kairos.rs	`95%`	✅
kairos-tx/src/asn.rs	`48%`	❌
kairos-tx/src/error.rs	`0%`	❌
kairos-server/tests/transactions.rs	`85%`	✅
kairos-server/src/state/transactions.rs	`57%`	❌
kairos-server/src/state/trie.rs	`35%`	❌
kairos-crypto/src/implementations/casper.rs	`6%`	❌
kairos-server/src/routes/deposit.rs	`88%`	✅
kairos-server/src/routes/transfer.rs	`90%`	✅
kairos-server/src/state/transactions/batch_state.rs	`42%`	❌

Minimum allowed coverage is 60%

Generated by 🐒 cobertura-action against 578f520

marijanp

Some tests would be great

marijanp · 2024-04-22T22:26:04Z

kairos-crypto/src/implementations/casper.rs

+        payload: kairos_tx::asn::SigningPayload,
+    ) -> Result<kairos_tx::asn::Transaction, CryptoError> {
+        // Validate payload signature.
+        let tx_hash = kairos_tx::hash(&payload).map_err(|_e| CryptoError::TxHashingError)?;


I think it would be great if all of these error types could take anything that is Debug and one could be able to propagate the inner errors message. I.e. I want to see what exact hashing error happened. The one that _e.to_string() would give me.

Done in 0242e4c.

marijanp · 2024-04-22T22:48:07Z

kairos-crypto/src/lib.rs

I don't see a benefit to this CryptoSigner trait. It should instead be replaced with a sign and verify generic function.
It also does too much at this point.

from_private_key_file and from_public_key should be removed and replaced with From trait implementations

to_public_key can be extracted to a From trait implementation too.

sign and verify could be extracted in a trait, but I would be surprised that there is nothing like this already somewhere.

Instead of all these steps, I would remove the CryptoSigner trait completely, and implement a generic function that says "if these traits are implemented ..." I will produce either this signed result or an error.

sign_tx could moreover be an alias for calling sign(tx.into()). If the ASN1 type would have a respective From trait implementation. And would moreover not require us to introduce a tx feature.

I would prefer if from_private_key_file remain a method, not a From impl.
I don't like From/Into doing IO.

I agree we don't need the CryptoSigner trait, since there is only one implementation just keep the Signer struct.

Currently this implementation signs transaction hashes.
I have my doubts about signing transaction hashes rather than transactions bodies.
It is more general, but I suspect it's a lot of overhead in a zkVM on simple transfer/withdraw payloads.

I have my doubts about signing transaction hashes rather than transactions bodies.

You have to hash the input before signing; signing algorithms take 255-bit input (for secp256k1) or 256-bit input (for ed25519). See https://crypto.stackexchange.com/a/59203

marijanp · 2024-04-22T22:58:30Z

kairos-tx/src/asn.rs

+#[non_exhaustive]
+pub struct Transaction {
+    pub public_key: PublicKey, // NOTE: Field name can be different than defined in schema, as only **order** is crucial
+    pub payload: SigningPayload,


I understand that you can compute the PayloadHash, I'm just wondering where the according computation happens? Or should it be added as a field here?

Thanks for catching it, I forgot to add hash field - added in 3003e72.

Computing PayloadHash happens in crypto module in sign_tx_payload(). This is the only safe method of constructing fully validated transaction.

marijanp · 2024-04-22T23:02:29Z

kairos-tx/src/lib.rs

+}
+
+// Constructor for non-exhaustive transaction struct.
+pub fn make_tx(


I don't understand the purpose of this function. it is basically new for a Transaction type. Not saying it should be a new.

You are right. I replaced this helper with constructor in 52b2854.

Avi-D-coder · 2024-04-22T23:19:12Z

Slightly off topic, but could we remove nonce from deposit?
What does it mean for the nonce of a deposit to not match account nonce.
Does the L1 contract have to reverse the deposit?
I think it would be simpler to remove it and denormalize the txn encoding.

I have to do that anyway for the trie/risc0 encoding representation of transactions.

…into feature/full-tx-format

koxu1996 · 2024-04-29T16:33:22Z

Ready for re-review, tests are missing though.

Avi-D-coder · 2024-04-29T16:37:24Z

kairos-crypto/src/lib.rs

+    #[cfg(feature = "tx")]
+    fn verify_tx(&self, tx: kairos_tx::asn::Transaction) -> Result<(), CryptoError>;
+    #[cfg(feature = "tx")]
+    fn sign_tx_payload(
+        &self,
+        payload: kairos_tx::asn::SigningPayload,
+    ) -> Result<kairos_tx::asn::Transaction, CryptoError>;
+


I don't love feature gated parts of traits.
Generally they should be separated out into a separate trait.

I also don't like having a trait with a single impl, but these issues can be addressed in followup at a much latter date.

Created follow up task - #86.

Avi-D-coder

Let's merge

Rom3dius

Good code

koxu1996 · 2024-04-30T08:43:53Z

Some tests would be great

Follow up task created - #87.

koxu1996 added 9 commits April 22, 2024 13:54

Update ASN.1 schema to support full transaction.

1c72ad7

Update kairos-tx schema to include new types.

d32ce25

Add converters for ASN.1 Signature and PublicKey.

9c2089c

Add constructor for ASN.1 Transaction.

34f3fee

Add sha2 dependency.

e549cd6

Implement transaction hashing.

171e5f9

Include kairos-tx in crypto library (feature flag).

c3fe52c

Add transaction related methods to crypto signer trait.

c44fd5f

Implement transaction signing and verification for Casper.

97adddb

koxu1996 added the demo Required for our first demo label Apr 22, 2024

koxu1996 requested review from marijanp and Avi-D-coder April 22, 2024 13:01

koxu1996 self-assigned this Apr 22, 2024

Fix clippy warnings and formatting.

6c042d5

koxu1996 marked this pull request as ready for review April 22, 2024 13:04

Add (de)serializer for full transaction.

c29f5bd

marijanp reviewed Apr 22, 2024

View reviewed changes

koxu1996 marked this pull request as draft April 29, 2024 11:46

koxu1996 added 10 commits April 29, 2024 14:01

Merge remote-tracking branch 'origin/feature/kairos-tx-improvements' …

b38f785

…into feature/full-tx-format

Allow cloning of new base types - signature and payload hash.

fca49f3

Replace make_tx helper with Transaction::new.

52b2854

Move hashing function into SigningPayload.

8015e35

Add missing PayloadHash to transaction.

3003e72

Warn about constructing transaction directly.

79c18c5

Update incorrect comment in sign_tx_payload().

54616bc

Fix formatting.

31fc954

Refactor encoding/decoding in Transaction.

609fa94

Fix formatting in crypto module.

69e1990

Propagate error details in CryptoError::TxHashingError.

0242e4c

koxu1996 marked this pull request as ready for review April 29, 2024 16:31

koxu1996 requested a review from marijanp April 29, 2024 16:32

Avi-D-coder reviewed Apr 29, 2024

View reviewed changes

Avi-D-coder approved these changes Apr 29, 2024

View reviewed changes

Merge branch 'main' into feature/full-tx-format

09cfc7e

koxu1996 requested review from jonas089 and Rom3dius April 30, 2024 08:32

Merge branch 'main' into feature/full-tx-format

578f520

Rom3dius approved these changes Apr 30, 2024

View reviewed changes

This was referenced Apr 30, 2024

Move feature gated parts of CryptoSigner trait #86

Closed

Missing tests in full transaction format #87

Open

koxu1996 merged commit f5fb2a6 into main Apr 30, 2024
7 checks passed

marijanp deleted the feature/full-tx-format branch April 30, 2024 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Full transaction format with signer integration #74

Full transaction format with signer integration #74

koxu1996 commented Apr 22, 2024 •

edited

Loading

github-actions bot commented Apr 22, 2024 •

edited

Loading

marijanp left a comment

marijanp Apr 22, 2024

koxu1996 Apr 29, 2024

marijanp Apr 22, 2024

Avi-D-coder Apr 22, 2024

xcthulhu Apr 23, 2024

marijanp Apr 22, 2024

koxu1996 Apr 29, 2024

koxu1996 Apr 29, 2024

marijanp Apr 22, 2024

koxu1996 Apr 29, 2024

Avi-D-coder commented Apr 22, 2024

koxu1996 commented Apr 29, 2024

Avi-D-coder Apr 29, 2024

koxu1996 Apr 30, 2024

Avi-D-coder left a comment

Rom3dius left a comment

koxu1996 commented Apr 30, 2024

Full transaction format with signer integration #74

Full transaction format with signer integration #74

Conversation

koxu1996 commented Apr 22, 2024 • edited Loading

Transaction format

Signer integration

github-actions bot commented Apr 22, 2024 • edited Loading

marijanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avi-D-coder commented Apr 22, 2024

koxu1996 commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avi-D-coder left a comment

Choose a reason for hiding this comment

Rom3dius left a comment

Choose a reason for hiding this comment

koxu1996 commented Apr 30, 2024

koxu1996 commented Apr 22, 2024 •

edited

Loading

github-actions bot commented Apr 22, 2024 •

edited

Loading