-
Notifications
You must be signed in to change notification settings - Fork 82
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: remove unused state variable bridge_prover_id
#749
Fix: remove unused state variable bridge_prover_id
#749
Conversation
I'm really curious about the changes in the |
I didn't dig into it further because there were quite a few changes. I just reverted the whole file back to the version on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. But with a couple of nitpicks.
b361906
to
b1015b0
Compare
This PR is a modified version of #726 Normally I would not make a whole separate PR, but I ended up starting this work from scratch off of `develop` because I could not understand why the gas costs in `repro` were increased so much in the old PR. I have since figured out that it was some change in `Cargo.lock` that was causing the problem, but by that point I already had all this code finished in a separate branch so I thought it would be easier to make a separate PR at that point. Key differences from #726 : 1. The `repro` tests pass without any changes to the gas values (i.e. this implementation does not create a significant gas increase). This is mostly due to leaving `Cargo.lock` untouched, but there are a few other improvements as well (see below). 2. Automatically migrate the state if the V1 state is found. This change means that after the first transaction, the fallback deserialization logic will not be executed and hence reading the state will be as efficient as it always was (as exemplified by the `repro` gas costs remaining unchanged). 3. Use `Cow` in `BorshableEngineState` so that `From<&EngineState> for BorshableEngineState` does not need to clone the owner account id. 4. Ensure removing the `bridge_prover_id` field from `NewCallArgs` is done in a backwards compatible way. This is not a performance improvement, but it is still a critical change. We must ensure that changes to the format of top-level contract functions are always backwards compatible otherwise the Borealis Engine and Borealis Refiner will not be able to parse old transactions (it will fail to deserialize the arguments).
## 2.9.1 2023-05-11 ### Changes - Removed unused state variable `bridge_prover_id` by @birchmd. (#749) - `modexp` has been improved to be greatly faster than before by @birchmd. (#757) ### Fixes - Fixed an issue where the owner could call `new` multiple times by @lempire123. (#733) - Fixed an issue with `deploy_upgrade` where the upgrade index isn't cleared by @lempire123. (#741) --------- Co-authored-by: lempire123 <[email protected]> Co-authored-by: Oleksandr Anyshchenko <[email protected]> Co-authored-by: Michael Birch <[email protected]>
This PR is a modified version of #726
Normally I would not make a whole separate PR, but I ended up starting this work from scratch off of
develop
because I could not understand why the gas costs inrepro
were increased so much in the old PR. I have since figured out that it was some change inCargo.lock
that was causing the problem, but by that point I already had all this code finished in a separate branch so I thought it would be easier to make a separate PR at that point.Key differences from #726 :
repro
tests pass without any changes to the gas values (i.e. this implementation does not create a significant gas increase). This is mostly due to leavingCargo.lock
untouched, but there are a few other improvements as well (see below).repro
gas costs remaining unchanged).Cow
inBorshableEngineState
so thatFrom<&EngineState> for BorshableEngineState
does not need to clone the owner account id.bridge_prover_id
field fromNewCallArgs
is done in a backwards compatible way. This is not a performance improvement, but it is still a critical change. We must ensure that changes to the format of top-level contract functions are always backwards compatible otherwise the Borealis Engine and Borealis Refiner will not be able to parse old transactions (it will fail to deserialize the arguments).