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

feat: resolve issue91 to add get_max_parallel_group #96

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

mvmt-ninja
Copy link

@mvmt-ninja mvmt-ninja commented Dec 14, 2023

Pull Request

Description

@l-monninger
Copy link
Collaborator

This is off to the correct start. A few things...

  1. Use set operations to reduce complexity of checking intersections. Just make sure we have determinism.
  2. OPINION: something like this is a little more readable than the loop breaking approach you took.
impl VerifiedExecutableBlock {

    pub fn new(transactions: Vec<VerifiedExecutableTransaction>) -> Self {
        Self(transactions)
    }

    pub fn get_max_parallel_groups(&self) -> VerifiedExecutableExecutionGroups {
        let mut groups: Vec<Vec<VerifiedExecutableTransaction>> = Vec::new();
        let mut objects_in_groups: Vec<HashSet<(ObjectID, bool)>> = Vec::new();
        let mut processed_digests: HashSet<SenderSignedDataDigest> = HashSet::new();

        for executable_transaction in &self.0 {
            let sender_signed_data = executable_transaction.clone().into_message();

            // Skip if transaction is already processed
            if processed_digests.contains(&sender_signed_data.full_message_digest()) {
                continue;
            }

            let TransactionData::V1(tx_data_v1) = sender_signed_data.transaction_data();
            let shared_objects = tx_data_v1.shared_input_objects();

            // Find a group where the transaction can be added without conflict
            let mut group_id = 0;
            while group_id < objects_in_groups.len() {
                let is_conflict = shared_objects.iter().any(|obj| {
                    objects_in_groups[group_id].contains(&(obj.id, true)) && obj.mutable
                });

                if !is_conflict {
                    break;
                }
                group_id += 1;
            }

            // If no suitable group, create a new one
            if group_id == objects_in_groups.len() {
                groups.push(Vec::new());
                objects_in_groups.push(HashSet::new());
            }

            // Add the transaction to the group
            for obj in shared_objects {
                objects_in_groups[group_id].insert((obj.id, obj.mutable));
            }
            groups[group_id].push(executable_transaction.clone());
            processed_digests.insert(sender_signed_data.full_message_digest());
        }

        VerifiedExecutableExecutionGroups(groups)
    }
}
  1. Let's start writing tests. I will help, but I'll need you to give me access to your fork. Going forward, you can just open a new feature branch for things like this on which we will collaborate.
  2. I'm not sure if we need to use input_objects instead of shared_input_objects. I think it's simply invalid to try and use the same owned InputObject more than once (outside of a PTB). Do you have clarity here?

@mvmt-ninja
Copy link
Author

mvmt-ninja commented Dec 15, 2023

Looks great. 👍

  1. Yes, I think so. Probably, for input_objects, the rule will work as same as for shared_input_objects. (i.e. There are owned objects of a signer in transactions. It may not agree to have same mutable owned objects in the transactions that are in the same group.)

Furthermore, what about the transactions with have the same signer? Can they go in the same group?

@l-monninger l-monninger changed the title feat: resolve issue91 to add get_max_paralletl_group feat: resolve issue91 to add get_max_parallel_group Dec 15, 2023
@l-monninger
Copy link
Collaborator

So, I think there's a good chance this can remain the way you've written it--i.e., using shared_input_objects. But, we need to confirm this. This can be a research task approached alongside this implementation, as it'll be a relatively easy change to make later.

More precisely, I think the complexity we need to try and figure out is whether using the same owned InputObject twice would in any way be valid. I think what should be happening is that owned InputObjects are essentially only ever going to be used once. They can be reused in a PTB; but, they can't be reused across PTBs because you should instead have a different object version, I think--or perhaps even just an entirely different object in some cases. I believe owned objects are to some degree exempt from the typical versioning scheme precisely because they are owned.

What I believe this would mean is...

  1. It's okay to just check shared_input_objects.
  2. Any second use of an owned InputObject is simply invalid.
    a. We need to check whether this will just cause the execution to fail in execution_transaction_to_effects. Or, whether there's something we should do beforehand.

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