Conversation
jasonsopko
left a comment
There was a problem hiding this comment.
Tested ACK fc5ec5c
Ubuntu 24.04, gcc 13.3, clang 18.1, on top of master b9ea7dc:
- The CI matrix run locally: gcc and clang with
-Wall -Werror,ENABLE_API=OFF,DATUM_API_FOR_UMBREL, ASan+UBSan. All build clean and pass--test.git diff --checkclean. - Negative control: the new
datum_blake2b_malformed_submit_hex_testsapplied to master without the code change fails five of its six cases, so the test does catch the bug. Master decodes the bad extranonce2, ntime and nonce to zero bytes, hashes them, and answersH-not-zero; only the bad job id was caught, by the index check. With the code change all six answerunknown-workbefore any hashing, as described. - Both writers of a BLAKE2b job's
prevhash(update_stratum_joband the coinbaser path) go throughdatum_stratum_job_refresh_blake2b, soblake2b_prevblock_hiddenis set before a job is published, and the submit path reads it only in the BLAKE2b branch. Every new decode follows the existing length checks, so nothing reads past the string. Droppingstrtoulalso drops its acceptance of a leading0x, whitespace or a sign, which no miner sends.
nit: datum_pow_decode_u32_hex_exact could be datum_pow_decode_hex_exact(hex, 4, buf) plus a big-endian unpack, keeping one decoder. Not blocking.
Not run with live miners.
luke-jr
left a comment
There was a problem hiding this comment.
This PR should really have at least 4 or 5 independent commits
| return hex[out_len<<1] == 0; | ||
| } | ||
|
|
||
| bool datum_pow_decode_u32_hex_exact(const char *hex, uint32_t *out) { |
There was a problem hiding this comment.
Moved into datum_utils as the generic hex_to_bin_exact helper. The PoW-specific decoder has been removed.
| for (size_t i = 0; i < 8; i++) { | ||
| nibble = datum_hex_nibble(hex[i]); | ||
| if (nibble < 0) return false; | ||
| value = (value << 4) | (uint32_t)nibble; | ||
| } |
There was a problem hiding this comment.
Probably should take inspiration from hex_to_bin
There was a problem hiding this comment.
Updated. hex_to_u32 now reuses the shared exact byte decoder and performs only the big-endian unpack. Tests cover uppercase, lowercase, mixed-case, truncated, overlong, and non-hex input.
fc5ec5c to
259cb4f
Compare
|
Updated in response to review:
Verification: fresh normal and ASan/UBSan builds and full test suites pass, and git diff --check is clean. Payout and address-validation behavior remains unchanged. @luke-jr, would you please take another look? |
jasonsopko
left a comment
There was a problem hiding this comment.
Tested ACK a35e10f
Re-review after the split. The six commits add up to the same change I ACKed at fc5ec5c plus what Luke asked for and one fix: the decoder now lives in datum_utils as hex_to_bin_exact, hex_to_u32 is a big-endian unpack on top of it (the nit from my first pass), and hex_to_bin_exact checks the first nibble before reading the second so a short string is rejected inside its allocation. Nothing else moved.
Ubuntu 24.04, gcc 13.3, clang 18.1, on master b9ea7dc:
- Each of the six commits builds with
-Wall -Werrorand passes--teston its own. - The CI matrix on the head (gcc and clang
-Werror,DATUM_API_FOR_UMBREL,ENABLE_API=OFF, gcc ASan+UBSan) builds clean and passes--test.git diff --checkclean. A clang UBSan-only build fails--testatdatum_protocol_tests.c:133, the misaligned store that is on master and that #14 fixes; not this PR's. - Negative control: this head with
datum_stratum.creverted to master fails five of the six malformed-submit cases and the cached-prevblock check in the refresh test, so the tests catch both changes. - Over a real socket on regtest (BLAKE2b from height 1), twelve
mining.submitlines against a gateway built from this head and one built from the same tree with master'sdatum_stratum.c. The master path answersH-not-zeroto all twelve: a job id whose coinbase byte isg0, an extranonce2 ending ing,0x000000and0000000as ntime,-0000001as nonce, all decode to something and get hashed. This head answersunknown-workto those nine before hashing andH-not-zeroto the three well-formed ones, uppercase hex included. fuzz_stratum_line, a libFuzzer harness of mine that feeds one JSON line to the Stratum command handler for an authorized miner with a job present, ASan+UBSan, on this head for 10 minutes, 73.6M inputs from the seed corpus and dictionary, no findings.
#15 carries this change in innerhat's restructured form. Whichever goes first is Luke's call: if this one does, #15 rebases onto identical hunks; if #15 does, this closes.
Did not test with live miners.
Summary
Rationale
The hidden previous-block value is constant for every share on a job. Computing it during job refresh removes a tagged SHA-256 operation from the share-submission hot path, at a storage cost of 32 bytes per job (8 KiB for 256 jobs).
The existing lookup-based conversion maps invalid hexadecimal pairs to zero. Rejecting malformed fields explicitly prevents distinct invalid submissions from aliasing valid zero bytes and avoids unnecessary proof-of-work processing.
Payout and address-validation behavior is unchanged.
Testing
datum_gateway --testsuitegit diff --check