Repository navigation
Conversation
…ulk transfer threshold, just reduce the padding
There was a problem hiding this comment.
🟡 Changes recommended
The regression test must verify that the BUSY reply is queued for transmission, not merely prepared.
1 open finding
What changed in this PR
Reports BUSY responses to the pool when bulk-transfer queuing fails, instead of silently dropping requests.
Changes:
- Adds shared BUSY response constants.
- Adds fallback replies for validation and unknown-parent requests.
- Adds a full-queue regression test.
| File | Description |
|---|---|
| src/datum_protocol.h | Defines BUSY response constants. |
| src/datum_protocol.c | Handles queuing failures and limits padding to avoid unnecessary bulk transfers. |
| src/datum_protocol_tests.c | Tests validation responses with a full bulk queue. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| datum_test(datum_protocol_job_validation_sblock(sizeof(validation_request), validation_request)); | ||
| datum_test(temp_data[0] == 0x50 && temp_data[1] == 0x92); | ||
| datum_test(temp_data[2] == pow.datum_job_id && temp_data[3] == DATUM_VALIDATION_ERROR_BUSY); |
jasonsopko
left a comment
jasonsopko
left a comment
There was a problem hiding this comment.
Tested ACK 15cc269
Builds with -Wall -Werror and passes --test in the gcc, clang, Umbrel, no-API and ASan+UBSan configs.
The code does what it says. The test is the weak spot, as Copilot noted. Five mutations of this PR, run against its own --test:
- no BUSY after
sblockfails to queue: caught datum_protocol_job_validation_send_errorbuilds the reply but never callsdatum_protocol_mining_cmd: not caught- no BUSY from
datum_protocol_job_validation_send_reply(0x90, 0x91): not caught datum_protocol_parent_fetch_replywithout the BUSY retry: not caught- padding no longer capped, so a direct reply can pass one fragment: not caught
The patch in the gist checks that each of the three handlers puts a BUSY-sized reply in the outbound buffer when the queue is full, and sends 20 direct replies sized 5 bytes short of one fragment. It catches all but the parent fetch case, which the test file can't reach while datum_protocol_parent_fetch_reply is static. It adds a declaration for datum_protocol_job_validation_stxlist to datum_protocol_internal.h.
Its 0x90 part trips UBSan, which every Linux and FreeBSD CI build has: datum_siphash_mod8 reads hash_bin, at offset 178 of T_DATUM_TEMPLATE_TXN, through a uint64_t pointer. #47 fixes that, and on top of it the patch passes all five configs.
What does the pool do with 0xF6 on 0x90, 0x91 and 0x92: wait and ask again, or count the job as failed validation?
Mutation script, patch and logs: https://gist.github.com/jasonsopko/7df5dc75053fc4abb65cb7b927c816a8
Without this, Gateway will just silently drop requests if there's too many bulk transfers going.