Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Replace txn_signature in ReplicaAccountInfo with transaction#30189

Merged
lijunwangs merged 5 commits intosolana-labs:masterfrom
ivandzen:pass-txn-ref-to-update-account
Feb 9, 2023
Merged

Replace txn_signature in ReplicaAccountInfo with transaction#30189
lijunwangs merged 5 commits intosolana-labs:masterfrom
ivandzen:pass-txn-ref-to-update-account

Conversation

@ivandzen
Copy link
Copy Markdown
Contributor

@ivandzen ivandzen commented Feb 8, 2023

Current implementation of ReplicaAccountInfo restricts possibilities for inflight account filtration

Current implementation includes transaction signature in ReplicaAccountInfo. This approach does not allow to filter accounts by matching other accounts in transaction in-flight (e. g. accept only those accounts included in transactions for specific programs). Current implementation forces to collect ALL accounts and transactions for some period of time and perform such complex filtration afterwards.

Pass reference to transaction object instead of transaction signature into ReplicaAccountInfo

Advanced in-flight filtration can be implemented in plugins by passing reference to transaction for every update_account event. This change doesn't bring any overhead comparing to current implementation (only data type of reference is changed) and brings only minor changes in source code.

@mergify mergify Bot added community Community contribution need:merge-assist labels Feb 8, 2023
@mergify mergify Bot requested a review from a team February 8, 2023 14:41
/// write_version.
pub write_version: u64,

/// Reference to transaction caused this account modification
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

caused --> causing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

pub write_version: u64,

/// Reference to transaction caused this account modification
pub txn: Option<&'a SanitizedTransaction>,
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Contributor Author

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.

@lijunwangs lijunwangs added the CI Pull Request is ready to enter CI label Feb 8, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 8, 2023
@lijunwangs
Copy link
Copy Markdown
Contributor

There is also some CI issues, please remember to do cargo fmt --all

@lijunwangs lijunwangs added the CI Pull Request is ready to enter CI label Feb 9, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 9, 2023
@lijunwangs lijunwangs added the CI Pull Request is ready to enter CI label Feb 9, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 9, 2023
Copy link
Copy Markdown
Contributor

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

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

LGTM

@lijunwangs lijunwangs merged commit 2f9146e into solana-labs:master Feb 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

community Community contribution need:merge-assist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants