fix(iota-benchmark): fix test_upgrade_compatibility

[bingyanglin]

Description of change

Now our min and max version of ProtocolVersion are both 1. The old logic needs to go to max version - 1 and upgrade to max version, which does not work because there is no version 0. In the testing we test the upgrade from 1 to 2 then it works.

Links to any relevant issues

Part of #3321

Type of change

Choose a type of change, and delete any options that are not relevant.

• Bug fix (a non-breaking change which fixes an issue)

How the change has been tested

Tested pass locally by

./scripts/simtest/cargo-simtest simtest \
  --color always \
  --package iota-benchmark --test simtest -- test::test_upgrade_compatibility

Change checklist

• I have followed the contribution guidelines for this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• I have added tests that prove my fix is effective or that my feature works
• I have checked that new and existing unit tests pass locally with my changes

[muXxer]

So basically because we don't have a second bytecode snapshot which can be loaded right now, we never test this logic, right?
Should we add a dummy one for the simtests, or should we just rely on working code, because the moment we create a second bytecode snapshot, we can fix the logic anyway in that future version, in case it is broken? @alexsporn

[alexsporn]

@bingyanglin use MAX_ALLOWED and it will handle the +1 for sim tests

[alexsporn]

So basically because we don't have a second bytecode snapshot which can be loaded right now, we never test this logic, right? Should we add a dummy one for the simtests, or should we just rely on working code, because the moment we create a second bytecode snapshot, we can fix the logic anyway in that future version, in case it is broken? @alexsporn

Yes, no need to test snapshot loading now. We can revisit this once we have a protocol version 2 in place

[muXxer]

Does this work as well? Then we don't have to remove the +1 in the future.

Suggested change

	let max_ver = ProtocolVersion::MAX.as_u64() + 1;
	let max_ver = ProtocolVersion::MAX_ALLOWED.as_u64();

[bingyanglin]

Under testing

[bingyanglin]

Unfortunately it does not pass.

[bingyanglin]

@muXxer @alexsporn I changed the MAX_ALLOWED to be MAX + 1 to pass the upgrade testing in 60e3a3e.

Note that SUI also uses

    #[cfg(msim)]
    pub const MAX_ALLOWED: Self = Self(MAX_PROTOCOL_VERSION + 1);

[muXxer]

NOOO that breaks another test I fixed by doing +4. 😢

[bingyanglin]

@muXxer got it. Sorry for that. Then how about we use MAX + 1 inside test_upgrade_compatibility now and add TODO comments, so we can remember to modify it when we have really version 2? Also we keep MAX_ALLOWED to be +4.

[muXxer]

btw, where did you see it doesn't work? The CI was green for this test, or am I confused?

[bingyanglin]

Or I am fine with simply ignore it for now.

[bingyanglin]

As discussed with @muXxer in the call we decided to use MAX + 1 for now and add TODO comments at 24ad1a9

[bingyanglin]

btw, where did you see it doesn't work? The CI was green for this test, or am I confused?

Sorry just saw this message. It was lagged because I didn't refresh this page. It failed twice in my machine. I reran the sim-testing GA for the commit to see if I missed something.