Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error messages #22

Merged
merged 2 commits into from
Aug 30, 2024
Merged

Improve error messages #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d91f0b
feat: improve deadline error messages
diegonehab Aug 30, 2024
98ba64c
feat: explain taint reason before bailing out
diegonehab Aug 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
### Changed
- Improved messages when there is a timeout
- Improved messages when session is tainted

## [0.9.1] - 2024-03-28
### Changed
Expand Down
140 changes: 63 additions & 77 deletions src/server-manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,15 @@ constexpr const uint64_t ROLLUP_INSPECT_STATE = 1;

static constexpr uint32_t manager_version_major = 0;
static constexpr uint32_t manager_version_minor = 9;
static constexpr uint32_t manager_version_patch = 1;
static constexpr uint32_t manager_version_patch = 2;
static constexpr const char *manager_version_pre_release = "";
static constexpr const char *manager_version_build = "";

static constexpr uint32_t machine_version_major = 0;
static constexpr uint32_t machine_version_minor = 7;

using namespace std::string_literals;

using namespace CartesiServerManager;
using namespace CartesiMachine;
using namespace Versioning;
Expand Down Expand Up @@ -131,6 +133,33 @@ static std::string request_metadata(const grpc::ServerContext &context) {
throw(e); \
} while (0);

// NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
#define CHECK_STATUS_OR_FAIL(st, d) \
do { \
if (!st.ok()) { \
if (st.error_code() == grpc::StatusCode::DEADLINE_EXCEEDED) { \
THROW((finish_error_yield_none{st.error_code(), \
"error contacting remote server: "s + d + " deadline exceeded"s})); \
} else { \
THROW((finish_error_yield_none{st.error_code(), \
"error contacting remote server: "s + st.error_message()})); \
} \
} \
} while (0);

// NOLINTNEXTLINE(cppcoreguidelines-macro-usage)
#define CHECK_STATUS_OR_TAINT(st, se, d) \
do { \
if (!st.ok()) { \
if (st.error_code() == grpc::StatusCode::DEADLINE_EXCEEDED) { \
THROW((taint_session{se, st.error_code(), \
"error contacting remote server: "s + d + " deadline exceeded"s})); \
} else { \
THROW((taint_session{se, st.error_code(), "error contacting remote server: "s + st.error_message()})); \
} \
} \
} while (0);

// gRPC async server calls involve a variety of objects:
// 1) The service object;
// 2) A server context object;
Expand Down Expand Up @@ -654,9 +683,7 @@ static void store(async_context &actx, const std::string &directory) {
grpc::Status status;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((finish_error_yield_none{std::move(status)}));
}
CHECK_STATUS_OR_FAIL(status, "store");
}

/// \brief Marks epoch finished and update all proofs now that all leaves are present
Expand Down Expand Up @@ -809,7 +836,8 @@ static handler_type::pull_type *new_FinishEpoch_handler(handler_context &hctx) {
session.session_lock_reason = new_lock_reason;
// If session is tainted, report potential data loss
if (session.tainted) {
THROW((finish_error_yield_none{grpc::StatusCode::DATA_LOSS, "session is tainted"}));
THROW((finish_error_yield_none{grpc::StatusCode::DATA_LOSS,
"session was previously tainted ("s + session.taint_status.error_message() + ")"}));
}
auto &epochs = session.epochs;
// If epoch is unknown, a bail out
Expand Down Expand Up @@ -933,9 +961,7 @@ static void shutdown_server(async_context &actx) {
grpc::Status status;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((finish_error_yield_none{std::move(status)}));
}
CHECK_STATUS_OR_FAIL(status, "fast");
}

/// \brief Creates a new handler for the EndSession RPC and starts accepting requests
Expand Down Expand Up @@ -1335,9 +1361,7 @@ static void check_server_version(async_context &actx) {
grpc::Status status;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((finish_error_yield_none{std::move(status)}));
}
CHECK_STATUS_OR_FAIL(status, "fast");
// If version is incompatible, bail out
if (response.version().major() != machine_version_major || response.version().minor() != machine_version_minor) {
THROW((finish_error_yield_none{grpc::StatusCode::FAILED_PRECONDITION,
Expand All @@ -1359,9 +1383,7 @@ static void check_server_machine(async_context &actx, const std::string &directo
grpc::Status status;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((finish_error_yield_none{std::move(status)}));
}
CHECK_STATUS_OR_FAIL(status, "machine (instantiation)");
}

/// \brief Asynchronously gets the initial machine configuration from server
Expand All @@ -1377,9 +1399,7 @@ static MachineConfig get_initial_config(async_context &actx) {
grpc::Status status;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((finish_error_yield_none{std::move(status)}));
}
CHECK_STATUS_OR_FAIL(status, "fast");
return response.config();
}

Expand Down Expand Up @@ -1611,9 +1631,7 @@ static uint64_t get_current_mcycle(async_context &actx) {
ReadCsrResponse response;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((finish_error_yield_none{std::move(status)}));
}
CHECK_STATUS_OR_FAIL(status, "fast");
return response.value();
}

Expand All @@ -1626,14 +1644,13 @@ static uint64_t check_is_yielded(async_context &actx) {
RunRequest run_request;
run_request.set_limit(current_mcycle); // This will not change the machine
grpc::ClientContext client_context;
set_deadline(client_context, actx.session.server_deadline.fast);
auto reader = actx.session.server_stub->AsyncRun(&client_context, run_request, actx.completion_queue);
grpc::Status run_status;
RunResponse run_response;
reader->Finish(&run_response, &run_status, actx.self);
actx.yield(side_effect::none);
if (!run_status.ok()) {
THROW((finish_error_yield_none{std::move(run_status)}));
}
CHECK_STATUS_OR_FAIL(run_status, "fast");
if (!run_response.iflags_y()) {
THROW((finish_error_yield_none{grpc::StatusCode::INVALID_ARGUMENT, "expected manual yield"}));
}
Expand All @@ -1656,9 +1673,7 @@ static hash_type get_root_hash(async_context &actx) {
GetRootHashResponse response;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((taint_session{actx.session, std::move(status)}));
}
CHECK_STATUS_OR_TAINT(status, actx.session, "machine (root hash)");
return cartesi::get_proto_hash(response.hash());
}

Expand Down Expand Up @@ -1843,9 +1858,7 @@ static void clear_memory_ranges(async_context &actx) {
reader->Finish(&replace_response, &replace_status, actx.self);
actx.yield(side_effect::none);
(void) replace_request.release_config();
if (!replace_status.ok()) {
THROW((taint_session{actx.session, std::move(replace_status)}));
}
CHECK_STATUS_OR_TAINT(replace_status, actx.session, "fast");
}
}

Expand All @@ -1863,9 +1876,7 @@ static void clear_rx_buffer(async_context &actx) {
reader->Finish(&replace_response, &replace_status, actx.self);
actx.yield(side_effect::none);
(void) replace_request.release_config();
if (!replace_status.ok()) {
THROW((taint_session{actx.session, std::move(replace_status)}));
}
CHECK_STATUS_OR_TAINT(replace_status, actx.session, "fast");
}

/// \brief Asynchronously writes data to a memory range
Expand All @@ -1886,9 +1897,7 @@ static void write_memory_range(async_context &actx, IT begin, IT end, const Memo
grpc::Status write_status;
reader->Finish(&write_response, &write_status, actx.self);
actx.yield(side_effect::none);
if (!write_status.ok()) {
THROW((taint_session{actx.session, std::move(write_status)}));
}
CHECK_STATUS_OR_TAINT(write_status, actx.session, "fast");
}

/// \brief Asynchronously writes an EVM ABI string to a memory range
Expand Down Expand Up @@ -1917,9 +1926,7 @@ static void write_evm_abi_string(async_context &actx, IT begin, IT end, const Me
grpc::Status write_status;
reader->Finish(&write_response, &write_status, actx.self);
actx.yield(side_effect::none);
if (!write_status.ok()) {
THROW((taint_session{actx.session, std::move(write_status)}));
}
CHECK_STATUS_OR_TAINT(write_status, actx.session, "fast");
}

/// \brief Asynchronously runs machine server up to given max cycle
Expand Down Expand Up @@ -1953,9 +1960,7 @@ static std::optional<RunResponse> run_machine(async_context &actx, uint64_t curr
RunResponse run_response;
reader->Finish(&run_response, &run_status, actx.self);
actx.yield(side_effect::none);
if (!run_status.ok()) {
THROW((taint_session{actx.session, std::move(run_status)}));
}
CHECK_STATUS_OR_TAINT(run_status, actx.session, "advance/inspect state increment");
// Check if yielded or halted or reached max_mcycle and return
if (run_response.iflags_y() || run_response.iflags_x() || run_response.iflags_h() ||
run_response.mcycle() >= max_mcycle) {
Expand Down Expand Up @@ -1988,9 +1993,7 @@ static std::string read_memory_range(async_context &actx, const MemoryRangeConfi
ReadMemoryResponse read_response;
reader->Finish(&read_response, &read_status, actx.self);
actx.yield(side_effect::none);
if (!read_status.ok()) {
THROW((taint_session{actx.session, std::move(read_status)}));
}
CHECK_STATUS_OR_TAINT(read_status, actx.session, "fast");
if (read_response.data().size() != read_request.length()) {
THROW((taint_session{actx.session, grpc::StatusCode::INTERNAL, "read returned wrong number of bytes!"}));
}
Expand Down Expand Up @@ -2092,9 +2095,7 @@ static evm_address_type read_voucher_address_and_payload_data_length(async_conte
ReadMemoryResponse read_response;
reader->Finish(&read_response, &read_status, actx.self);
actx.yield(side_effect::none);
if (!read_status.ok()) {
THROW((taint_session{actx.session, std::move(read_status)}));
}
CHECK_STATUS_OR_TAINT(read_status, actx.session, "fast");
if (read_response.data().size() != read_request.length()) {
THROW((taint_session{actx.session, grpc::StatusCode::INTERNAL, "read returned wrong number of bytes!"}));
}
Expand Down Expand Up @@ -2127,9 +2128,7 @@ static std::string read_voucher_payload_data(async_context &actx, uint64_t paylo
ReadMemoryResponse read_response;
reader->Finish(&read_response, &read_status, actx.self);
actx.yield(side_effect::none);
if (!read_status.ok()) {
THROW((taint_session{actx.session, std::move(read_status)}));
}
CHECK_STATUS_OR_TAINT(read_status, actx.session, "fast");
if (read_response.data().size() != payload_data_length) {
THROW((taint_session{actx.session, grpc::StatusCode::INTERNAL, "read returned wrong number of bytes!"}));
}
Expand All @@ -2153,9 +2152,7 @@ static uint64_t read_tx_payload_data_length(async_context &actx) {
ReadMemoryResponse read_response;
reader->Finish(&read_response, &read_status, actx.self);
actx.yield(side_effect::none);
if (!read_status.ok()) {
THROW((taint_session{actx.session, std::move(read_status)}));
}
CHECK_STATUS_OR_TAINT(read_status, actx.session, "fast");
if (read_response.data().size() != read_request.length()) {
THROW((taint_session{actx.session, grpc::StatusCode::INTERNAL, "read returned wrong number of bytes!"}));
}
Expand Down Expand Up @@ -2184,9 +2181,7 @@ static std::string read_tx_payload_data(async_context &actx, uint64_t payload_da
ReadMemoryResponse read_response;
reader->Finish(&read_response, &read_status, actx.self);
actx.yield(side_effect::none);
if (!read_status.ok()) {
THROW((taint_session{actx.session, std::move(read_status)}));
}
CHECK_STATUS_OR_TAINT(read_status, actx.session, "fast");
if (read_response.data().size() != payload_data_length) {
THROW((taint_session{actx.session, grpc::StatusCode::INTERNAL, "read returned wrong number of bytes!"}));
}
Expand All @@ -2211,9 +2206,7 @@ static proof_type get_proof(async_context &actx, uint64_t address, uint64_t log2
GetProofResponse proof_response;
reader->Finish(&proof_response, &proof_status, actx.self);
actx.yield(side_effect::none);
if (!proof_status.ok()) {
THROW((taint_session{actx.session, std::move(proof_status)}));
}
CHECK_STATUS_OR_TAINT(proof_status, actx.session, "machine (proof)");
return cartesi::get_proto_merkle_tree_proof(proof_response.proof());
}

Expand Down Expand Up @@ -2272,9 +2265,7 @@ static void snapshot(async_context &actx) {
Void response;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((taint_session{actx.session, std::move(status)}));
}
CHECK_STATUS_OR_TAINT(status, actx.session, "fast");
}

/// \brief Asynchronously rollback machine server. Used after an input was skipped.
Expand All @@ -2288,9 +2279,7 @@ static void rollback(async_context &actx) {
Void response;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((taint_session{actx.session, std::move(status)}));
}
CHECK_STATUS_OR_TAINT(status, actx.session, "fast");
}

/// \brief Asynchronously resets the iflags.y flag after a machine has yielded
Expand All @@ -2304,9 +2293,7 @@ static void reset_iflags_y(async_context &actx) {
Void response;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((taint_session{actx.session, std::move(status)}));
}
CHECK_STATUS_OR_TAINT(status, actx.session, "fast");
}

/// \brief Asynchronously gets the value of HTIF's fromhost CSR
Expand All @@ -2322,9 +2309,7 @@ static uint64_t get_htif_fromhost(async_context &actx) {
ReadCsrResponse response;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((taint_session{actx.session, std::move(status)}));
}
CHECK_STATUS_OR_TAINT(status, actx.session, "fast");
return response.value();
}

Expand All @@ -2342,9 +2327,7 @@ static void set_htif_fromhost(async_context &actx, uint64_t value) {
Void response;
reader->Finish(&response, &status, actx.self);
actx.yield(side_effect::none);
if (!status.ok()) {
THROW((taint_session{actx.session, std::move(status)}));
}
CHECK_STATUS_OR_TAINT(status, actx.session, "fast");
}

/// \brief Asynchronously sets htif fromhost ack to specify a given request
Expand Down Expand Up @@ -2762,7 +2745,8 @@ static handler_type::pull_type *new_AdvanceState_handler(handler_context &hctx)
session.session_lock_reason = new_lock_reason;
// If session is tainted, report potential data loss
if (session.tainted) {
THROW((finish_error_yield_none{grpc::StatusCode::DATA_LOSS, "session is tainted"}));
THROW((finish_error_yield_none{grpc::StatusCode::DATA_LOSS,
"session was previously tainted ("s + session.taint_status.error_message() + ")"}));
}
// If active epoch does not match expected, bail out
if (session.active_epoch_index != advance_state_request.active_epoch_index()) {
Expand Down Expand Up @@ -2962,7 +2946,8 @@ static handler_type::pull_type *new_InspectState_handler(handler_context &hctx)
session.session_lock_reason = new_lock_reason;
// If session is tainted, report potential data loss
if (session.tainted) {
THROW((finish_error_yield_none{grpc::StatusCode::DATA_LOSS, "session is tainted"}));
THROW((finish_error_yield_none{grpc::StatusCode::DATA_LOSS,
"session was previously tainted ("s + session.taint_status.error_message() + ")"}));
}
// We should be able to find the active epoch, otherwise bail
auto &epochs = session.epochs;
Expand Down Expand Up @@ -2996,7 +2981,8 @@ static handler_type::pull_type *new_InspectState_handler(handler_context &hctx)
}
// There is a chance the session was tainted between our yielding and being resumed
if (session.tainted) {
THROW((finish_error_yield_none{grpc::StatusCode::DATA_LOSS, "session is tainted"}));
THROW((finish_error_yield_none{grpc::StatusCode::DATA_LOSS,
"session is tainted ("s + session.taint_status.error_message() + ")"}));
}
async_context actx{session, request_context, hctx.completion_queue.get(), self, yield};
process_pending_query(hctx, actx, e);
Expand Down
Loading