Filecoin TX params buffer size is very small

[kacperzuk-neti]

ledger-filecoin/app/src/parser_txdef.h

Line 34 in de3ee0e

#define MAX_PARAMS_BUFFER_SIZE 200

Params size is limited to 200B. We have a use case in which this isn't enough:

1. We're doing a multisig propose transaction
2. Which proposes invoking EVM contract
3. Which contains a multicall of a few different EVM calls

Our first test transactions (see CBOR-encoded tx below) have ~500B of params data, but we expect it to get to around ~1000.
Is it technically feasible to increase this buffer size?

The transaction:

8A005502972DE5CB57E5208A9FEF1887B39B2BE0EBDA5FEE5501539A38D7116E30F44985EFF6E99A2D5FBADF951901401A044A58AB44000189DD44000185BF025901E88456040ADC451DCFCE6429224C835B09E60FFAD5EBBBC9F0401AE525AA155901C75901C4AC9650D800000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000C0000000000000000000000000000000000000000000000000000000000000004443AEEEB2000000000000000000000000FF00000000000000000000000000000000023319000000000000000000000000000000000000000000000000000000000000000A000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000084F654B137000000000000000000000000FF000000000000000000000000000000000233190000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000003E800000000000000000000000000000000000000000000000000000000

🔗 zboto Link

[ainhoa-a]

Thank you for the request, we're already working into this.

[emmanuelm41]

Hey @kacperzuk-neti, thanks for your comments. We are already checking it. My guess is that this should be possible, but I want to add a test for such a case like this. Do you have the encoded blob you would send to the device? Is it possible you share it? We will add it as a test case on the app repo.

[kacperzuk-neti]

@emmanuelm41 thanks for looking into it. Encoded Filecoin message to sign is in the first ticket message, I'll copy it here:

8A005502972DE5CB57E5208A9FEF1887B39B2BE0EBDA5FEE5501539A38D7116E30F44985EFF6E99A2D5FBADF951901401A044A58AB44000189DD44000185BF025901E88456040ADC451DCFCE6429224C835B09E60FFAD5EBBBC9F0401AE525AA155901C75901C4AC9650D800000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000C0000000000000000000000000000000000000000000000000000000000000004443AEEEB2000000000000000000000000FF00000000000000000000000000000000023319000000000000000000000000000000000000000000000000000000000000000A000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000084F654B137000000000000000000000000FF000000000000000000000000000000000233190000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000003E800000000000000000000000000000000000000000000000000000000

You can decode it on cbor.me. Though as mentioned, we expect it to grow to perhaps even twice this size depending on exact parameters.

Is this what you need or do you need something lower-level? APDU commands? We're using INS_SIGN_SECP256K1 command via sign function of https://github.com/Zondax/ledger-filecoin-js

[emmanuelm41]

I am sorry I did not see the blob on your first message. That is exactly what I need. I am checking right now!

[emmanuelm41]

@kacperzuk-neti I am currently working on it here #167. As it is right now, I see an error parsing the cbor-encoded tx. I need to check what it is happening. According to cbor.me, the blob is a valid cbor.

[emmanuelm41]

@kacperzuk-neti now it is working. I need to adjust now the max value, as it won't fit on nano s devices. Resources are really different between that one and the others, so I don't think it will be feasible on nano s devices

[kacperzuk-neti]

Maybe streaming would be an option as a long-term solution? Here's a sample from ethereum app: https://github.com/LedgerHQ/app-ethereum/blob/develop/src_features/signMessageEIP712/field_hash.c#L239

Instead of loading all into memory and then hashing and signing it, they take a chunk, ask user to confirm on device, update the hash and drop the data. That way they support arbitrarily long data.
But it seems that it would require refactoring a lot of code, so that's definitely not something that can be done right away.

Alternatively the data could perhaps be stored in NVRAM? That would make the app larger, but wouldn't require as much changes in the code.

[emmanuelm41]

Yes, I agree there are many options to have support to much bigger params fields. I went straight to the quicker one, as it is easier to increase that buffer size, which will increase the stack usage. One thing is the buffer, other is the space used to show that param on the screen. Storing the input data on nvram is something we already do. But here we are talking about parsing mostly. We could implement lazy parsing too, which would allow us to parse just the piece of info we need, use it, and discard it.

I added a zemu test for the tx blob provided, and it is working for all devices, except nano s. That should unlock you now! Could you please have a look and let me know?

[emmanuelm41]

@kacperzuk-neti It is already merged on master. Now we will send the PR to ledger. It will be released once ledger does it!

[kacperzuk-neti]

Sounds great, thank you! It will definitely help a lot already.
Are there any plans on implementing one of the other options, so that Nano S can get the improvement as well?

[disappearer]

I too have a nano s (plus) ledger.