StakingSudo::ProcessQueue is still necessary, despite deprecation warnings.

[AmitPr]

If I do not call

app.sudo(SudoMsg::Staking(StakingSudo::ProcessQueue {})).unwrap();

After updating the block time, when I have an unbonding that should be disbursed:

app.update_block(|block| {
    block.time = block.time.plus_seconds(60);
});

Then the unstaked tokens do not get distributed to the address that unbonded them.

This is despite the deprecation message:

use of deprecated variant `cw_multi_test::StakingSudo::ProcessQueue`: This is not needed anymore. Just call `update_block`

[maurolacy]

You should update the block height as well, isn't?

Perhaps updating the block's time alone should be removed from the multi-test API, btw. To advance the block time without changing the block height is not a sensible operation for a real blockchain. That, or make the block time update update the block height as well. And vice versa.

[DariuszDepta]

Hi @AmitPr, where can I take a look at your test case? I would like to reproduce this locally.

[DariuszDepta]

Hi @maurolacy, could you (when you have time, of course) just list (in bullet points) how to simulate this situation in test? I have checked the code and it should work properly when the block height is modified, but it would be better to have a test for this. Thanks in advance!

[maurolacy]

The point is, that in the OP's example he was modifying only the block's time, not the block's height. And that shouldn't be a valid operation, as it's not possible to do that in a real (PoS) blockchain setting.

In fact the only meaningful operation would be something like advance_block (i.e. next_block stuff), in which both the height and time are modified (increased).

[maurolacy]

Not sure how to improve on this. But letting people play with the block freely during block updating seems to be confusing.

[DariuszDepta]

I am closing this issue, but filed a dedicated task to cover this case: #136

[AmitPr]

Hi @DariuszDepta, @maurolacy,

Sorry for the very late update. Have just finally gotten around to figuring out why the behavior differs. Here's my testcase:

    let (mut app, a) = setup_env();
    do_stake(&mut app, 200u128).unwrap();
    app.update_block(|block| {
        block.height += 1;
        block.time = block.time.plus_seconds(86400);
    });
    app.sudo(SudoMsg::Staking(StakingSudo::ProcessQueue {}))
         .unwrap();
    do_unstake(&mut app, 100u128).unwrap();
    app.update_block(|block| {
        block.height += 1;
        block.time = block.time.plus_seconds(60);
    });
    app.sudo(SudoMsg::Staking(StakingSudo::ProcessQueue {}))
        .unwrap();

As you can see, I'm updating the block time, and then triggering the staking module operations.

In the update_block function, you can see the following order of operations:

    // this let's use use "next block" steps that add eg. one height and 5 seconds
    pub fn update_block<F: Fn(&mut BlockInfo)>(&mut self, action: F) {
        self.router
            .staking
            .process_queue(&self.api, &mut self.storage, &self.router, &self.block)
            .unwrap();
        action(&mut self.block);
    }

Meaning that the staking module's operations are triggered before the block is updated. Not sure whether there's a reason for this order of operations, but this seems pretty unintuitive -- Wouldn't we expect that the staking operations happen after the block is updated, simulating logic running in the BeginBlocker?

[maurolacy]

Yes, changing the order looks like a good idea. It'll be compatible with the previous behaviour as well.