This repository was archived by the owner on Jan 22, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Replace txn_signature in ReplicaAccountInfo with transaction #30189
Merged
lijunwangs
merged 5 commits into
solana-labs:master
from
ivandzen:pass-txn-ref-to-update-account
Feb 9, 2023
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
d0912f3
Replace txn_signature in ReplicaAccountInfo with transaction
ivandzen 41d5e42
Fix CI-check and review issues
ivandzen 3e6b20a
Merge branch 'master' into pass-txn-ref-to-update-account
ivandzen 187ed81
Merge branch 'master' into pass-txn-ref-to-update-account
ivandzen a738d73
Merge branch 'master' into pass-txn-ref-to-update-account
ivandzen File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
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.
Can SanitizedTransaction change under our feet and resulting in missing a version update?
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.
I'm not sure how can it be possible. update_account notification happens syncronously after transaction processing in a single thread, reference is immutable. If such change is possible then it definitely happens regardless plugin behaviour.
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.
I meant update in different validator versions. I don't have a good solution either -- duplicating the object is an overkill and will likely impact performance. In the end the plugin has to compile with the validator and test to make sure it works.
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.
But there's no cloning here. It all depends on behaviour of plugin. In my implementation of plugin there's no cloning at all. This field is used to proceed filtration at the very beginning of account_update handler and not passed any further.
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.
Plugin than pass only signature of transaction to account table. And it will be enough to match accounts and transactions in different tables in database
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.
If you are talking about compatibility between different versions of validator (in case when SenitizedTransaction changed internally) then of course you should always have plugin built against corresponding validator libraries. But it is the same as for TransactionReplicaInfo - we have the same reference to SanitizedTransaction inside. Of course better way for compatibility is to have special structure which clones internals of transaction. But this approach is very inefficient in terms of performance. So I think this implementation of ReplicaAccountInfo doent not bring any additional drawbacks other than already exists in plugin engine design.
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.
Yes, we faced such issues when moving from 1.14.10 to 1.14.12 (if i'm not mistaken with numbers) - SanitizedTransaction changed it's message field type.