Skip to content

feat: forester: multiple trees + rollover support #998

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

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

sergeytimoshin
Copy link
Contributor

No description provided.

@sergeytimoshin sergeytimoshin force-pushed the sergey/forester-trees branch 2 times, most recently from d3d13a4 to 0742fa0 Compare July 16, 2024 15:57
@sergeytimoshin sergeytimoshin marked this pull request as ready for review July 16, 2024 15:57
@sergeytimoshin sergeytimoshin marked this pull request as draft July 16, 2024 15:57
@sergeytimoshin sergeytimoshin force-pushed the sergey/forester-trees branch from f7ffdd6 to 7abd47a Compare July 16, 2024 16:01
@sergeytimoshin sergeytimoshin marked this pull request as ready for review July 16, 2024 16:01
#[derive(Debug, Clone)]
pub struct TreeSyncMetadata {
pub last_synced_at: DateTime<Utc>,
pub last_synced_root: [u8; 32],
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the conceptual idea behind this?
how often do you sync trees?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it for now.
I sync the trees on every start, but that's obviously not enough, and syncing on every rollover is not enough either.
I see 2 options:

  1. Separate thread that would subscribe to account_compression account changes and do a sync on each change.
  2. Just a cron job, every N minutes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

The forester epochs and slots will change this a bit. (Foresters are only eligible to forest during their own slots.)
It’s probably enough to sync the tree leading up and during the slot.

Comment on lines 183 to 273
async fn run_subscribe_addresses<R: RpcConnection>(
config: Arc<ForesterConfig>,
rpc_pool: RpcPool<R>,
tree_data: TreeData,
rollover_state: Arc<RolloverState>,
) {
let indexer_rpc = init_rpc(config.clone(), false).await;
let indexer = Arc::new(tokio::sync::Mutex::new(PhotonIndexer::new(
config.external_services.indexer_url.to_string(),
indexer_rpc,
)));

subscribe_addresses(config.clone(), rpc_pool, indexer).await;
subscribe_addresses(config.clone(), rpc_pool, indexer, tree_data, rollover_state).await;
}

async fn run_nullify_state<R: RpcConnection>(config: Arc<ForesterConfig>, rpc_pool: RpcPool<R>) {
async fn run_nullify_addresses<R: RpcConnection>(
config: Arc<ForesterConfig>,
rpc_pool: RpcPool<R>,
tree_data: TreeData,
rollover_state: Arc<RolloverState>,
) {
let indexer_rpc = init_rpc(config.clone(), false).await;
let indexer = Arc::new(tokio::sync::Mutex::new(PhotonIndexer::new(
config.external_services.indexer_url.to_string(),
indexer_rpc,
)));

nullify_state(config.clone(), rpc_pool, indexer).await;
nullify_addresses(config.clone(), rpc_pool, indexer, tree_data, rollover_state).await;
}
Copy link
Contributor

@ananas-block ananas-block Jul 17, 2024

Choose a reason for hiding this comment

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

Not sure how much but it feels like there is some duplicate code in this file.
It can probably be unified more or redundant functions removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

really would like to move code like this which is duplicate in forester and test-utils into a forester-utils crate which forester and test-utils depend on.

Copy link
Contributor Author

@sergeytimoshin sergeytimoshin Jul 18, 2024

Choose a reason for hiding this comment

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

I started to extract duplicated code from forester & test-utils, and I think it would be better to make it in different PR, too many changes there.

@sergeytimoshin sergeytimoshin marked this pull request as draft July 17, 2024 08:48
@sergeytimoshin sergeytimoshin force-pushed the sergey/forester-trees branch 2 times, most recently from 3367e25 to 65c73c4 Compare July 18, 2024 13:11
@sergeytimoshin sergeytimoshin force-pushed the sergey/forester-trees branch from 65c73c4 to a63c02a Compare July 18, 2024 13:12
@sergeytimoshin sergeytimoshin marked this pull request as ready for review July 18, 2024 13:29
@ananas-block ananas-block merged commit 0ca8d95 into main Jul 18, 2024
14 checks passed
@ananas-block ananas-block deleted the sergey/forester-trees branch July 18, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants