Spec change wishlist

[Trolldemorted]

We should do some cleanups to reduce the WTFs/minute.

Field renaming

• use milliseconds in CheckerTaskMessage.roundLength (literally every the other date field uses ms).
• rename CheckerTaskMessage.flagIndex and EnoLogMessage.flagIndex to something that makes sense (it also covers noise and havoc). Maybe variant, variantIndex or variantId?
• rename CheckerTaskMessage.runId to something that makes sense (the name run is used nowhere else).
• drop CheckerTaskMessage.serviceId.
• drop CheckerTaskMessage.serviceName.
• document CheckerTaskMessage.method variants ("putflag", "getflag", "putnoise", "getnoise", "havoc").
• rename ScoreboardInfoTeam.flagUrl to ScoreboardInfoTeam.countryCode (because that's what is inside the field).
• rename Team.countryflagUrl to Team.countryCode (because that's what is inside the field).
• rename Scoreboard.currentRound to Scoreboard.currentRoundId (to be consistent with Team and ServiceDetail).
• document Scoreboard time fields to always be UTC.
• document FirstBlood time fields to always be UTC.
• determine whether FirstBlood.storeDescription is used where the content should come from and maybe drop.
• create a field that is equal across a put/get/get/... series in EnoLogMessage and CheckerTaskMessage (e.g. {flag|noise|havoc}_t10_r200_v0) for kibana and checkers.
• rename CheckerTaskMessage.roundId to CheckerTaskMessage.currentRoundId to make the semantics clear

Ecosystem Consistency

• should we demand from every piece of infrastructure to adhere to the new names and no introduce confusing renamings?

[ldruschk]

I would also recommend to add documentation explaining what each of these fields actually means, what values it can contain, what values can be considered to be unique etc.

At the moment this understanding this requires knowledge of undocumented behavior of the engine, which could also potentially change

[domenukk]

• document! / specify exploit method

[domenukk]

Round length in milliseconds is useless imho, it'll never be less than a second or need that granularity. At least add an optional RoundSecs for people that craft their own json

[ldruschk]

Round length in milliseconds is useless imho, it'll never be less than a second or need that granularity. At least add an optional RoundSecs for people that craft their own json

in my opinion, the focus with spec should be a) simplicity and b) consistency. While I agree that the granularity probably won't be required, it is confusing why the timeout is in milliseconds whereas the round length is in seconds, when usually both of these values will be in roughly the same magnitude.

But under no circumstances would I agree to adding a second, conflicting roundSecs in addition to the roundLength field, as that would require additional and (imo) unnecessary logic in the checkers.

[Trolldemorted]

document! / specify exploit method

I have created a discussion pad with a proposal and sent you guys the link, plz comment! However I might keep that change out of this cleanup, since it should be a clean addition not related to the other changes.

At least add an optional RoundSecs for people that craft their own json

Can we please not bloat a communication specification with something that can be solved by dividing through 1000? This doesn't make any sense at all, especially since there are exactly 0 checkers that use the field and were used during ENOWARS.

Specs should be minimal and unambiguous, adding useless fields does not help in that matter.

[domenukk]

An API (even if it's REST) should be, first and foremost, usable. Looks like I was thinking of timeout the whole time. Maybe make that seconds instead?
Consistent and usable.
The default in the python checkerlib makes is 30 right now, I'm sure this has some fun side effects.
https://github.com/enowars/enochecker/blob/19a653ff310ccd921973ccdbe5dcc1a709255685/src/enochecker/checkerservice.py#L45

[Trolldemorted]

Maybe make that seconds instead?

Would that improve anything?

[domenukk]

Consistency with round_lenght and usability for calling it manually (timeout 30 is easier to type than 30000)

[ldruschk]

I honestly don't really care what we choose as long as it is consistent. I believe the timeout was "recently" converted to milliesconds, was there a reason for this?

[Trolldemorted]

usability for calling it manually (timeout 30 is easier to type than 30000)

does anyone really call it manually with something different from the default? o:O

was there a reason for this?

Yeah, for short stresstesting rounds it mattered, because enochecker's timeout handling would throw overzealous timeout exceptions (cc @MMunier ).

I see no point in not using millis everywhere, someone being too lazy to type 3 zeroes once upon a lifetime hardly qualifies as an improvement.

[ldruschk]

There is also a traceback returned as part of the CheckerResultMessage here: https://github.com/enowars/enochecker/blob/19a653ff310ccd921973ccdbe5dcc1a709255685/src/enochecker/checkerservice.py#L275

should this be added to the spec?

[Trolldemorted]

EnoLauncher doesn't use it and I didn't know this existed - does it contain the stack trace as a string? Could it be useful for anything?

Imho the trace should not end up in the scoreboard, and for all other intents and purposes just logging the trace to elk should suffice.

[MMunier]

rename CheckerTaskMessage.flagIndex and EnoLogMessage.flagIndex to something that makes sense (it also covers noise and havoc). Maybe variant, variantIndex or variantId?

I'd suggest calling constant field something along the lines of ChainId / TaskChain(Id).

[MMunier]

Regarding the time-units, there are 2 Possibilities imho.
Either we use the smallest time-scale we want to resolve i.e. [ms] as our Integer-Reference,
or we just default to using seconds (the SI-Unit) as a float.

[domenukk]

Internally, the checker timeouts in python should always have been floats, else it was buggy. I'm pro floats, but if we stick to milliseconds, at least fix the default in enochecker (30 millis seems overly harsh)

[domenukk]

I'd suggest calling constant field something along the lines of ChainId / TaskChain(Id).

Nitpick: all JSON styleguides I know of indicate names should be camelCase, so the first letter should be lowercased

[Savallator]

It was changed to ms because with seconds it was impossible to do stress-testing with short round times. If desired, the checkerlib can just convert it to float by dividing by 1000? For Manual Tests, the default will be enough in almost all cases anyways. The Internal timeouts used to check this are ms as well...