Merkle tree #99

relatko · 2022-07-26T20:12:11Z

Adds Merkle tree support

Thus app icon copies norrectly in non-dockerized build, or build via make shell.

pgebheim · 2022-08-05T17:39:30Z

How does this relate to #96 ?

relatko · 2022-08-08T05:55:33Z

@pgebheim This is a very basic implementation of #96. At the moment it supports 7^4 transactions (exponent is quite easy to change in the future) and avoids any optimizations and complications.

pgebheim · 2022-08-08T06:13:57Z

Thanks!

js/src/helperV2.js

janmazak · 2022-09-07T20:08:21Z

js/src/index.js

    validateCryptoOptions(cryptoOptions);

    const getVersionResponse = await this.getVersion();
    const pathSerializationVersion = (compareVersion(getVersionResponse, 0, 9, 12) <= 0) ? 0 : 1;

-    return this.signGetChunks(path, cryptoOptions, pathSerializationVersion, message).then((chunks) => {
+    // APDU call sequence without Merkle trees
+    if (compareVersion(getVersionResponse, 0, 10, 3) <= 0) {


Maybe the compareVersion function can be renamed so that the name would indicate what it does (e.g. isVersionAtLeast or something). It's unnecessarily hard to understand what <= 0 means for its result.

It is quite standard three-way comparison. Maybe slightly less readable but slightly more flexible. I think I will keep it now, but I will consider this change when there is more backward compatibility code.

js/src/index.js

janmazak · 2022-09-07T20:48:04Z

js/src/index.js

@@ -98,6 +100,31 @@ export default class FlowApp {
    return FlowApp.prepareChunks(serializedPath, message);
  }

+  async signGetChunksv2(path, options, pathSerializationVersion, message, scriptHash) {


I don't understand this function. E.g.

what are basic chunks vs. other chunks, or actually what are chunks?

why is it async? all of the code seems synchronous in the end (or I have overlooked something)

since there are no types, code like scriptHash.slice(0, 16) is incomprehensible to me; perhaps we can extract the operation ".slice(0,16)" into a function with a meaningful name

js/src/helperV2.js

docs/APDUSPEC.md

janmazak

Looks OK, some suggestions for improvements in the comments.

janmazak · 2022-09-08T09:58:49Z

app/src/common/app_main.c

+            return false;
+        case 4:
+            if (validateStoredTxMetadataMerkleTreeLevel(&(G_io_apdu_buffer[OFFSET_DATA]), rx - OFFSET_DATA) != PARSER_OK) {
+                THROW(APDU_CODE_DATA_INVALID);


It would be safest to reset the proof state before THROW. The same in case 5 and possibly elsewhere (e.g. if it is possible to send APDU 1 before APDU 0).

app/src/common/app_main.c

README.md

janmazak · 2022-09-08T11:25:06Z

README.md

+
+1. Add/modify the manifest files in `transaction_metadata` directory, modify the script to include new files.
+2. Copy the resulting `txMerkleTree.js` file to `js/src`. Re-build js and re-install speculos (to include newly build js).
+3. Take the top level hash from `txMerkleTree.js`  and move it to `merkleTreeRoot` variable of `app/src/tx_metadata.c` (you need to split the hex into C array of uint8_t's).


This is annoying and error-prone. It would be better if an appropriate C source file was generated by some script and included in tx_metadata.c (e.g. as with https://github.com/vacuumlabs/ledger-app-cardano-shelley/tree/develop/tokenRegistry).

For now, I do not want to introduce anything elaborate and I want to work hands on for some time. As the comment bellow suggest, changes will be necessary very soon (backward compatibility, scaling up the processes 100s of transactions). When I will have enough information I will definitely automate this but as of now I am not sure how do this in such a way that I will not have to rework the solution soon.
Errors should be easily detected by tests, so I am not that worried about this.

README.md

app/src/tx_metadata.c

janmazak · 2022-09-08T11:58:19Z

app/src/tx_metadata.c

+        return PARSER_METADATA_ERROR;
+    }
+
+    //validate that current hash is in the list


I'd rather move this calculation into a function, it's a small detail that takes too much space (and would be covered in a single statement if this was not C).

My opinion is that this function is quite simple and this represent the the non-trivial thing the function does.

app/src/tx_metadata.c

transaction_metadata/index.js

Wolog2021 · 2022-10-03T21:52:14Z

app/Makefile

@@ -48,7 +48,7 @@ endif

 APPVERSION_M=0
 APPVERSION_N=10
-APPVERSION_P=0
+APPVERSION_P=4


Are we jumping from 0.10.0 to 0.10.4?

I am leaving space. We plan to insert a small release containing staking transactions. This will be 0.10.1. I can see 0.10.2 somehow happen. Maybe we should change it to 0.11.0. What do you think?

Because of the breaking change?

relatko added 9 commits May 25, 2022 11:01

Move app icon copying from to app/Makefile

d56f695

Thus app icon copies norrectly in non-dockerized build, or build via make shell.

Merkle trees to remove tx metadata from ledger app

3a546ee

Fix backward compatibility tests

ff40d58

Documentation how to modify the list of txs

6e8f61f

Modify testcase generation to contain metadata

b7b15d2

Make cpp tests work

842eebf

Remove output scan build

20fb3de

Add further negative tests + minor fixes

2cce189

Make some argument strings shorter

e358b02

relatko force-pushed the merkle_tree branch from 4916e1b to e358b02 Compare August 3, 2022 14:03

Add Github Workflows CI + bump containers

7b2a990

pgebheim added this to the v0.10.2 milestone Sep 7, 2022

janmazak reviewed Sep 7, 2022

View reviewed changes

janmazak reviewed Sep 8, 2022

View reviewed changes

relatko added 2 commits September 19, 2022 20:22

Incorporating Jan Mazak's minor review suggestions

9be4e5a

Further minor review code improvements

7f22fe0

Wolog2021 reviewed Sep 22, 2022

View reviewed changes

transaction_metadata/index.js Outdated Show resolved Hide resolved

Wolog2021 reviewed Sep 22, 2022

View reviewed changes

transaction_metadata/index.js Show resolved Hide resolved

Wolog2021 reviewed Oct 3, 2022

View reviewed changes

Remove empty branches from txMerkleTree.js

5561ccf

relatko force-pushed the merkle_tree branch from b6dc404 to 5561ccf Compare October 4, 2022 12:29

relatko added 4 commits October 5, 2022 20:40

Fix testnet mainnet compare in Merkle tree generation

d3b285f

Restrict alloved curves in Makefile + bump containers

4562aaa

Remove CircleCI

d4a8ae3

Merge branch 'to-merge-with-ledgerHQ' into merkle_tree

d96d11f

relatko force-pushed the merkle_tree branch from 54aea85 to d96d11f Compare October 10, 2022 09:54

Avoid duplicate workflows run

0aebbc2

Wolog2021 approved these changes Oct 18, 2022

View reviewed changes

Wolog2021 merged commit be60ce3 into master Oct 20, 2022

relatko mentioned this pull request Oct 25, 2022

GitHub workflows #103

Closed

relatko deleted the merkle_tree branch November 29, 2022 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merkle tree #99

Merkle tree #99

relatko commented Jul 26, 2022

pgebheim commented Aug 5, 2022

relatko commented Aug 8, 2022

pgebheim commented Aug 8, 2022

janmazak Sep 7, 2022

relatko Sep 16, 2022

janmazak Sep 7, 2022

janmazak left a comment

janmazak Sep 8, 2022

janmazak Sep 8, 2022

relatko Sep 16, 2022

janmazak Sep 8, 2022

relatko Sep 19, 2022

Wolog2021 Oct 3, 2022

relatko Oct 4, 2022

Wolog2021 Oct 4, 2022

Merkle tree #99

Merkle tree #99

Conversation

relatko commented Jul 26, 2022

pgebheim commented Aug 5, 2022

relatko commented Aug 8, 2022

pgebheim commented Aug 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janmazak 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