Speed up signature check for orders with no pre-interactions

[m-lord-renkse]

Description

Speed up signature check for orders with no pre-interactions by directly calling isValidSignature instead of our Signatures contract.

Changes

• In the cases with no pre-interactions, directly call isValidSignature
• Modify the eht safe e2e tests to use properly a SAFE transaction using the corresponding signature and forcing the new code to be tested/executed

How to test

1. Acceptance tests

Performance improvement

By looking at the Signatures contract, it is apparent that the speed it is apparent that the improvement won't be significant. We are mostly passing some small checks from the smart contract to Rust code, but the heavy operations are still done in the smart contract.

I did some speed tests locally (I know the numbers will differ in real production, but I wanted to get some approximation). What I did is to do Instant::now() + now.elapse() wrapping the critical code, and after many tests, and I got in average these results:

• Previous implementation: 2982196 nanoseconds
• New implementation: 2388463 nanoseconds

There is definitely an improvement. In production the improvement should be greater, because I was using anvil in localhost which reduces the latency significantly.

[squadgazzz]

Previous implementation: 2982196 nanoseconds
New implementation: 2388463 nanoseconds
There is definitely an improvement. In production the improvement should be greater, because I was using anvil in localhost which reduces the latency significantly.

It would be better to create an image to run it on prod for a while(like it was done with recent improvement PRs). Just be aware to checkout from the latest release tag.

So, currently, I see the improvement will be in the network delay only, right? Oh, wait, we still call the contract. There won't be any improvement at all(a few nanosecs don't count IMO)?

       let magic_bytes = contract
            .methods()
            .is_valid_signature(Bytes(check.hash), Bytes(check.signature.clone()))
            .call()
            .await

[m-lord-renkse]

Previous implementation: 2982196 nanoseconds
New implementation: 2388463 nanoseconds
There is definitely an improvement. In production the improvement should be greater, because I was using anvil in localhost which reduces the latency significantly.

It would be better to create an image to run it on prod for a while(like it was done with recent improvement PRs). Just be aware to checkout from the latest release tag.

So, currently, I see the improvement will be in the network delay only, right? Oh, wait, we still call the contract. There won't be any improvement at all(a few nanosecs don't count IMO)?

       let magic_bytes = contract
            .methods()
            .is_valid_signature(Bytes(check.hash), Bytes(check.signature.clone()))
            .call()
            .await

Yeah, we should definitely run it on prod and see the impact, but I am afraid the impact will be so small that it won't be seen in the noise 🤔

The improvement is the contract we call, in the previous code, we always call a contract function which checks the signature taking into account the pre-interactions, so the smart contract checks if the pre-interactions are filled or not, and then call isValidSignature. So now, we call directly isValidSignature if the pre-interactions are empty. We move some checks from smart contract to Rust code.

Performance wise, it will be better for sure. How much? we don't know, but I assume it will reach 1 ms at most. But it is my educated guess.

[MartinquaXD]

I am afraid the impact will be so small that it won't be seen in the noise 🤔

If the impact is not measurable we shouldn't merge the code IMO. All of these optimizations are just things that could improve the runtime but ultimately might not for whatever reason so it's inherently a research task.

Oh, wait, we still call the contract. There won't be any improvement at all(a few nanosecs don't count IMO)?

Mateo's benchmark showed an improvement of 20% on the request itself. If we add network latency to the mix this in itself will likely not be a significant improvement but this could have a compounding impact due to the RPC request batching which works worse if fast and slow requests happen to be in the same batch.

Ultimately we should benchmark in prod and only merge if this improves the runtime.

[m-lord-renkse]

Ultimately we should benchmark in prod and only merge if this improves the runtime.

Yes, I agree. I already did the job, so it's worth trying it at least. I will do it on Monday (I don't want to leave it running unsupervised over the weekend) 👌

[MartinquaXD]

Yes, I agree. I already did the job, so it's worth trying it at least. I will do it on Monday (I don't want to leave it running unsupervised over the weekend) 👌

I don't want our speed initiatives to drag on too much. I can test it out over the weekend since I'll probably be working some more as oncall anyway.

[m-lord-renkse]

Yes, I agree. I already did the job, so it's worth trying it at least. I will do it on Monday (I don't want to leave it running unsupervised over the weekend) 👌

I don't want our speed initiatives to drag on too much. I can test it out over the weekend since I'll probably be working some more as oncall anyway.

@MartinquaXD That would be lovely 🙏 thank you very much!

[squadgazzz]

Lg