Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions geyser-plugin-interface/src/geyser_plugin_interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,52 @@ pub struct ReplicaAccountInfoV3<'a> {
pub txn: Option<&'a SanitizedTransaction>,
}

#[derive(Debug, Clone, PartialEq, Eq)]
#[repr(C)]
/// Updater of the account, which can be either a transaction or the runtime.
pub enum ReplicaAccountUpdater<'a> {
Runtime,
Transaction {
message_hash: &'a Hash,
signature: &'a Signature,
is_simple_vote: bool,
},
}

#[derive(Debug, Clone, PartialEq, Eq)]
#[repr(C)]
/// Information about an account being updated
pub struct ReplicaAccountInfoV4<'a> {
/// The Pubkey for the account
pub pubkey: &'a [u8],

/// The lamports for the account
pub lamports: u64,

/// The Pubkey of the owner program account
pub owner: &'a [u8],

/// This account's data contains a loaded program (and is now read-only)
pub executable: bool,

/// The epoch at which this account will next owe rent
pub rent_epoch: u64,

/// The data held in this account.
pub data: &'a [u8],

/// A global monotonically increasing atomic number, which can be used
/// to tell the order of the account update. For example, when an
/// account is updated in the same slot multiple times, the update
/// with higher write_version should supersede the one with lower
/// write_version.
pub write_version: u64,

/// The updater of the account, which can be either a transaction or the
/// runtime.
pub updater: ReplicaAccountUpdater<'a>,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am concerned with such changes removing fields for the transaction. For plugins relying on this, they would need to make large changes to look up. ReplicaTransactionInfoV3 already exposes SanitizedTransaction so this change little facts on the encapsulation side. Also, SanitizedTransaction is not exactly internal as it is declared in the SDK. I think we should address them comprehensively in a new breaking change for interfaces.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ReplicaTransactionInfoV3 already exposes SanitizedTransaction so this change little facts on the encapsulation side

I'm not sure what you mean. You reviewed #6515 which removes SanitizedTransaction from ReplicaTransactionInfoV3 in favor of using VersionedTransaction.

Also, SanitizedTransaction is not exactly internal as it is declared in the SDK.

Unfortunately many internal types have been added to the SDK and we are trying to clean that up. SanitizedTransaction cannot be used for transactions that fail account resolution so we need to migrate away from using that type.

I think we should address them comprehensively in a new breaking change for interfaces.

Yes, they will need to migrate when they upgrade to agave v3 which is an appropriate time to make such breaking changes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I meant removing the transaction and forcing the current plugin to the additional lookup. I am okay to change to VersionedTransaction here as well. I would be easier for the plugin to change to integrate the new changes. Otherwise, they would have to cache the transaction changes or do additional lookup in the database. Some plugin may not even subscribe to the transaction interface and solely depends on this interface.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah I'm ok with switching to VersionedTransaction here but I don't think it's the best idea because unlike transaction updates, we wouldn't have any transaction metadata like resolved lookup table addresses

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You meant it did not have TransactionStatusMeta?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah, exactly

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The earlier account updater interface did not have TransactionStatusMeta either. Can one still make use of the message in it?

}

/// A wrapper to future-proof ReplicaAccountInfo handling.
/// If there were a change to the structure of ReplicaAccountInfo,
/// there would be new enum entry for the newer version, forcing
Expand All @@ -119,6 +165,7 @@ pub enum ReplicaAccountInfoVersions<'a> {
V0_0_1(&'a ReplicaAccountInfo<'a>),
V0_0_2(&'a ReplicaAccountInfoV2<'a>),
V0_0_3(&'a ReplicaAccountInfoV3<'a>),
V0_0_4(&'a ReplicaAccountInfoV4<'a>),
}

/// Information about a transaction
Expand Down
25 changes: 16 additions & 9 deletions geyser-plugin-manager/src/accounts_update_notifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
use {
crate::geyser_plugin_manager::GeyserPluginManager,
agave_geyser_plugin_interface::geyser_plugin_interface::{
ReplicaAccountInfoV3, ReplicaAccountInfoVersions,
ReplicaAccountInfoV4, ReplicaAccountInfoVersions, ReplicaAccountUpdater,
},
log::*,
solana_account::{AccountSharedData, ReadableAccount},
Expand Down Expand Up @@ -121,38 +121,45 @@ impl AccountsUpdateNotifierImpl {
txn: &'a Option<&'a SanitizedTransaction>,
pubkey: &'a Pubkey,
write_version: u64,
) -> ReplicaAccountInfoV3<'a> {
ReplicaAccountInfoV3 {
) -> ReplicaAccountInfoV4<'a> {
ReplicaAccountInfoV4 {
pubkey: pubkey.as_ref(),
lamports: account.lamports(),
owner: account.owner().as_ref(),
executable: account.executable(),
rent_epoch: account.rent_epoch(),
data: account.data(),
write_version,
txn: *txn,
updater: match txn {
Some(txn) => ReplicaAccountUpdater::Transaction {
signature: txn.signature(),
message_hash: txn.message_hash(),
is_simple_vote: txn.is_simple_vote_transaction(),
},
None => ReplicaAccountUpdater::Runtime,
},
}
}

fn accountinfo_from_account_for_geyser<'a>(
&self,
account: &'a AccountForGeyser<'_>,
) -> ReplicaAccountInfoV3<'a> {
ReplicaAccountInfoV3 {
) -> ReplicaAccountInfoV4<'a> {
ReplicaAccountInfoV4 {
pubkey: account.pubkey.as_ref(),
lamports: account.lamports(),
owner: account.owner().as_ref(),
executable: account.executable(),
rent_epoch: account.rent_epoch(),
data: account.data(),
write_version: 0, // can/will be populated afterwards
txn: None,
updater: ReplicaAccountUpdater::Runtime,
}
}

fn notify_plugins_of_account_update(
&self,
account: ReplicaAccountInfoV3,
account: ReplicaAccountInfoV4,
slot: Slot,
is_startup: bool,
) {
Expand All @@ -165,7 +172,7 @@ impl AccountsUpdateNotifierImpl {
for plugin in plugin_manager.plugins.iter() {
let mut measure = Measure::start("geyser-plugin-update-account");
match plugin.update_account(
ReplicaAccountInfoVersions::V0_0_3(&account),
ReplicaAccountInfoVersions::V0_0_4(&account),
slot,
is_startup,
) {
Expand Down
Loading