Skip to content

Check grafting from pre 0.0.6 to 0.0.6 and later #5929

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 5 commits into from
Apr 15, 2025
Merged

Conversation

zorancv
Copy link
Contributor

@zorancv zorancv commented Apr 7, 2025

This PR tries to check the consistency of the POI implementation for grafting of subgraph in case there is a transition from a spec version older than 0.0.6 towards the 0.0.6 or newer. The grafting block has a conversion of the legacy hashing algorithm to the new one, and here we test that this conversion did not change.

On order to keep the POI with the expected values, we need to make sure that anvil produces the same genesis block hence a fixed timestamp for starting it. Also both local start and remote one need to have same parameters. In addition to that we need to wait until the last checked grafted block is mined before we start the integration tests.

Moreover there is need for specifying an index parameter in the fetching POI query in order to avoid error when a local environment variable GRAPH_POI_ACCESS_TOKEN is set as that would set it to zero address and produce inconsistent POI value.

@zorancv zorancv force-pushed the zoran/poi-test branch 19 times, most recently from 5a462d9 to d215fc8 Compare April 9, 2025 22:10
@zorancv zorancv marked this pull request as ready for review April 10, 2025 14:08
@zorancv zorancv requested a review from lutter April 10, 2025 16:51
Copy link
Collaborator

@lutter lutter left a comment

Choose a reason for hiding this comment

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

Nice test! Just a few minor comments

}
}
}
let one_sec = time::Duration::from_secs(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be better as a constant

}
}
let one_sec = time::Duration::from_secs(1);
thread::sleep(one_sec);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably not a big deal here, but you shouldn't use thread::sleep in async code; use tokio::time::sleep instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For current usage is irrelevant as all tests need to wait, but if ever used elsewhere it could matter. Changing it.

@@ -796,6 +806,82 @@ async fn test_remove_then_update(ctx: TestContext) -> anyhow::Result<()> {
Ok(())
}

async fn test_subgraph_grafting(ctx: TestContext) -> anyhow::Result<()> {
async fn get_bloock_hash(block_number: i32) -> Option<String> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

type 'bloock'

// We need to make sure that the preconditions for POI are fulfiled
// namely that the anvil produces the proper block hashes for the
// blocks of which we will check the POI
assert_eq!(block_hash, block_hashes[(i - 1) as usize]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am confused by this - the comment made me think we would check anvil, but we are querying graph-node. Maybe the comment should just not mention anvil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. The 'blockchain' is more appropriate here,

@@ -0,0 +1,25 @@
specVersion: 0.0.5
description: Grafted Subgraph
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this naming confusing - 'grafted subgraph' implies that this subgraph is grafted onto something else. Maybe just calling it 'base subgraph' would be better. And what's currently called 'grafting subgraph' could then be called 'grafted subgraph', but leaving that as 'grafting subgraph' is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, those names are more intuitive.

@zorancv zorancv requested a review from lutter April 14, 2025 13:51
@zorancv zorancv merged commit bafc3dd into master Apr 15, 2025
6 checks passed
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