Issue with flags checking and related roles behaviour

[lorbax]

In this todo! we see that the check_flags panics if used in any other protocol except MininProtocol. Indeed, this method is used in IsUpstream trait, that does not have an extension (possibly empty) to the JobDeclaration protocol which is implemented for JDS
As a result, apparently there is no way for the JDS to check the bit REQUIRES_ASYNC_JOB_MINING of SetupConnection.flag in JobDeclaration protocol, making the SRI slightly no specs compliant.

1. Things to do: test with MG if different flags trigger the desired behaviour in the particular case above.
2. Check this issue affects also other parts of the code.

[pavlenex]

This one should be looked into as a priority before we push a new release.

[pavlenex]

On a dev call @xyephy agreed he would like to tackle this, to achieve this he will contact @Fi3 who will provide more details on how to tackle it. But high-level approach would be to fix Fix function for JDS and return warning for any other case.

[xyephy]

On a dev call @xyephy agreed he would like to tackle this, to achieve this he will contact @Fi3 who will provide more details on how to tackle it. But high-level approach would be to fix Fix function for JDS and return warning for any other case.

conversation on track

[Fi3]

@xyephy this function: https://github.com/stratum-mining/stratum/blob/dev/protocols/v2/subprotocols/common-messages/src/setup_connection.rs#L88

Check if the passed flag in the SetupConnection message https://github.com/stratum-mining/sv2-spec/blob/main/03-Protocol-Overview.md#361-setupconnection-client---server and the feature supported by upstream are compatible.

For example in the case of the mining protocol these are the available flags: https://github.com/stratum-mining/sv2-spec/blob/main/05-Mining-Protocol.md#531-setupconnection-flags-for-mining-protocol

So let's say that downstream require standard job 0b_0000_0000_0000_0000_0000_0000_0000_0001 but the pool do not accept them 0b_0000_0000_0000_0000_0000_0000_0000_0000 (as SetupConnection message flags not SetupConnectionSuccess flags)¹ it should return false.

As you can see this function handle only the mining protocol but also the job declaration protocol have flags. IMO this function try to be too smart. Should not try to extrapolate the meaning of the flags. It should only take 2 u32, 0s every bit without meaning (the last 29 for mining protocol the last 31 for job declaration protocol), XNOR every bit and if everything is 1 return true otherwise false. When the protocol do not define flags (for example template distribution protocol) we have 4 options:

1. return true (we are 0ing the all 32 bit and apply XNOR)
2. return false (the operation do not have any sense and we return false)
3. return an error (the operation do not have any sense and we return an error)
4. panic (the operation do no have any sense and we fail early with panic!("this protocol do not have flags"))

I would go with 4 but I guess that no one want panics in the codebase so I listed also the other options.

Footnotes

1. to make things more complicated for each protocol that have flags there are 2 kind of flags in the protocol SetupConnection flags and SetupConnectionSuccess flags.For the mining protocol, not accepting version rolling as a server is expressed with 0b_****_****_****_****_****_****_****_*0** when we are dealing with SetupConnection flags (because is the contrary of requiring version rolling).
   But not supporting version rolling can be expressed also in SetupConnectionSuccess notation as 0b_****_****_****_****_****_****_****_***1. At the end, for both, the result is that job on that connection do not have version bit changed; the semantic of the 2 flags is a little bit different the first one means that version rolling must be done, the second one means that version rolling must not be done. So we can end up in the case were no one signal an hard requirement and version rolling could be done.
   A server that do not accept version rolling will have a bitmask to apply to the SetupConnection flags like that 0b_****_****_****_****_****_****_****_*0**, and it will answer with 0b_****_****_****_****_****_****_****_***1 in SetupConnectionSuccess. As you can see SetupConnection and SetupConnectionSuccess flags are 2 different type, with different semantic. Even position of bit related to similar things are different. That to say that they must be treated separately and that THIS ISSUE IS ABOUT SetupConnection flags. In SRI these things are used only by very old code about upstream discovery, that turned out to be completely useless (not even sure if it is correct, shame on me). Btw they should be supported by the libraries, so maybe we can open other issues related to that; I don't see them as something that we should prioritize right now. ↩

[pavlenex]

@xyephy Was this enough of info for you to get started on this? We have it planned for the next release ,so would appreciate if you can let us know how you're progressing with it?

[xyephy]

@xyephy Was this enough of info for you to get started on this? We have it planned for the next release ,so would appreciate if you can let us know how you're progressing with it?

I'm finalising on the fix, the info given was helpful.

[pavlenex]

Hey @xyephy We'd like to include this one in the next minor release, which we plan to ship this week, how are you progressing with this one? Ideally we'd need time to review it as well so it can be included in 1.0.1.

[xyephy]

Hey @xyephy We'd like to include this one in the next minor release, which we plan to ship this week, how are you progressing with this one? Ideally we'd need time to review it as well so it can be included in 1.0.1.

@pavlenex I've pushed a PR #918, it can be reviewed and recommendations for any improvements are welcome. Apologies for the delay.

[plebhash]

hey @xyephy any progress here?

@Shourya742 expressed interest in helping on this, so I'll co-assign him

[Shourya742]

I have added the MG test for the flow to verify the fix. I will also be adding the corresponding connection changes in JDS to support checking the async flag. Currently, it just sends a hardcoded response without any validation. Progress can be tracked on the PR: #1035.

[plebhash]

relevant for the context of this issue: