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

fix: update function selector to be 4 bytes #293

Merged
merged 11 commits into from
Apr 19, 2023
Merged

Conversation

Maddiaa0
Copy link
Member

Description

Serialisation issues were being caused by the function selector in the function tree leaf being 4 bytes in typescript and 32 bytes in the circuits. This pr fixes this

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

Sorry, something went wrong.

@Maddiaa0 Maddiaa0 marked this pull request as ready for review April 18, 2023 14:51
@Maddiaa0 Maddiaa0 changed the title fix: update function selector to be 32 bytes fix: update function selector to be 4 bytes Apr 18, 2023
@PhilWindle
Copy link
Collaborator

Just a couple of things, doesn't the TS pad the selector to 32 bytes? This is done in contract_tree.ts. However, we should switch it to use the new FunctionLeafPreImage type in TS for serialisation and then keep the 2 types (TS and C++) in sync.

@spypsy
Copy link
Member

spypsy commented Apr 18, 2023

@PhilWindle I've added a commit that addresses your comment.
Using FunctionLeafPreimage class no longer pads the selector

if (fnLeaf.length !== 32 + 1 + 32 + 32) throw new Error(`Invalid length for function leaf`);
export async function computeFunctionLeaf(wasm: CircuitsWasm, fnLeaf: FunctionLeafPreimage) {
const fnLeafBuf = fnLeaf.toBuffer();
if (fnLeafBuf.length !== 32 + 1 + 32 + 32) throw new Error(`Invalid length for function leaf`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets at least make '32 + 1 + 32 + 32' a static of FunctionLeafPreImage.

Copy link
Member

@spypsy spypsy Apr 18, 2023

Choose a reason for hiding this comment

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

so this was necessary before since we were using a plain buffer as input, think it could be removed now, right?
also it should be 4 + 1 + 32 + 32 now

@Maddiaa0 Maddiaa0 mentioned this pull request Apr 18, 2023
6 tasks
@LHerskind
Copy link
Contributor

Is 4 bytes sufficient for the selector? Ethereum uses it, but it is pretty easy to make collisions. How annoying devex would collisions give in our case? @iAmMichaelConnor or @kevaundray do you have input on this?

@kevaundray
Copy link
Contributor

Is 4 bytes sufficient for the selector? Ethereum uses it, but it is pretty easy to make collisions. How annoying devex would collisions give in our case? @iAmMichaelConnor or @kevaundray do you have input on this?

I think 4 bytes is pretty small -- you only need 2^16 function signatures before you get a collision. If possible, I'd opt for 8 bytes so you need 2^32 function signatures for a collision

@Maddiaa0 Maddiaa0 requested a review from PhilWindle April 19, 2023 11:46
@@ -50,10 +51,13 @@ async function generateFunctionLeaves(functions: ContractFunctionDao[], wasm: Ci
const vkHash = await hashVKStr(f.verificationKey!, wasm);
const acirHash = keccak(Buffer.from(f.bytecode, 'hex'));
// TODO: selector is currently padded to 32 bytes in CBINDS, check this.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets remove this out of date comment

@@ -28,4 +28,8 @@ export class FunctionLeafPreimage {
const reader = BufferReader.asReader(buffer);
return new FunctionLeafPreimage(reader.readBytes(4), reader.readBoolean(), reader.readFr(), reader.readFr());
}

static verifyBufferSize(buffer: Buffer): boolean {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is needed now is it? Let's remove it.

Copy link
Member Author

@Maddiaa0 Maddiaa0 Apr 19, 2023

Choose a reason for hiding this comment

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

It is being used abis.ts line 92. Can we remove that check too? I think its still necessary as we are using a plain buffer as the input?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I just find it a bit messy. What do you think of sanity checking the length of the buffer inside toBuffer and throwing if it's wrong? It should be impossible to construct an instance of this class such that toBuffer returns a buffer of the wrong size so I think it's fair to throw. Then everything is encapsulated in this class. And lets create a readonly field in the class with the expected length.

Copy link
Member Author

Choose a reason for hiding this comment

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

that sounds good to me

cheethas added 3 commits April 19, 2023 12:01
refactor: use single method to perform length check

refactor: use single method to perform length check
@Maddiaa0 Maddiaa0 force-pushed the md/fix-func-selector branch from 2c7fd7d to c3315d3 Compare April 19, 2023 12:33
@Maddiaa0 Maddiaa0 requested a review from PhilWindle April 19, 2023 12:33
wasm.call('pedersen__init');
return Fr.fromBuffer(await wasmAsyncCall(wasm, 'abis__compute_function_leaf', { toBuffer: () => fnLeaf }, 32));
return Fr.fromBuffer(await wasmAsyncCall(wasm, 'abis__compute_function_leaf', { toBuffer: () => fnLeafBuf }, 32));
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can just pass fnLeaf into here can't we?

@Maddiaa0 Maddiaa0 requested a review from PhilWindle April 19, 2023 12:45
@PhilWindle PhilWindle merged commit f3a66a4 into master Apr 19, 2023
@PhilWindle PhilWindle deleted the md/fix-func-selector branch April 19, 2023 15:37
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.

None yet

6 participants