[FVM] reducing register touches #2502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
FVM Benchstat comparisonThis branch with compared with the base branch onflow:master commit dc85e89 The command Bench tests were run a total of 7 times on each branch. Collapsed results for better readability
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ramtinms
commented
May 26, 2022
| @@ -34,7 +34,7 @@ type Context struct { | |||
| CadenceLoggingEnabled bool | |||
| EventCollectionEnabled bool | |||
| ServiceEventCollectionEnabled bool | |||
| AccountFreezeAvailable bool | |||
| AccountFreezeEnabled bool | |||
There was a problem hiding this comment.
renamed it to match other ones.
| // all accounts provides enoguh weights | ||
| // | ||
| // if KeyWeightThreshold is set to a negative number, signature verification is skipped | ||
| type TransactionVerifier struct { |
There was a problem hiding this comment.
Whats the benefit of merging all the checks into one processor?
I was thinking that the frozen checker could actually be run for all transactions in a block(chunk?) in parallel before any transaction invocation. This would only change the behaviour of the freeze so that it is valid only from the next block on, which I think would be acceptable.
There was a problem hiding this comment.
I think checking frozen accounts should happen at the time of the register reads, for example, we do have extra checks when a contract is going to load from an account that is not the authorizer of the tx but should still block that call, etc ...
ramtinms
commented
Jun 3, 2022
Comment on lines
+12
to
+13
| maskExist byte = 0b0000_0001 | ||
| maskFrozen byte = 0b1000_0000 |
There was a problem hiding this comment.
Instead of using enum style I did it mask style, cause if something gets frozen and unfrozen we can maintain future states for accounts, while maintaining frozen state
ramtinms
commented
Jun 3, 2022
Comment on lines
-32
to
-35
| func keyPublicKey(index uint64) string { | ||
| return fmt.Sprintf("public_key_%d", index) | ||
| } | ||
|
|
There was a problem hiding this comment.
moved it to the end of file
fxamacker
approved these changes
Jun 21, 2022
Comment on lines
+254
to
+269
| for _, authorizer := range tx.Authorizers { | ||
| err := accounts.CheckAccountNotFrozen(authorizer) | ||
| if err != nil { | ||
| return fmt.Errorf("checking frozen account failed: %w", err) | ||
| } | ||
| } | ||
|
|
||
| err := accounts.CheckAccountNotFrozen(tx.ProposalKey.Address) | ||
| if err != nil { | ||
| return fmt.Errorf("checking frozen account failed: %w", err) | ||
| } | ||
|
|
||
| err = accounts.CheckAccountNotFrozen(tx.Payer) | ||
| if err != nil { | ||
| return fmt.Errorf("checking frozen account failed: %w", err) | ||
| } |
There was a problem hiding this comment.
Not sure if it's feasible, maybe we can batch read account status registers for multiple addresses and then check frozen status for all read registers.
There was a problem hiding this comment.
nice idea, though it requires way more changes as I don't think we can read batch of registers yet from inside the fvm yet. going to create a follow up PR to batch read in places that are possible.
janezpodhostnik
approved these changes
Jun 21, 2022
|
bors merge |
bors bot
added a commit
that referenced
this pull request
Jun 22, 2022
2502: [FVM] reducing register touches r=ramtinms a=ramtinms This PR - merges `frozen` and `exists` register keys into a new register (AccountStatus) - adds a migration for ^ change - removes frozen enabling step as it's already covered inside transaction ENV dynamically. - moves remaining frozen checks into the tx verifier process (less allocation) - optimized getContract method call for happy path, so we don't need to load `contract_names` registers by default (this saves at least 4-5 register reads per tx) - removes old migration codes Co-authored-by: ramtinms <ramtin@axiomzen.co> Co-authored-by: Ramtin M. Seraj <ramtinms@users.noreply.github.com>
|
bors cancel |
|
Canceled. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR
frozenandexistsregister keys into a new register (AccountStatus)contract_namesregisters by default (this saves at least 4-5 register reads per tx)