Bank::check_reserved_keys #3100
Conversation
apfitzge
force-pushed
the
reverify_transaction
branch
from
f8e45c9 to
9f17ee5
Compare
|
This looks like an improvement because we actually don't need to do a full signature verification at epoch boundaries. Looks like regardless of epoch boundary we still need to do the precompile verification and reload the ALTs. So the only thing we need to do at the epoch boundary is check if new reserved keys have been added this epoch? |
apfitzge
force-pushed
the
reverify_transaction
branch
from
9f17ee5 to
bc65939
Compare
runtime/src/bank.rs
Outdated
| // Precompiles may be verified at this time if feature is not activated. | ||
| // Precompile verification can be dependent on features of the bank. | ||
| let move_precompile_verification_to_svm = self | ||
| .feature_set | ||
| .is_active(&feature_set::move_precompile_verification_to_svm::id()); | ||
| if !move_precompile_verification_to_svm { | ||
| verify_precompiles(tx, &self.feature_set)?; | ||
| } |
There was a problem hiding this comment.
As I was going through to write tests, I've become a bit unsure this is necessary.
Is there a way to deactivate a feature? iirc (at least on mnb) they are only activated.
If the transaction initially passed verification (will have to reach this point), then there's no way this could fail if we've only activated features since. If we can deactivate them, then this is necessary because a precompile's activation could have been removed
@jstarry wdyt?
There was a problem hiding this comment.
Well in theory we could add a new precompile but yeah very unlikely to add one before we move precompile verification to the SVM. How about we skip precompile reverification and just add a comment to the PRECOMPILES static saying that if a new precompile is added, we will need to be mindful of reverifying them at epoch boundaries?
There was a problem hiding this comment.
Oh actually we should leave this. We verify the precompiles in the worker threads for parallelism. So this is not necessarily re-verifying, just verifying for first time.
core/src/banking_stage/consumer.rs
Outdated
| return Err(TransactionError::ResanitizationNeeded); | ||
| } | ||
| // Epoch has rolled over. Need to re-verify the transaction. | ||
| bank.reverify_transaction(tx)?; |
There was a problem hiding this comment.
I think it would be easier to follow this map function if this epoch rollover if branch was only responsible for checking the reserved key set. Then, rather than having an else branch, we always check if the ALT expired and always do precompile verification (until it's moved to SVM)
There was a problem hiding this comment.
god damn how did i not see that. thanks!
apfitzge
force-pushed
the
reverify_transaction
branch
from
032415e to
89575d3
Compare
|
rebasing and hoping CI downstream issues resolve themselves 😬 |
Problem
Summary of Changes
Bank::check_reserved_keysFixes #