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

Make snapshot reverting transactions cheaper #157

Merged
merged 44 commits into from
Aug 19, 2022

Conversation

vtleonardo
Copy link
Contributor

@vtleonardo vtleonardo commented Aug 11, 2022

Scope

What is changing with this PR?
This PR is reorganizing the snapshots method to revert earlier in case of re-submission of the same snapshots and doing some housekeeping for better organization. The re-org will also improve the messages we see on the golang side when multiple validators try to do the snapshot.

Gas used by a reverted transaction by doing re-submission of same snapshot data (without the full ring buffer):
Before this PR: 210266
After this PR: 53148

@vtleonardo vtleonardo requested review from a team as code owners August 11, 2022 14:00
@github-actions github-actions bot added go Pull requests that update Go code javascript Pull requests that update Javascript code solidity labels Aug 11, 2022
@vtleonardo vtleonardo marked this pull request as draft August 11, 2022 14:02
@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #157 (579b7af) into main (4b18e3e) will increase coverage by 0.05%.
The diff coverage is 92.50%.

@@            Coverage Diff             @@
##             main     #157      +/-   ##
==========================================
+ Coverage   53.33%   53.39%   +0.05%     
==========================================
  Files         338      338              
  Lines       43025    43061      +36     
  Branches      366      366              
==========================================
+ Hits        22948    22993      +45     
+ Misses      17650    17641       -9     
  Partials     2427     2427              
Impacted Files Coverage Δ
bridge/contracts/Snapshots.sol 94.01% <91.89%> (-1.91%) ⬇️
bridge/contracts/ETHDKG.sol 85.49% <100.00%> (+0.64%) ⬆️
badgerTrie/smt_merkle_proof.go 69.53% <0.00%> (ø)
bridge/contracts/utils/ImmutableAuth.sol 52.11% <0.00%> (+1.40%) ⬆️
...idge/contracts/libraries/StakingNFT/StakingNFT.sol 100.00% <0.00%> (+3.82%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions github-actions bot removed the javascript Pull requests that update Javascript code label Aug 12, 2022
@github-actions github-actions bot added the javascript Pull requests that update Javascript code label Aug 12, 2022
@vtleonardo vtleonardo changed the title WIP: Make snapshot reverting transactions cheaper Make snapshot reverting transactions cheaper Aug 12, 2022
@vtleonardo vtleonardo marked this pull request as ready for review August 12, 2022 19:50
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Go side of things look good.

Comment on lines +212 to +213
groupSignature_;
bClaims_;
Copy link

Choose a reason for hiding this comment

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

If this is because of a linter warning for unused variables there might be a different workaround. Possibly removing the name from the signature, or replacing with a _:

function checkBClaimsSignature(bytes calldata, bytes calldata)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this is to silence the linter. Unfortunately, we cannot change the signature (both name and variables) because this Mock needs to conform to the ISnapshots solidity interface.

@nelsonhp nelsonhp merged commit f0f2967 into alicenet:main Aug 19, 2022
@ghost ghost added the t/feature New feature or request label Aug 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code javascript Pull requests that update Javascript code solidity t/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants