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

fix sync crashing with constraint fails, refactor db queries, improve performance #23

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

haardikk21
Copy link
Contributor

@haardikk21 haardikk21 commented Mar 7, 2024

This PR does a few things:

  • Solves eth: start_block recovery after crashing during sync #22
  • Refactor database schema to use (transaction_hash, log_index) as primary key for chain_events and all related tables
  • Refactors database queries to avoid running into conflicts, delete unused queries and code
  • Fixes a bug with ethers-rs that was leading to IdRegistry events being unable to be parsed properly
  • Improves performance a bit by fetching logs for each contract in a single call for all events we care about instead of one-by-one per event
  • Adds a nice little updating progressbar for sync progress instead of basic println! statements

Copy link
Contributor

@gregfromstl gregfromstl left a comment

Choose a reason for hiding this comment

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

One small thing: could we make the progress bar pick up from where it left off when restarting? Besides that LGTM

@haardikk21
Copy link
Contributor Author

One small thing: could we make the progress bar pick up from where it left off when restarting? Besides that LGTM

yeah i went back and forth on making it start from FARCASTER_START_BLOCK vs the current block it's at

i'll update to make it start from FARCASTER_START_BLOCK to make it look nicer and the % mean something tangible

@haardikk21 haardikk21 requested a review from avichalp March 9, 2024 08:39
Copy link
Collaborator

@avichalp avichalp left a comment

Choose a reason for hiding this comment

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

LGTM overall. Left some questions in comments for just for further clarification.

@@ -1,6 +1,7 @@
use super::errors::*;

const FARCASTER_EPOCH: i64 = 1609459200000;
const FARCASTER_EPOCH_IN_SECONDS: u32 = 1609459200;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the rationale behind keeping two variables here? Does it improves performance because I could imagine this function being called on every message received for validation?

let logs = get_logs(self.provider.clone(), &filter).await?;
.topic0(vec![
get_signature_topic(REGISTER_SIGNATURE),
get_signature_topic(TRANSFER_SIGNATURE),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice!

}
Err(e) => {
warn!("Failed to process Transfer log: {:?}", e);
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have a doubt here. Shouldn't we return an error early if we fail to process a log? Why silence these errors?

// Overriding the `signature` is required here due to a bug in ethers-rs that happens if we mention the `key` parameter as `Bytes`
// Since we have to use `H256` for `key`, the calculated signature doesn't match the actual signature for this event
#[derive(Debug, Clone, EthEvent)]
#[ethevent(signature = "0x09e77066e0155f46785be12f6938a6b2e4be4381e59058129ce15f355cb96958")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

How was it syncing before without overriding the sig? 🤔

row.fid,
row.units,
hex::encode(&row.payer)
);
values.push(value);
}
let query = format!(
"INSERT INTO storage_allocations (id, rented_at, expires_at, chain_event_id, fid, units, payer) VALUES {}",
values.join(", ")
"INSERT INTO storage_allocations (id, rented_at, expires_at, transaction_hash, log_index, fid, units, payer) VALUES {} {}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we not using query_builder anymore? I personally found that approach to be more readable.

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.

3 participants