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

Add test checkpoint data builder #20749

Merged
merged 7 commits into from
Jan 4, 2025
Merged

Conversation

lxfind
Copy link
Contributor

@lxfind lxfind commented Dec 31, 2024

Description

This PR adds a builder that could generate arbitrary checkpoint data.
It will make it easy to test the indexer.

Test plan

Added unit tests.


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • Protocol:
  • Nodes (Validators and Full nodes):
  • gRPC:
  • JSON-RPC:
  • GraphQL:
  • CLI:
  • Rust SDK:

Copy link

vercel bot commented Dec 31, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 4, 2025 7:56pm
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Jan 4, 2025 7:56pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Jan 4, 2025 7:56pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Jan 4, 2025 7:56pm

Copy link
Contributor

@wlmyng wlmyng left a comment

Choose a reason for hiding this comment

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

I'm probably assuming a lot, but it looks like the TestCheckpointDataBuilder's purpose is to quickly generate transactions + checkpoints that are self-contained and individually correct, for the purpose of benchmarking indexer performance?

A noob question, but why not use Simulacrum? With #20729 approved, we could also generate test checkpoint data by writing test files through the transactional test runner, which would similarly allow us to generate arbitrary checkpoint data

pub fn new(checkpoint: u64) -> Self {
Self {
checkpoint,
epoch: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of defaulting to 0, thinking we should instead set it to None, and on TestCheckpointDataBuilder::build_checkpoint, if we didn't explicitly set this, then we'd error out

Otherwise, during our testing, we might neglect to set the correct epoch, resulting in strange behavior in our indexer

Perhaps a struct that wraps the checkpoint data builder, and would be solely responsible for advancing the epoch correctly

Edit: returning to this, seeing as how state isn't maintained across transactions and across checkpoints, I see that this isn't meant to be something like simulacrum to emulate a lockstep network but a way to generate txs and checkpoints in bulk

Comment on lines 146 to 226
/// Mutate an existing object in the transaction.
/// `object_idx` is a convenient representation of the object's ID.
pub fn mutate_object(mut self, object_idx: u64) -> Self {
let tx_builder = self.next_transaction.as_mut().unwrap();
let object_id = derive_object_id(object_idx);
let object = self
.object_map
.get(&object_id)
.cloned()
.expect("Mutating an object that doesn't exist");
tx_builder.mutated_objects.push(object);
self
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like this doesnt make any changes to the object, just registers it in the tx builder's mutated_objects, so i guess this would be useful when testing object mutation but we don't care what exactly, or when mocking complex object mutation behavior

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct!

Comment on lines 224 to 233
/// Complete the current transaction and add it to the checkpoint.
pub fn finish_transaction(mut self) -> Self {
let TransactionBuilder {
sender_idx,
gas,
created_objects,
mutated_objects,
deleted_objects,
events,
} = self.next_transaction.take().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

so within the checkpoint builder, state isn't preserved from one transaction to the next, we consume the contents of the transactionBuilder and finalize it into a standalone transaction

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just updated the PR to support generating multiple checkpoints.

@lxfind
Copy link
Contributor Author

lxfind commented Jan 2, 2025

This will be used to write unit tests for various handler pipelines in indexer-alt, not for benchmarking.
It would be very difficult to use Simulacrum to quickly generate various shaped checkpoint data to write unittests. The test would be very complicated and mostly around how to construct certain transactions. For instance, to delete or wrap objects, or to test events, you would have to then implement Move contracts and make proper calls to do that.

@lxfind
Copy link
Contributor Author

lxfind commented Jan 3, 2025

#20761 is a good demonstration on how much easier it becomes to test the handlers.

Copy link
Contributor

@wlmyng wlmyng left a comment

Choose a reason for hiding this comment

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

Gotcha, this looks good to me. This would indeed be nice to have for indexer unit tests, where we don't care so much about correctness from the network and more about whether we're indexing to the data or feature tables correctly

crates/sui-types/src/test_checkpoint_data_builder.rs Outdated Show resolved Hide resolved
Comment on lines +274 to +289
/// Wrap an existing object in the transaction.
/// `object_idx` is a convenient representation of the object's ID.
pub fn wrap_object(mut self, object_idx: u64) -> Self {
let tx_builder = self.checkpoint_builder.next_transaction.as_mut().unwrap();
let object_id = Self::derive_object_id(object_idx);
assert!(self.live_objects.contains_key(&object_id));
tx_builder.wrapped_objects.insert(object_id);
self
}
Copy link
Contributor

Choose a reason for hiding this comment

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

in this case, should we remove it from the live_objects set? and also update the mutated_objects set?

Copy link
Contributor

@wlmyng wlmyng Jan 3, 2025

Choose a reason for hiding this comment

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

Returning to this: agree that it makes more sense to keep the various object functions restricted to its corresponding set, i.e, wrap and unwrap_object deal with the unwrapped_objects field, and so on, then consolidate in finish_transaction

Comment on lines +423 to +436
self.live_objects
.extend(output_objects.iter().map(|o| (o.id(), o.clone())));
Copy link
Contributor

Choose a reason for hiding this comment

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

ah i see, when we unwrap, its added to output_objects, and then we extend the live_objects here

Copy link
Contributor

Choose a reason for hiding this comment

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

and this is because we derive the input_objects from live_objects, so if we do this earlier we'll erroneously add unwrapped objects

@lxfind
Copy link
Contributor Author

lxfind commented Jan 4, 2025

@wlmyng Thank you for the review!

@lxfind lxfind force-pushed the add-teset-checkpoint-data-builder branch from 7b77e36 to cbee358 Compare January 4, 2025 02:20
@lxfind lxfind temporarily deployed to sui-typescript-aws-kms-test-env January 4, 2025 02:20 — with GitHub Actions Inactive
@lxfind lxfind enabled auto-merge (squash) January 4, 2025 02:20
@lxfind lxfind disabled auto-merge January 4, 2025 19:53
@bmwill
Copy link
Contributor

bmwill commented Jan 4, 2025

What's the difference or benefit of this compared to simulacrum which already provides a way to build chain states

@wlmyng
Copy link
Contributor

wlmyng commented Jan 4, 2025

What's the difference or benefit of this compared to simulacrum which already provides a way to build chain states

Biggest thing for me over simulacrum is that while it simplifies the checkpoint and epoch creation process, there's still a lot of overhead to do even a simple transaction, and gets unwieldy when trying to emulate complex transaction effects

But perhaps we can consolidate? A simulacrum that can also mock out transaction execution .. or is that a thing already?

@lxfind
Copy link
Contributor Author

lxfind commented Jan 4, 2025

What's the difference or benefit of this compared to simulacrum which already provides a way to build chain states

It's really about how easy it is to write tests that need some form of checkpoint data.
For instance, these tests: 833c798
I initially tried to implement it using simulacrum, but gave up fairly quickly. Most of the code is to deal with transaction construction in order to emit objects in the effects that I need. I also had to write quite some Move code for it. It would become harder once we start testing pipelines that need arbitrary Move calls, events and etc.

@bmwill
Copy link
Contributor

bmwill commented Jan 4, 2025

Ah, so if i understand correctly then this doesn't do any execution but lets you just craft a txn/effect how you want to more easily create test scenarios that would otherwise take a lot of setup for?

@lxfind
Copy link
Contributor Author

lxfind commented Jan 4, 2025

Correct!

@lxfind lxfind merged commit 525f26f into main Jan 4, 2025
52 checks passed
@lxfind lxfind deleted the add-teset-checkpoint-data-builder branch January 4, 2025 23:46
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