Store information about operators' validator status #37

uint · 2022-01-25T14:29:00Z

Closes #23

Waiting for confirmation from @alpe that this is what he wanted before moving this out of draft.

uint · 2022-01-26T08:38:17Z

contracts/tgrade-valset/src/contract.rs

+        .keys(deps.storage, None, None, Order::Ascending)
+        .collect::<StdResult<_>>()?;
+    for op in ops {
+        let active = validators.iter().any(|val| val.operator == op);


Not very efficient, but since the validators list is expected to be small, it probably doesn't matter?

Actually, if you'll look at my PR I'm fighting similar inefficiency issue: #35 (comment)

Validators amount may be up to 300, so it doesn't seem much - but this is end block. So every end block there will be loop within loop for X(<=300) validators, and in worst case there will be 3 additional loops from me. :)
I think this starts to add up.

Not necessarily disarding your implementation, just pointing that out.

I still think that all those separate loops (along with calculating validators diff at 562) could be made into more tight implementation - although probably much more complex..

ethanfrey

Maybe Mauro can explain more

ethanfrey · 2022-01-26T10:18:06Z

contracts/tgrade-valset/src/contract.rs

@@ -532,6 +538,19 @@ fn end_block(deps: DepsMut, env: Env) -> Result<Response, ContractError> {
    // calculate and store new validator set
    let (validators, auto_unjail) = calculate_validators(deps.as_ref(), &env)?;

+    // update operators list with info about whether or not they're active validators
+    let ops: Vec<_> = operators()


This is the beginning of the problem.
the validators set is bounded: (eg 100 or 300) based on config settings.
Operators list is unbounded (anyone can register, this stays forever)... it can be 10.000 easily if people register.

Do not range this

ethanfrey · 2022-01-26T10:20:12Z

contracts/tgrade-valset/src/contract.rs

+        .collect::<StdResult<_>>()?;
+    for op in ops {
+        let active = validators.iter().any(|val| val.operator == op);
+        operators().update::<_, StdError>(deps.storage, &op, |op| {


You need to use the diff in the active set to do this. We do this somewhere internally inside calculate_validators and update the weight of each operator.

At that point we can efficiently update the active flag (set it when None -> Some(), unset it when Some() -> None, do nothing for all other cases

uint · 2022-01-26T17:40:26Z

Optimized this following Ethan's suggestion (using the diff we already have to update the active_validator flags).

The CI error seems unrelated - it fails on installing cosmwasm_vm check_contract.

uint · 2022-01-27T09:04:28Z

Rebased to fix CI problems.

maurolacy · 2022-01-27T12:07:56Z

contracts/tgrade-valset/src/contract.rs

@@ -540,7 +546,27 @@ fn end_block(deps: DepsMut, env: Env) -> Result<Response, ContractError> {
    let old_validators = VALIDATORS.load(deps.storage)?;
    VALIDATORS.save(deps.storage, &validators)?;
    // determine the diff to send back to tendermint
-    let (diff, update_members) = calculate_diff(validators, old_validators);
+    let (diff, add, remove) = calculate_diff(validators, old_validators);


Alternatively, you could use diff directly below, with the convention that zero power means removal.

That way you can keep the calculate_diff signature the same, and keep rewards the distribution computation internal.

maurolacy · 2022-01-27T12:12:04Z

contracts/tgrade-valset/src/contract.rs

+    for op in add {
+        operators().update::<_, StdError>(deps.storage, &Addr::unchecked(op.addr), |op| {
+            let mut op = op.ok_or_else(|| StdError::generic_err("operator doesn't exist"))?;
+            op.active_validator = true;
+            Ok(op)
+        })?;
+    }
+    for op in remove {
+        operators().update::<_, StdError>(deps.storage, &Addr::unchecked(op), |op| {
+            let mut op = op.ok_or_else(|| StdError::generic_err("operator doesn't exist"))?;
+            op.active_validator = false;
+            Ok(op)
+        })?;
+    }


Looks good. Consider using diff directly, as mentioned above.

Ah, but you need the operator key. Makes sense.

The alternative then would be to do this update internally, as part of calculate_diff. But I think this is good / better for clarity.

maurolacy · 2022-01-27T12:16:43Z

Can you rebase from main? And we merge.

ethanfrey

Looks correct and only touches the diff which is much nicer than the first version.

Minor style comment, but nothing blocking merge

ethanfrey · 2022-01-27T12:29:48Z

contracts/tgrade-valset/src/contract.rs

@@ -540,7 +546,27 @@ fn end_block(deps: DepsMut, env: Env) -> Result<Response, ContractError> {
    let old_validators = VALIDATORS.load(deps.storage)?;
    VALIDATORS.save(deps.storage, &validators)?;
    // determine the diff to send back to tendermint
-    let (diff, update_members) = calculate_diff(validators, old_validators);
+    let (diff, add, remove) = calculate_diff(validators, old_validators);
+    let update_members = RewardsDistribution::UpdateMembers {


Hmm... Why not just return this as it was before (same function signature for calculate_diff ) and iterate over
for op in update_members.add for example? It should avoid a clone even.

We'd end up in this situation:
A function called calculate_diff actually calculates the rewards distribution msg for use with a specific contract's interface, and then we use that message to get back the diff for another use.

I'd rather make calculate_diff do what it advertises in the first place. Feels less convoluted.

As for the cloning, I think we'd still end up cloning those strings (Addr::unchecked does that when it's constructed from a &str), except they'd be cloned implicitly rather than explicitly. I could be missing something.

Makes sense, i.e. moving the rewards distribution out of calculate_diff is a good thing:tm:. Let's merge as it is.

uint · 2022-01-27T12:30:53Z

Can you rebase from main? And we merge.

Done!

Store information about operators' validator status

uint commented Jan 26, 2022

View reviewed changes

ethanfrey reviewed Jan 26, 2022

View reviewed changes

uint requested a review from ueco-jb January 26, 2022 17:38

uint marked this pull request as ready for review January 26, 2022 17:38

uint requested a review from ethanfrey January 26, 2022 17:38

uint force-pushed the 23-validator-status branch from e0f4667 to f6269bd Compare January 27, 2022 09:03

uint force-pushed the 23-validator-status branch from f6269bd to b8b95ad Compare January 27, 2022 09:13

uint requested review from hashedone and maurolacy January 27, 2022 11:03

maurolacy approved these changes Jan 27, 2022

View reviewed changes

uint added 4 commits January 27, 2022 13:30

valset: keep validator status with operator info

48be09b

valset: test active_validator field in operator info

919d05e

lints

e09bbee

valset: optimize marking operators as active val

41d7206

uint force-pushed the 23-validator-status branch from b8b95ad to 41d7206 Compare January 27, 2022 12:30

ethanfrey approved these changes Jan 27, 2022

View reviewed changes

maurolacy merged commit 05e0c1d into main Jan 27, 2022

maurolacy deleted the 23-validator-status branch January 27, 2022 12:57

maurolacy added a commit that referenced this pull request Jan 27, 2022

Merge pull request #37 from confio/23-validator-status

52cd173

Store information about operators' validator status

uint added breaking Breaking changes (API or State) breaking-state Storage breaking: Requires migration labels Feb 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store information about operators' validator status #37

Store information about operators' validator status #37

uint commented Jan 25, 2022

uint Jan 26, 2022

ueco-jb Jan 26, 2022

ethanfrey left a comment

ethanfrey Jan 26, 2022

ethanfrey Jan 26, 2022

uint commented Jan 26, 2022 •

edited

Loading

uint commented Jan 27, 2022

maurolacy Jan 27, 2022 •

edited

Loading

maurolacy Jan 27, 2022

maurolacy Jan 27, 2022

maurolacy Jan 27, 2022

maurolacy commented Jan 27, 2022

ethanfrey left a comment

ethanfrey Jan 27, 2022

maurolacy Jan 27, 2022

uint Jan 27, 2022 •

edited

Loading

maurolacy Jan 27, 2022

uint commented Jan 27, 2022

Store information about operators' validator status #37

Store information about operators' validator status #37

Conversation

uint commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uint commented Jan 26, 2022 • edited Loading

uint commented Jan 27, 2022

maurolacy Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy commented Jan 27, 2022

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uint Jan 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uint commented Jan 27, 2022

uint commented Jan 26, 2022 •

edited

Loading

maurolacy Jan 27, 2022 •

edited

Loading

uint Jan 27, 2022 •

edited

Loading