-
Notifications
You must be signed in to change notification settings - Fork 141
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
WaitUntil feature on MG #1068
WaitUntil feature on MG #1068
Conversation
Bencher
🚨 1 ALERT: Threshold Boundary Limit exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 2 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
🚨 6 ALERTS: Threshold Boundary Limits exceeded!
Click to view all benchmark results
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
1466f93
to
bbfdc23
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I left some suggestion for small improvements.
It's also worth noting that the current implementation of this new feature introduces the possibility of MG entering an infinite loop, which is a potential footgun.
That would happen if some MG test defined a match_message_type
with a "condition": "WaitUntil"
but the expected message never arrived.
In that scenario, Executor::execute
would enter an infinite loop and the test would never return TEST FAILED
nor TEST OK
.
So maybe in the future we might realize we need something like this:
pub enum Condition {
WaitUntil(u32), // timeout
...
}
where the u32
represents a timeout
that breaks the infinite loop inside Executor::execute
but for now, I think this is good to merge so we can unblock the other work that depends on it
you are right I thought that there was a timer for the entire MG test. BTW this should be a nice-to-have and an easy fix |
you're right @lorbax this is documented on the MG stratum/utils/message-generator/README.md Lines 281 to 294 in 7841021
so if some MG test (or mock) has the following conditions:
then eventually the original but if some MG test (or mock) has the following conditions:
then the test would get stuck. however, this can be easily solved with good documentation, so that the user is aware that here's a commit where I add the docs for this new feature, feel free to cherry pick it into your branch so it's included in this PR: plebhash@2be0191 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we also add this for other related ActionResults? I tested it locally and it looks good. I agree with @plebhash's view on having a timeout for this action due to the possibility of staleness.
let message: Sv2Frame<AnyMessage<'static>, _> = | ||
message.try_into().unwrap(); | ||
debug!("RECV {:#?}", message); | ||
let header = message.get_header().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let header = message.get_header().unwrap(); | |
let header = match sv2_frame.get_header() { | |
Some(hdr) => hdr, | |
None => { | |
error!("Failed to get header from Sv2Frame"); | |
success = false; | |
break; | |
} | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't know if I can commit this, the get_header()
produces an Option<T>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lorbax can you elaborate? this match
works exactly on a Option
, what's wrong with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added this part of code
This timeout seems to be valid only for commands, while we are talking about a timeout for the action. I proposed also a global timeout, longer then all the other timeouts. |
I think that it make sense with |
Pushed a commit for applying the review. |
b957b3b
to
ac836f4
Compare
@lorbax we need docs for this new feature, please add something to as already stated, I added some to plebhash@2be0191 you can either cherry-pick this commit, or just use it as inspiration to write something on your own words |
404b694
to
4036214
Compare
new feature: it is possible to add the condition "WaitUntil" to a `MatchMessageType` action result. The condition idea is to wait until the desired message arrives. In the condition config there is a timeout, that avoids the MG test getting stuck. There is also a list of allowed messages. If the test awaits a certain message, then all the other messages arrived must belong to this user specified list of allowed messages. This config is passed to the MG in the json file. A `MatchMessageField` with the `WaitUntil` condition has the following shape: ```json "results": [ { "type": "match_message_type", "value": "0x1b", "condition": {"WaitUntil": {"WaitUntilConfig": {"timeout": 120, "allowed_messages": ["0x1b", "0x16"] } } } } ], ``` This action ignores a message with id `0x16`, fails if arrives a message with id `0x17` (for example) and succeeds if arrive the message with desired id `0x1b`. Add README notes for WaitUntil feature
fix to issue 1059.
To see a description of the changes apported to this PR, look at the commit message and the README changes.
The feature introduced is backward compatible, as it is introduced optionally.
How to test it:
easy way. look at
translation-proxy.json
MG test. YOu can see that it is launched TProxy. Go the config used and change it in order that thechannel_nominal_hashrate
is few magnitude order higher than themin_individual_miner_hashrate
. This forces the channel to be updated (possibly more than once) and anUpdateChannel
. Then you should seeRECEIVED 22, WAITING MESSAGE TYPE 27
several times (i.e.UpdateChannel
) and finallyMATCHED WAITED MESSAGE TYPE 27
, namelySubmitSharesExtended
, resulting in success of the MG test. Reducing theminer_num_submits_before_update
to 1 may help testing. The hashrates depend on your hardware, on my PC this workedhard way. test PR 1001: checkit out and rebase it on top of the current PR. Then test it following instructions in the two top commits. Finally change this MG-test with the following