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 more conflict types to ConflictFinder #238

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

grasphoper
Copy link

@grasphoper grasphoper commented Nov 5, 2024

📝 Summary

Add more conflict types to ConflictFinder as per #226

A brief overview of the changes:

  • I added multiple new mapping entries to ConflictFinder, namely
    group_balance_reads: HashMap<Address, Vec<usize>>,
    group_balance_writes: HashMap<Address, Vec<usize>>,
    group_contract_creations: HashMap<Address, Vec<usize>>,
    group_contract_destructions: HashMap<Address, Vec<usize>>,
  • Added conflicts that can be related to those fields
  • Added combine_groups - a utility fn to reuse code
  • Added ConflictFinder.add_group_to_index and ConflictFinder.remove_group_from_index to reuse code and improve readability; index here means search index, or different member mappings of ConflictFinder

✅ I have completed the following steps:

  • Run make lint
  • Run make test
  • Added tests (if applicable)

@grasphoper
Copy link
Author

For anyone reviewing, please take a look at TODO comments I left and let me know what you think!

@ZanCorDX
Copy link
Contributor

ZanCorDX commented Nov 7, 2024

The original code had some copy/paste (our bad!) but you may avoid it.
Example:
on remove_group_from_index you can create a fn like:

    fn remove_group_from_map<K: std::cmp::Eq + std::hash::Hash + Clone>(
        group_id: usize,
        key_to_group: &mut HashMap<K, Vec<usize>>,
        keys: &Vec<K>,
    ) {
        for key in keys {
            let groups = key_to_group.entry(key.clone()).or_default();
            if let Some(idx) = groups.iter().position(|el| *el == group_id) {
                groups.swap_remove(idx);
            }
        }
    }

And just call it 6 times.
The same goes for add (maybe somewhere else?).

@ZanCorDX
Copy link
Contributor

ZanCorDX commented Nov 7, 2024

About the TODO ideas:
1- We could change HashMap to BTreeMap ensuring that SlotKey's ordering compares Address first. Posible con: BTreeMap could be expensive.
2- Use nested HashMaps. The first level for address and the second level for key.
This way we don't get redundant structures.

@grasphoper
Copy link
Author

@ZanCorDX thank you for all your suggestions. I implemented them and just pushed updated code! Went with a 2-layer HashMap as the "address conflict checking" solution.

@grasphoper
Copy link
Author

Hi @ZanCorDX . Any more changes you would want to this?

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