allowing implementers to take automated action based on error_codes

[plebhash]

recently @Fi3 reached out to me pointing out that this section of the spec is too limiting:

The protocol uses string error codes. The list of error codes can differ between implementations, and thus implementations MUST NOT take any automated action(s) on the basis of an error code.

We should use this space to coordinate discussions on potential approaches to make this a bit more flexible.

[plebhash]

one alternative I proposed:

The protocol uses string error codes. Implementations MAY use error codes for automated actions. The list of error codes can differ between implementations, and therefore implementations MUST do a logging no-op for unknown error codes.

[plebhash]

proposal by @Fi3:

is very limiting maybe we should specify the error codes and add one custom or log that is only used for logging the issue and it should not be used to take automatic action. Like stale-share is specified in spec and have meaning but log-mymessage is implementation dependat and can only be used to log.

[GitGab19]

It's not clear to me the problem that we should try to solve here.

Do we want to allow implementations to take action based on a specific error code? If so, why should we impose a specific set of predefined error codes? Why not simply being less restrictive as in the alternative proposed by @plebhash in this comment?

@Fi3 can you provide a specific example (real problem) for which that proposal would not be enough?

[Fi3]

if you allow implementation to take automatic action you should at least provide an set of error codes that implementation are expected to always receive, like stale share for example. So that an implementation can decide to do something when one of them happen. Then for all the other error code (we can prefix them with log or just do not include them or whatever) an implementation should only log them, since it can not relay on the other implementation to provide that specific error code on that specfic case. One thing is to say every time that we have a stale share I know for sure that the pool is going to send the stale-share error code (this case is much easier to handle). Another think is to say the pool could provide stale-share or could not or could provide it but just for a specific case or could send another error code ecc ecc. In the latter case I maybe should not execute any automatic action that is not logging the error. I still think that allow for generic error code with a specific description provided by who is raising the error can be very useful for logging and for debugging possible issues.

[GitGab19]

In case of a stale share, the Pool sends a SubmitShares.Error (with or without the predefined error code).

If the client wants to fallback to a secondary pool (or do anything else) I think the message received is enough.

Which specific use case do you see in having a specific predefined stale-share error code here?

[Fi3]

I could accept a stale-share if is withing a certain window from a SNPH and I could decide to always fall back or whatever. The point is that if pool is not required to send stale-share when the share is staled there is no safe way for a client to do anyhting. A share could be in Error for a lot of good reasons that I may want to handle in different ways.

[Fi3]

I maybe do not want to fall back to another pool anytime that I have a SubmitShares.Error. This is only an example but if we agree to change this part of the spec we can discuss every specfic error in his own thread. And in this specific case I really do not want do fail when I have a stale share that is within a certain time window from a SNPH, and I want the pool to send this exact error for stale shares cause for every share I want to know if the pool is going to pay it or not and why

[plebhash]

@Fi3 also mentioned that SRI might be already violating the MUST NOT take automated action based on error_codes

since the refactors my detailed knowledge of the application layer is not so fresh anymore, so I'll defer to @GitGab19 + @Shourya742 to confirm whether this is currently true or not

[Fi3]

yep no time to look into it rn sorry. But if I remember well the JDC do something bases on error code that is not just logging it.

[jakubtrnka]

MUST NOT take automated action based on error_codes

Btw, I think this statement is quite silly. Nobody can prevent me from writing so smart handling that it always makes a good decision based on the complicated error code. And if I do that, how is this a violation of the specification?

Anyways,
I don't particularly like the idea of having some list of "well-known-codes" - if I understood it correctly from the discussion above.
The original authors of the spec decided to define the Reject msg with string. It looks almost as if it has been done on purpose. So I wouldn't necessarily fight against it.

To be honest, I don't understand this 👇

proposal by @Fi3:

I could accept a stale-share if is withing a certain window from a SNPH and I could decide to always fall back or whatever. The point is that if pool is not required to send stale-share when the share is staled there is no safe way for a client to do anyhting. A share could be in Error for a lot of good reasons that I may want to handle in different ways.

IMO every submit is at any time either valid or not valid. Everything after SNPH, in example above, is clearly invalid and should be labeled as stale. If rejected, the reason is perhaps not as important. So I don't see any essential use case for automated actions.

But on the other hand
Also imagine that SRI client software has beautiful and reliable handling of error codes within SRI ecosystem, but regular and boring everywhere else. Could be motivation to use SRI ;-) So maybe it's not that bad to be vague regarding error codes after all. (I don't suggest doing that)

[Fi3]

It looks almost as if it has been done on purpose. So I wouldn't necessarily fight against it.

this was something that needed to be discussed and have never been.

IMO every submit is at any time either valid or not valid. Everything after SNPH, in example above, is clearly invalid and should be labeled as stale. If rejected, the reason is perhaps not as important. So I don't see any essential use case for automated actions.

If a submit is invalid cause is stale I should not disconnect from the pool. If a submitted that have the right target is invalid for other reason it means that there is an issue with the pool with the proxy or that the pool do not want to pay the share for some reasons, I may be want to switch pool immediately or reset the connection.

I'm not arguing against what the client does with error code (also if the spec say explicitly to not do anyhting) the main point here is that some error code could be very useful to have defined in the spec so I can relay that every implementation in that particular case is going to send me that specific error code, then the action to be taken is depend by the implementation. You can achieve it having different errors message but it make things more complicated.

[Fi3]

It looks almost as if it has been done on purpose. So I wouldn't necessarily fight against it.

more about that point, if something have been done on purpose but is wrong we should still change it. Btw I remember that when I started SRI this particular point on the spec was something about which no one were very convinced and needed to be though more.

[Fi3]

This discussion originate cause there are certain error that are very generic and it could be nice to take automatic action based on error code (that as per spec should be used only for logs). For example:

/// Message used by upstream to reject [`SubmitSharesStandard`] or [`SubmitSharesExtended`].
///
/// In case the upstream is not able to immediately validate the submission, the error is sent as
/// soon as the result is known. This delayed validation can occur when a miner gets faster
/// updates about a new `prevhash` than the upstream does.
#[derive(Serialize, Deserialize, Debug, Clone)]
pub struct SubmitSharesError<'decoder> {
    /// Channel identification.
    pub channel_id: u32,
    /// Unique sequential identifier of the submit within the channel.
    pub sequence_number: u32,
    /// Rejection reason.
    ///
    /// Possible error codes:
    ///
    /// - invalid-channel-id
    /// - stale-share
    /// - difficulty-too-low
    /// - invalid-job-id
    pub error_code: Str0255<'decoder>,
}

A stale-share is something that is supposed to happen (I would be concerned if I do not see any), so I do not want to close the connection if stale shares are below a certain threshold. Where all the other error codes should lead to immediately close the connection. I think that we can find other error that are too generic.

I'm not trying to push for a spec change I just wanted to point it out. I'm more than ok in leaving the spec like that, since is not a big issue, the downstream in the specific above case can just trust upstream to close the connection when it make sense, and treat every share error that do not lead to connection being closed by upstream as a stale-share.

[plebhash]

cross referencing: stratum-mining/sv2-apps#364

[plebhash]

with stratum-mining/sv2-apps#365, now JDC is selectively avoiding fallback when it receives DeclareMiningJob.Error with error_code = stale-chain-tip

[Fi3]

nice, we need to fix this part of the spec to allow it:

and thus implementations MUST NOT take any automated action(s) on the basis of an error code