stratum: reject low-diff shares before spending a RandomX hash on them

SubmitBlock validated in the wrong order. After two O(1) length checks it
went straight to CheckRandomXSolution() -- a full ~65ms
randomx_calculate_hash -- and only afterwards tested the share target. So
32 arbitrary bytes from any peer bought a RandomX hash, and the cost was
paid in three bad places at once: on the shared HTTP/RPC libevent thread
(stratum uses EventBase(), the same base ThreadHTTP dispatches, and RPC
replies are posted back onto it), inside the read loop that holds
cs_stratum so BlockWatcher cannot push new work to real miners, and
holding the global cs_randomx_validator that block validation also takes.

One connection writing submit lines pins that thread indefinitely.

Move the share-target test above the RandomX verify. CBlockHeader::
GetHash() is SerializeHash over the header including nSolution, so
meeting the target still requires real SHA256d grinding -- an attacker
now pays for the hash instead of the node. Semantics are unchanged,
including that an empty local_diff parses to zero and still rejects; only
the position moved, and the diagnostics are recomputed locally since the
old message used variables declared further down.

This does not make the submit path cheap, only bounded: at the default
share target the grind is small, so a submit rate limit and moving
SubmitBlock off the event loop are both still wanted.

Also add the authorization check every sibling handler has and
mining.submit lacked. Being explicit about what that is worth:
mining.authorize validates no credential, so the gate is parity and
handshake-ordering, not authentication. The reorder above is the part
that actually bounds an unknown peer.

src/stratum.cpp

@@ -656,9 +656,18 @@ void CustomizeWork(const StratumClient& client, const StratumWork& current_work,
             LogPrint("stratum", "%s\n", msg);
             throw std::runtime_error(msg);
         }
-        if (cb.vout[0].scriptPubKey == (CScript() << OP_FALSE)) {
+        // Unconditional. This used to be guarded on the coinbase still carrying the OP_FALSE
-            cb.vout[0].scriptPubKey = GetScriptForDestination(addr.Get());
+        // placeholder, which made it a no-op for every client after the first once a customized
+        // coinbase had been written back into the shared template -- so those miners silently
+        // mined the first miner's payout address. The template is now left pristine (see
+        // GetWorkUnit), and stamping unconditionally means a coinbase that somehow arrives
+        // already customized can never be inherited by a different miner.
+        if (!addr.IsValid()) {
+            const std::string msg = strprintf("%s: no valid payout address for this client; unable to customize work", __func__);
+            LogPrint("stratum", "%s\n", msg);
+            throw std::runtime_error(msg);
         }
+        cb.vout[0].scriptPubKey = GetScriptForDestination(addr.Get());
     }
 
     // cb_branch = current_work.m_cb_branch;
@@ -864,20 +873,24 @@ std::string GetWorkUnit(StratumClient& client)
         static const std::vector<unsigned char> dummy(32-extranonce1.size(), 0x00); // extranonce2
         CustomizeWork(client, current_work, client.m_addr, extranonce1, dummy, cb, bf, cb_branch);
 
-        // without 2 lines below equihash solutinon on SubmitWork will be incorrect, bcz we should
-        // change vtx[0] in current work and re-calc hashMerkleRoot
-        // TODO: refactor all of these ... may be change this in current_work directly is bad idea,
-        // and we should do all checks and hashMerkleRoot at SubmitBlock(...)
-
-        current_work.GetBlock().vtx[0] = cb;
-        current_work.GetBlock().hashMerkleRoot = current_work.GetBlock().BuildMerkleTree();
-
     }
 
     CBlockHeader blkhdr;
     // Setup native proof-of-work
 
-    blkhdr = current_work.GetBlock().GetBlockHeader(); // copy entire blockheader created with CreateNewBlock to blkhdr
+    // The shared template MUST keep its pristine OP_FALSE coinbase. This previously did
+    //     current_work.GetBlock().vtx[0] = cb;
+    //     current_work.GetBlock().hashMerkleRoot = current_work.GetBlock().BuildMerkleTree();
+    // which published one client's coinbase to every other client on the same job: the merkle
+    // root they were told to mine, and the block they eventually submitted, both committed to
+    // the first client's payout address. Derive this client's header from a local copy instead,
+    // which is what the TODO that used to sit here was asking for.
+    {
+        CBlock tmp(current_work.GetBlock());
+        tmp.vtx[0] = cb;
+        blkhdr = tmp.GetBlockHeader();
+        blkhdr.hashMerkleRoot = tmp.BuildMerkleTree();
+    }
     // CDataStream ds(SER_GETHASH, SERIALIZE_TRANSACTION_NO_WITNESS);
     CDataStream ds(SER_GETHASH, PROTOCOL_VERSION);
     ds << cb;
@@ -1012,9 +1025,32 @@ bool SubmitBlock(StratumClient& client, const uint256& job_id, const StratumWork
                          : std::vector<unsigned char>(sol.begin() + 3, sol.end());
 
         blkhdr.hashFinalSaplingRoot = current_work.GetBlock().hashFinalSaplingRoot;
-        blkhdr.hashMerkleRoot = current_work.GetBlock().hashMerkleRoot;
+        // Recompute from the coinbase CustomizeWork() just derived for THIS client. Reading the
+        // shared template's root would be wrong now that the template is left pristine, and was
+        // wrong before too -- it carried whichever client happened to request work first.
+        {
+            CBlock tmp(current_work.GetBlock());
+            tmp.vtx[0] = cb;
+            blkhdr.hashMerkleRoot = tmp.BuildMerkleTree();
+        }
         blkhdr.nNonce = (uint256) nonce;
 
+        // Cheap SHA256d filter first. This test used to sit *below* the RandomX verify, so 32
+        // arbitrary bytes from any peer bought a full ~65ms randomx_calculate_hash before anything
+        // rejected them -- on the shared HTTP/RPC libevent thread, and holding the global
+        // cs_randomx_validator that block validation also takes. GetHash() is SerializeHash over the
+        // header including nSolution, so passing this costs real grinding. Semantics are unchanged:
+        // an empty local_diff still parses to zero and still rejects, exactly as before.
+        if (!instance_of_cstratumparams.fAllowLowDiffShares &&
+            UintToArith256(blkhdr.GetHash()) > arith_uint256(current_work.local_diff)) {
+            CBlockIndex diff_index;
+            diff_index.nBits = UintToArith256(blkhdr.GetHash()).GetCompact();
+            const double share_diff = GetDifficulty(&diff_index);
+            diff_index.nBits = arith_uint256(current_work.local_diff).GetCompact();
+            const double target_diff = GetDifficulty(&diff_index);
+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Low diff share (diff %g, local %g)", share_diff, target_diff));
+        }
+
         // block is constructed, now it's time to VerifyEH
 
         if (StratumIsRandomX()) {
@@ -1072,10 +1108,8 @@ bool SubmitBlock(StratumClient& client, const uint256& job_id, const StratumWork
         std::chrono::duration<double, std::milli> elapsed;
         uint64_t shares_accepted_since_last;
 
-        // TODO: we need to check hash > local port diff, and if it's true -> throw an exception -> diff too low (!)
+        // (the low-diff share check moved above the RandomX verify -- see SubmitBlock's cheap
-        if (!instance_of_cstratumparams.fAllowLowDiffShares)
+        // SHA256d filter -- so that attacker-controlled bytes cannot buy a RandomX hash)
-            if (UintToArith256(blkhdr.GetHash()) > arith_uint256(current_work.local_diff))
-                throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Low diff share (diff %g, local %g)", hush_real_diff, hush_local_diff));
 
         if (finish > start)
         {
@@ -1323,6 +1357,18 @@ UniValue stratum_mining_submit(StratumClient& client, const UniValue& params)
 
     const std::string method("mining.submit");
     BoundParams(method, params, 5,5);
+
+    // Parity with every other handler (GetWorkUnit, mining.aux.*, mining.extranonce.*), which all
+    // refuse an unauthorized client. NOTE this is not authentication: mining.authorize validates no
+    // credential, so it only costs an attacker one extra line. It is here so the submit path cannot
+    // be reached without at least completing the handshake; the cheap-target check below is what
+    // actually bounds the work an unknown peer can force.
+    if (!client.m_authorized && client.m_aux_addr.empty()) {
+        const std::string msg = strprintf("%s: share submitted by an unauthorized client", __func__);
+        LogPrint("stratum", "%s\n", msg);
+        throw JSONRPCError(RPC_INVALID_REQUEST, "Stratum client not authorized.  Use mining.authorize first, with a DragonX R.. address as the username or 'x' to mine to the default address.");
+    }
+
     // First parameter is the client username, which is ignored.
 
     /* EWBF 31 bytes job_id fix */
@@ -1334,7 +1380,16 @@ UniValue stratum_mining_submit(StratumClient& client, const UniValue& params)
         if (job_id_str.length() == 63) {
             fEWBFJobIDFixNeeded = true;
             for(const auto& hexDigit : hexDigits) {
-                ret = uint256(ParseHex(job_id_str + hexDigit));
+                // ParseHex() stops at the first non-hex character and returns a SHORT vector
+                // without signalling an error, and base_blob(const std::vector<unsigned char>&)
+                // asserts vch.size() == 32. Constructing without checking therefore lets any
+                // 63-character job_id containing a non-hex byte abort the daemon -- from an
+                // unauthenticated client, before any other validation. Skip bad candidates
+                // instead; if none of the 16 completions parse, ret stays null, misses
+                // work_templates below, and the handler returns false cleanly.
+                std::vector<unsigned char> vch = ParseHex(job_id_str + hexDigit);
+                if (vch.size() != 32) continue;
+                ret = uint256(vch);
                 if (work_templates.count(ret)) break;
             }
         }