Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ledger-tool: support Geyser accounts updates #26909

Merged
merged 1 commit into from
Aug 9, 2022

Conversation

riptl
Copy link
Contributor

@riptl riptl commented Aug 3, 2022

Problem

See #26886

Support Geyser plugins in solana-ledger-tool verify

Summary of Changes

Adds --geyser-plugin-config flag to ledger-tool.

Only supports account updates for now.

Fixes #

@mergify mergify bot added the community Community contribution label Aug 3, 2022
@mergify mergify bot requested a review from a team August 3, 2022 22:19
ledger-tool/src/main.rs Outdated Show resolved Hide resolved
@riptl
Copy link
Contributor Author

riptl commented Aug 4, 2022

not sure why CI failed. probably timeout

@steviez
Copy link
Contributor

steviez commented Aug 4, 2022

not sure why CI failed. probably timeout

Yep, timed out ... I just rekicked for ya

@riptl riptl force-pushed the replay-geyser branch 2 times, most recently from e07c1f0 to 034cf55 Compare August 4, 2022 20:39
Comment on lines +936 to +940
let (confirmed_bank_sender, confirmed_bank_receiver) = unbounded();
drop(confirmed_bank_sender);
Copy link
Member

Choose a reason for hiding this comment

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

@lijunwangs can you comment on whether it's ok to setup the geyser plugin service without slot/block notifications? It looks like we expect to send block metadata notifications whenever account data notifications are enabled:

) = if account_data_notifications_enabled || transaction_notifications_enabled {

If it's ok, I think it would be better to change the confirmed_bank_receiver arg in GeyserPluginService::new to an Option and pass None instead of a receiver that will never be triggered.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is currently no means to disable block/slot notifications. The plugin implementation can itself just ignore the calls when called if they decided it is needed. I think at least the slot status is needed to make determination of the accounts data, otherwise we would not know if the accounts update are really accepted by the network.

Copy link
Member

Choose a reason for hiding this comment

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

@terorie have you looked into setting up a slot status notification sender that can be used by the ledger tool? Or do you not need it for your use-case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have you looked into setting up a slot status notification sender that can be used by the ledger tool?

@jstarry my end-goal is making the ledger-tool verify callback stream a drop-in replacement for validator. I thought it makes sense to split functionality into separate PRs but I'm happy to add all 3 (slot updates, accounts, transactions) to this one.

Or do you not need it for your use-case?

I only need account updates (for https://bpf.wtf/sol-state-history/) but a few users on CT reached out saying they want to backfill their Geyser infra with ledger-tool. I can see the value in only having to build a single Geyser integration for both history & real-time indexing.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'm happy with separate PR's, just wanted to make sure this PR puts geyser support in a useful state, which it seems it does

lijunwangs
lijunwangs previously approved these changes Aug 9, 2022
Copy link
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.

Looks good to me

@lijunwangs
Copy link
Contributor

@terorie Can you please rebase and resolve conflicts?

@mergify mergify bot dismissed lijunwangs’s stale review August 9, 2022 13:48

Pull request has been modified.

@riptl
Copy link
Contributor Author

riptl commented Aug 9, 2022

@lijunwangs done sir

@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Aug 9, 2022
@mergify mergify bot merged commit f7c6901 into solana-labs:master Aug 9, 2022
@riptl riptl deleted the replay-geyser branch August 9, 2022 15:45
xiangzhu70 pushed a commit to xiangzhu70/solana that referenced this pull request Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merge this Pull Request automatically once CI passes community Community contribution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants