Pool is not properly checking the SetCustomMiningJob message received

[GitGab19]

While I and @plebhash were working on #923, he made a good point, which I'm going to try to summarize here.

When JDC declares a mining job to JDS, theoretically JDS should check it, and approve/deny it. After that, JDC communicated to Pool the job it's going to be used through the SetCustomMiningJob message.

But in our current state, Pool is not checking fields contained in that message, and so a malicious JDC could try to cheat the Pool by using a wrong field (such as the prev_hash). In addiction to that, while looking at Pool handlers, I found this function which calls the check_set_custom_mining_job one, which always returns true (so doing no checks at all).

So we need to implement those checks on Pool side, in the places I linked in this description.

Please @plebhash feel free to expand if you think I missed something 🙏

[GitGab19]

Oh, a tiny detail/suggestion I missed.
In the case in which Pool is receiving a wrong SetCustomMiningJob (for example containing an old prev_hash) it should answer to JDC with a SetCustomMiningJob.Error. Also in the specs, there's a suggested error-code called invalid-job-param-value-{} which could be perfect for this: https://github.com/stratum-mining/sv2-spec/blob/main/05-Mining-Protocol.md#5320-setcustomminingjoberror-server---client

[lorbax]

As I understood the specs, the DeclareMiningJobSuccess.new_mining_job_token can be used to commit the declared job. This token has to be different then AllocateMiningJobTokenSuccess.mining_job_token (otherwise the doenstream can start mining before receiving a SetCustomMiningJobSeccess) and should contain the JDS signature, covering the declared job. When the jdc sends a setCustomMiningJob, the pool just checks the signature against the JDS pubkey.

[Shourya742]

Can take this up

[Fi3]

Pool should just verify if the token is valid:

stratum/roles/jd-server/src/lib/job_declarator/message_handler.rs

Line 116 in 6f80d04

new_mining_job_token: signed_token(

[plebhash]

Pool should just verify if the token is valid:

stratum/roles/jd-server/src/lib/job_declarator/message_handler.rs

Line 116 in 6f80d04

new_mining_job_token: signed_token(

@Fi3 these are the messages exchanged between JDC and JDS during Job Declaration:

• AllocateMiningJobToken + AllocateMiningJobToken.Success
• DeclareMiningJob + DeclareMiningJob.Success

none of these messages contain any information about prev_hash, so even though mining_job_token is valid, it shouldn't be used as the only metric for the pool to accept SetCustomMiningJob messages.

what if JDC sends a SetCustomMiningJob with a valid mining_job_token but an old prev_hash? or worse: some prev_hash that is nonsense?

that could be a bug or an attack from JDC... but either way, the pool is going to reward JDC for shares that are actually worthless

and that was the discussion I had with @GitGab19 , where we both arrived to the conclusion that a pool should only respond with a SetCustomMiningJob.Success if SetCustomMiningJob carries the correct prev_hash (which represents the current state of the network).

[Fi3]

IMO pool should not enforce the prev hash. Miners will chose on which chain to mine and be rewarded accordingly. This to lower the effectiveness of selfish mining.

[GitGab19]

IMO pool should not enforce the prev hash. Miners will chose on which chain to mine and be rewarded accordingly. This to lower the effectiveness of selfish mining.

Are you suggesting here that responsibility about checking the prev-hash is on JDS, right?