add error_code constants#2151
Conversation
GitGab19
left a comment
There was a problem hiding this comment.
I looked on the specs and reported almost on every "message" the error_code(s) that we have there, to simplify the review and being sure we're covering all we have on the specs.
That said, I think my initial understanding was to define the error codes (as constants) only for the ones which are explicitly defined in the current specs.
But reviewing this PR made me realize that probably your goal was to cover all the codes we have on the specs + everything we are already using at the application level (which very likely would be useful also for some adopters).
Is my understanding correct?
| pub const ERROR_CODE_DECLARE_MINING_JOB_MISSING_TXS: &str = "missing-txs"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX: &str = | ||
| "invalid-job-param-value-coinbase-tx"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_INPUT: &str = | ||
| "invalid-job-param-value-coinbase-tx-input"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INTERNAL_ERROR: &str = "internal-error"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_STALE_CHAIN_TIP: &str = "stale-chain-tip"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INVALID_JOB: &str = "invalid-job"; |
There was a problem hiding this comment.
Where are these in the specs? I'm looking at the JD chapter, and I only see this sentence:
Possible error codes:
- invalid-mining-job-token
- invalid-job-param-value-{} - {} is replaced by a particular field name from DeclareMiningJob message
There was a problem hiding this comment.
Aren't we missing invalid-job-param-value-version, invalid-job-param-value-wtxid_list, and invalid-job-param-value-excess-data here?
There was a problem hiding this comment.
all invalid-job-param-value-* were changed into invalid-*
if a specific parameter is invalid, we write that in a straightforward way, no templates or patterns induced by spec
| // Commonly used SetCustomMiningJobError error_code values. | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_JD_NOT_SUPPORTED: &str = "jd-not-supported"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_CHANNEL_ID: &str = "invalid-channel-id"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_MINING_JOB_TOKEN: &str = | ||
| "invalid-mining-job-token"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_JOB_NOT_YET_VALIDATED: &str = "job-not-yet-validated"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_PREV_HASH_LENGTH: &str = | ||
| "invalid-prev-hash-length"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_STALE_CHAIN_TIP: &str = "stale-chain-tip"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_NBITS: &str = | ||
| "invalid-job-param-value-nbits"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_VERSION: &str = | ||
| "invalid-job-param-value-version"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX: &str = | ||
| "invalid-job-param-value-coinbase-tx"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_VERSION: &str = | ||
| "invalid-job-param-value-coinbase-tx-version"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_PREFIX: &str = | ||
| "invalid-job-param-value-coinbase-prefix"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_INPUT_N_SEQUENCE: | ||
| &str = "invalid-job-param-value-coinbase-tx-input-n-sequence"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_OUTPUTS: &str = | ||
| "invalid-job-param-value-coinbase-tx-outputs"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_LOCKTIME: &str = | ||
| "invalid-job-param-value-coinbase-tx-locktime"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_MERKLE_PATH: &str = | ||
| "invalid-job-param-value-merkle-path"; |
There was a problem hiding this comment.
On the specs we have:
Possible errors:
- invalid-channel-id
- invalid-mining-job-token
- invalid-job-param-value-{} - {} is replaced by a particular field name from SetCustomMiningJob message
There was a problem hiding this comment.
Aren't we missing invalid-job-param-value-min-ntime?
There was a problem hiding this comment.
yes we are
I'm adding it to this PR (as invalid-min-ntime), and taking note of JDS validation gap stratum-mining/sv2-apps#500
as of today, SRI already implements this PR covers all I cannot say I introduced all I've always treated the so whenever I was faced with some edge case that the spec didn't explicitly list, I just established the |
|
I see, and I guess it makes sense. My question now is: should we have constants for all the error-codes which are currently listed on the specs? Even if they are not currently used by our implementation? |
hmm makes sense, currently this PR is not covering 100% of the changing |
3393943 to
2f9f21e
Compare
|
ok now this PR covers 100% of the |
| pub const ERROR_CODE_SUBMIT_SHARES_INVALID_SHARE: &str = "invalid-share"; | ||
| pub const ERROR_CODE_SUBMIT_SHARES_STALE_SHARE: &str = "stale-share"; | ||
| pub const ERROR_CODE_SUBMIT_SHARES_INVALID_JOB_ID: &str = "invalid-job-id"; | ||
| pub const ERROR_CODE_SUBMIT_SHARES_DIFFICULTY_TOO_LOW: &str = "too-low-difficulty"; |
There was a problem hiding this comment.
this should be difficulty-too-low (?)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| /// - invalid-channel-id | ||
| /// - stale-share | ||
| /// - difficulty-too-low | ||
| /// - too-low-difficulty |
There was a problem hiding this comment.
this should be difficulty-too-low (?)
| // Commonly used SetCustomMiningJobError error_code values. | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_JD_NOT_SUPPORTED: &str = "jd-not-supported"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_CHANNEL_ID: &str = "invalid-channel-id"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_MINING_JOB_TOKEN: &str = | ||
| "invalid-mining-job-token"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_JOB_NOT_YET_VALIDATED: &str = "job-not-yet-validated"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_PREV_HASH_LENGTH: &str = | ||
| "invalid-prev-hash-length"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_STALE_CHAIN_TIP: &str = "stale-chain-tip"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_NBITS: &str = | ||
| "invalid-job-param-value-nbits"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_VERSION: &str = | ||
| "invalid-job-param-value-version"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX: &str = | ||
| "invalid-job-param-value-coinbase-tx"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_VERSION: &str = | ||
| "invalid-job-param-value-coinbase-tx-version"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_PREFIX: &str = | ||
| "invalid-job-param-value-coinbase-prefix"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_INPUT_N_SEQUENCE: | ||
| &str = "invalid-job-param-value-coinbase-tx-input-n-sequence"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_OUTPUTS: &str = | ||
| "invalid-job-param-value-coinbase-tx-outputs"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_LOCKTIME: &str = | ||
| "invalid-job-param-value-coinbase-tx-locktime"; | ||
| pub const ERROR_CODE_SET_CUSTOM_MINING_JOB_INVALID_JOB_PARAM_VALUE_MERKLE_PATH: &str = | ||
| "invalid-job-param-value-merkle-path"; |
There was a problem hiding this comment.
Aren't we missing invalid-job-param-value-min-ntime?
| pub const ERROR_CODE_DECLARE_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX: &str = | ||
| "invalid-job-param-value-coinbase-tx"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_INPUT: &str = | ||
| "invalid-job-param-value-coinbase-tx-input"; |
There was a problem hiding this comment.
These should be coinbase-tx-prefix and coinbase-tx-suffix?
There was a problem hiding this comment.
note: I removed the invalid-job-param-value- pattern, so the error messages on this comment are
invalid-coinbase-tx
as discussed on the call, this error comes up when BitcoinCoreIPCEngine::handle_declare_mining_job fails to deserialize the coinbase tx via rust-bitcoin APIs
it's kind of a generic error because rust-bitcoin APIs don't give us much information about it
invalid-coinbase-tx-input
after the coinbase tx has been deserialized on BitcoinCoreIPCEngine::handle_declare_mining_job, we check whether it only has 1 single input
otherwise, it's not an actual coinbase tx
overall, with regards to explicit references to coinbase-tx-prefix and coinbase-tx-suffix, since we're no longer following invalid-job-param-value- pattern, I think we don't need to cover those?
tbh, I wonder if it would even be possible to cover them
| pub const ERROR_CODE_DECLARE_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_INPUT: &str = | ||
| "invalid-job-param-value-coinbase-tx-input"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INTERNAL_ERROR: &str = "internal-error"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_STALE_CHAIN_TIP: &str = "stale-chain-tip"; |
There was a problem hiding this comment.
How can we have this error code if there's not chain-tip in the message?
There was a problem hiding this comment.
this is an internal implementation detail of jd_server_sv2::job_declarator::job_validation::bitcoin_core_ipc + bitcoin_core_sv2::job_declaration_protocol::handlers
if there's a ProvideMissingTransactions round-trip, and the MempoolMirror context chain-tip drifts in the mean time, DeclareMiningJob failes with this error code
| pub const ERROR_CODE_DECLARE_MINING_JOB_MISSING_TXS: &str = "missing-txs"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX: &str = | ||
| "invalid-job-param-value-coinbase-tx"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INVALID_JOB_PARAM_VALUE_COINBASE_TX_INPUT: &str = | ||
| "invalid-job-param-value-coinbase-tx-input"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INTERNAL_ERROR: &str = "internal-error"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_STALE_CHAIN_TIP: &str = "stale-chain-tip"; | ||
| pub const ERROR_CODE_DECLARE_MINING_JOB_INVALID_JOB: &str = "invalid-job"; |
There was a problem hiding this comment.
Aren't we missing invalid-job-param-value-version, invalid-job-param-value-wtxid_list, and invalid-job-param-value-excess-data here?
bcb75de to
4eaf07c
Compare
4eaf07c to
2296dd6
Compare
|
@GitGab19 FYI since your last review I'm doing some additional changes here, which are already one step aligned with the vision described in #2149 (comment) more specifically, with 582536f I'm making sure that error but not all error variants carry a /// Variants carrying `&'static str` are intended to be used as `error_code` values in the response message.
///
/// Variants without `&'static str` SHOULD lead to a client disconnection or application
/// shutdown.
/// ...
|
close #2142
companion stratum-mining/sv2-apps#495