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

Simplify txbuilder and improve keybase signing #1627

Merged
merged 8 commits into from
Feb 4, 2020
Merged

Conversation

NicolasMahe
Copy link
Member

@NicolasMahe NicolasMahe commented Feb 2, 2020

This PR remove the custom TxBuilder that we have in favour of using the default one directly.
The custom keybase signing has been re-implemented to add a cache on the plain private key of the user. This impressively improves the performance! The engine can sign 1,000 of txs in less than 1 sec (before only around 3 or 4 per sec)

Related to #1570

Next step is to investigate mempool issue by running the monitoring tool

@NicolasMahe NicolasMahe self-assigned this Feb 2, 2020
Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

I don't really like the management of the cache for the private key.

  • The delay might be unnecessary for now
  • The management of the lastSign is not clear. We should support have a map to cache all of them or none of them.

@NicolasMahe
Copy link
Member Author

NicolasMahe commented Feb 3, 2020

I don't really like the management of the cache for the private key.

  • The delay might be unnecessary for now
  • The management of the lastSign is not clear. We should support have a map to cache all of them or none of them.

I remove the cache based on time in favour of a cache using a map to store all private keys used. The index of the map is the sha256 checksum of the account's name and password.


Performance comparison with the e2e tests execution/many_executions_in_parallel with a block time of 1sec that creates 10 executions:
before: 13.31s
after: 2.10s

🍾 🎊

Copy link
Member

@antho1404 antho1404 left a comment

Choose a reason for hiding this comment

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

Would be nice to have the actual performance improvement with some metrics like #1570 (comment) to really see the impact of it ;)

cosmos/keybase.go Outdated Show resolved Hide resolved
cosmos/keybase.go Outdated Show resolved Hide resolved
cosmos/keybase.go Outdated Show resolved Hide resolved
cosmos/keybase_test.go Outdated Show resolved Hide resolved
cosmos/keybase.go Outdated Show resolved Hide resolved
cosmos/client.go Outdated Show resolved Hide resolved
cosmos/keybase_test.go Outdated Show resolved Hide resolved
cosmos/keybase.go Show resolved Hide resolved
@NicolasMahe
Copy link
Member Author

NicolasMahe commented Feb 4, 2020

Here is the result before and after with the monitoring metrics:
before:
Screen Shot 2020-02-04 at 14 50 18
everything is stuck and i had to stop the process at 14:34

after:
Screen Shot 2020-02-04 at 15 47 54
no issue at all. ALL erc20 events are saved to influxdb!!
45,000 txs executed in 45min. and that's not even the maximum!

@antho1404 check again now we have the dashboard in place!

@krhubert krhubert merged commit bea066b into dev Feb 4, 2020
@krhubert krhubert deleted the fix/tx-signing branch February 4, 2020 11:44
@NicolasMahe NicolasMahe added this to the next milestone Feb 5, 2020
@NicolasMahe NicolasMahe added the release:fix Pull requests that fix something label Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix Pull requests that fix something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants