Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New feature for the Message Generator #1059

Closed
lorbax opened this issue Jul 17, 2024 · 10 comments
Closed

New feature for the Message Generator #1059

lorbax opened this issue Jul 17, 2024 · 10 comments
Assignees

Comments

@lorbax
Copy link
Collaborator

lorbax commented Jul 17, 2024

In the Message Generator, an action is an operation that is performed against the tested software. An action sends a list of messages (that may be empty) and perform a list of checks (that may be empty) on the response message from the tested software. These checks are called action_results. One example of action result is MatchMessageType, which checks that the received message has a specific type. For example, if we are testing a certain upstream, a typical action sends a SetupConnection and checks that the response has the message type corresponding to SetupConnectionSuccess.
There are some cases in which the tested software can answer with two or more allowed messages; for example, after a NewExtendedMiningJob, a downstream can send either a SubmitSharesExtended or an Updatechannel. The reception of any of these messages fall within the correct behaviour of a SRI compliant software.
Nevertheless, there is no way right now to check this situation. If we set an action that sends NewExtendedMiningJob and waits for an UpdateChannel the test will fail id receives a SubmitSharesExtended. Similarly, If we set an action that sends NewExtendedMiningJob and waits for an SubmitSharesExtended the test will fail id receives a UpdateChannel.
This issue presents itself in this during the checking of the result of this action.

It is needed a feature that signal that a result that checks a received message has to be performed either at the first message received, or at the first message received of a certain type (i.e. waits until the reception of a specific message, and exit with a success when this happens).

I am already working on this issue.

@GitGab19 GitGab19 assigned GitGab19 and lorbax and unassigned GitGab19 Jul 17, 2024
@GitGab19 GitGab19 added this to the 1.0.2 milestone Jul 17, 2024
@GitGab19 GitGab19 moved this from Todo 📝 to In Progress 🏗️ in SV2 Roadmap 🛣️ Jul 17, 2024
@GitGab19 GitGab19 linked a pull request Jul 19, 2024 that will close this issue
@plebhash
Copy link
Collaborator

this is great!

we get many MG CI false alarms from this missing feature, as it was already described here: #928 (comment)

EXECUTING ../../test/message-generator/test/translation-proxy/translation-proxy.json
...
WRONG MESSAGE TYPE expected: 27 received: 22

I'm excited to bring one more enhancement to our MG test stack!

@jbesraa
Copy link
Contributor

jbesraa commented Jul 21, 2024

Nevertheless, there is no way right now to check this situation. If we set an action that sends NewExtendedMiningJob and waits for an UpdateChannel the test will fail id receives a SubmitSharesExtended.

If SubmitSharesExtended always happen before UpdateChannel why would you test UpdateChannel only? why not test that first you receive SubmitSharesExtended and then UpdateChannel? even if you just wanna test things only inside UpdateChannel, you would need to validate that you first got SubmitSharesExtended right? aint that possible with MG tests?

@plebhash
Copy link
Collaborator

If SubmitSharesExtended always happen before UpdateChannel

why would you take this as a reliable assumption? there's absolutely no guarantee this is true, and that's kind of the point.

after upstream sends NewExtendedMiningJob, the next message from downstream could be:

  • SubmitSharesExtended if a new share is found
  • UpdateChannel if the nominal hashrate changed

also keep in mind that this is just an example illustrating one possible scenario, and you might be missing the forest for the trees by fixating on this specific sequence of messages

the main point is that we want a MG feature that allows for waiting for one specific message type... if other messsages come along, they are ignored, until finally the expected message is received

@Fi3
Copy link
Collaborator

Fi3 commented Jul 23, 2024

A possible solution could be change MatchMessageType(u8) into MatchMessageType(Vec<u8>) and fail if the message type is not in the vec. An advantage of it is that it do not add complexity with a new message and can still be backward compatible if in the parser we transform u8 into -> [u8] and [u8] stay [u8] so you can use both a single byte or an array. A disadvantage is that is very specific, for example what you do if you have 2 or more possible messages and you want to MatchMessageField? Another approach that would still be backward compatible and a lot more powerful but will generate more verbose tests is to change Action instead of ActionResult. Right now Action have a field called result containing a vector of ActionResult and every ActionResult in that field must be true, we need a way to add mutually exclusive ActionResutl; the most straightforward way IMO is to change Action.result from Vec<ActionResult> into Vec<Vec<ActionResult>>

We have to understand if we only need mutually exclusive MatchMessageType or if we need also other ActionResults if not I would go with the first solution (or something similar) if yes I would go with the second solution (or something similar)

@plebhash
Copy link
Collaborator

plebhash commented Jul 23, 2024

thanks for this insight @Fi3

We have to understand if we only need mutually exclusive MatchMessageType or if we need also other ActionResults if not I would go with the first solution (or something similar) if yes I would go with the second solution (or something similar)

both solutions would imply that we drop #1068 and restart the new feature from scratch. It would be painful to drop the work done on #1068 but I'm not necessarily against it, if it means that we get to do things the "right way" that also scale better in the future.

and with that in mind, I would go for the most powerful and future-proof solution: turn Action.result into Vec<Vec<ActionResult>>

that would allow us to also have similar functionality for multiple types of result, such as:

  • match_message_type
  • match_message_field
  • match_message_len
  • match_extension_type

@jbesraa
Copy link
Contributor

jbesraa commented Jul 23, 2024

If SubmitSharesExtended always happen before UpdateChannel

why would you take this as a reliable assumption? there's absolutely no guarantee this is true, and that's kind of the point.

after upstream sends NewExtendedMiningJob, the next message from downstream could be:

* `SubmitSharesExtended` if a new share is found

* `UpdateChannel` if the nominal hashrate changed

also keep in mind that this is just an example illustrating one possible scenario, and you might be missing the forest for the trees by fixating on this specific sequence of messages

the main point is that we want a MG feature that allows for waiting for one specific message type... if other messsages come along, they are ignored, until finally the expected message is received

Yea that was just an assumption based on the main issue comment by lorbax.

Anyhow, what @Fi3 is suggesting makes a lot of sense. I would also encourage looking into the best solution here.

@lorbax
Copy link
Collaborator Author

lorbax commented Jul 23, 2024

A possible solution could be change MatchMessageType(u8) into MatchMessageType(Vec<u8>) and fail if the message type is not in the vec. An advantage of it is that it do not add complexity with a new message and can still be backward compatible if in the parser we transform u8 into -> [u8] and [u8] stay [u8] so you can use both a single byte or an array. A disadvantage is that is very specific, for example what you do if you have 2 or more possible messages and you want to MatchMessageField? Another approach that would still be backward compatible and a lot more powerful but will generate more verbose tests is to change Action instead of ActionResult. Right now Action have a field called result containing a vector of ActionResult and every ActionResult in that field must be true, we need a way to add mutually exclusive ActionResutl; the most straightforward way IMO is to change Action.result from Vec<ActionResult> into Vec<Vec<ActionResult>>

We have to understand if we only need mutually exclusive MatchMessageType or if we need also other ActionResults if not I would go with the first solution (or something similar) if yes I would go with the second solution (or something similar)

Can you describe how would it be used a mutually exclusive ActionResult? It seems that in this way you cannot test situations in which two messages types can both being received.

@Fi3
Copy link
Collaborator

Fi3 commented Jul 23, 2024

You change the ActionResult type so is MatchMessageType(Vec<u8>) instead of MatchMessageType(u8) and fail if the message type is not in the vec. So if you have [8,9] you pass if the message type received is 8 or is 9

@lorbax
Copy link
Collaborator Author

lorbax commented Jul 23, 2024

You change the ActionResult type so is MatchMessageType(Vec<u8>) instead of MatchMessageType(u8) and fail if the message type is not in the vec.

I wasn't specific enough. I didn't understand the second approach, can you provide a use case?

For the first approach, you mean to have a success whenever you have a message type in the list given to the MG.
How would you manage when you want to check that a message of a certain type is received eventually? Following you proposal, you should write MatchMessageType(vec![number]), but this should be reserved for the behavior "check that the first message is of a certain type".
I think that if you want to do that you have to add a number in the Vec inside MatchMessageType which does not represent any SV2 message, so that the MG is forced to check the message type of the first entry of the Vec.
Feasible, but sounds tricky.

@Fi3
Copy link
Collaborator

Fi3 commented Jul 23, 2024

Yep you are right the solution that I proposed check if one statement between a set of them is true. But what we need here is to check that one statement must between a set of them must be true and the other are optional.

@pavlenex pavlenex removed this from the 1.0.2 milestone Jul 30, 2024
@pavlenex pavlenex closed this as not planned Won't fix, can't repro, duplicate, stale Aug 16, 2024
@github-project-automation github-project-automation bot moved this from In Progress 🏗️ to Done ✅ in SV2 Roadmap 🛣️ Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants