feat(ethexe-consensus): squash transitions by actor #4772
Conversation
…edundant state transitions
|
Review changes with Changed Files
|
grishasobol
left a comment
grishasobol
left a comment
There was a problem hiding this comment.
do you think equal transitions is not super rare case? I thought that optimisation is something like
program transitions: H1 -> H2, H2 -> H3, H3 -> H4
optimized: H1 -> H4
totally so vec<(pid, transition)> won't have pid duplicates |
ukint-vs
changed the title
ethexe/consensus/src/mock.rs
Outdated
| ); | ||
|
|
||
| let mut transitions = [cc1.transitions, cc2.transitions].concat(); | ||
| transitions.sort_by_key(|t| t.actor_id); |
ethexe/consensus/src/utils.rs
Outdated
| } | ||
|
|
||
| // Collect all transitions in true chronological order (oldest -> newest) | ||
| let all_transitions: Vec<StateTransition> = transitions.into_iter().rev().flatten().collect(); |
There was a problem hiding this comment.
why aren't they from the very beginning in correct oldest -> newest order?
ethexe/consensus/src/utils.rs
Outdated
| // Group transitions by actor_id and squash consecutive transitions for the same actor | ||
| let mut actor_transitions: std::collections::BTreeMap<ActorId, Vec<StateTransition>> = | ||
| std::collections::BTreeMap::new(); | ||
| for transition in all_transitions { |
There was a problem hiding this comment.
you may avoid at least one allocation for vec of all_transitions var if you will be inserting the values in map in the same time
There was a problem hiding this comment.
or even mutate in place (instead of collecting vec of transitions belonging to specific actor), so you will only have vec -> map -> into_vec()
ethexe/consensus/src/utils.rs
Outdated
| } | ||
|
|
||
| // Pick the newest transition | ||
| let mut squashed = transitions_for_actor.last().unwrap().clone(); |
There was a problem hiding this comment.
I'd add in debug assert check for choronological order:
A -> B -> C -> D etc
ethexe/consensus/src/utils.rs
Outdated
| squashed.inheritor = inheritor; | ||
| } | ||
|
|
||
| squashed.messages = all_messages; |
There was a problem hiding this comment.
you may just create a new value instead of some mutations where it's possible to make a mistake in future extensions
There was a problem hiding this comment.
e.g. forget about new field in transition
ethexe/consensus/src/utils.rs
Outdated
|
|
||
| // Messages should be accumulated: [msg1, msg2] | ||
| assert_eq!(squashed.messages.len(), 2); | ||
| assert!(squashed.messages.contains(&msg1)); |
There was a problem hiding this comment.
you actually don't have any order assertions here.
I'd suggest to go with 1) easier naming: why block2 sends message1 etc; 2) provide full exact assertions
There was a problem hiding this comment.
so you may have just 1, but much more useful test instead of 4-5 of them
|
I'm also wondering why this action is performed only by participants, but not by block producer? he may want to specify if they're (validators) gonna have squashed commitment for this time etc?? |
Resolves #4744.
introduce the full squashed-commitment flow to the consensus. The core addition is a new
aggregate_chain_commitmentpipeline plus supporting changes across mocks, validators, and tests.This let producers and participants work with the compact, per-actor representation needed by the router contract.
aggregate_chain_commitment:@gear-tech/dev