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.

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

refactor: use noir::Result for return type instead of std::optional<s… #216

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 4 additions & 5 deletions src/noir/consensus/block_executor.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,8 @@ struct block_executor {
return true;

/// Validate block
if (auto err = block_->validate_basic(); err.has_value()) {
elog(fmt::format("invalid header: {}", err.value()));
if (auto ok = block_->validate_basic(); !ok) {
elog(fmt::format("invalid header: {}", ok.error()));
return false;
}

Expand Down Expand Up @@ -363,9 +363,8 @@ struct block_executor {
if (abci_responses_->end_block.consensus_param_updates.has_value()) {
// Note: must not mutate consensus_params
next_params = abci_responses_->end_block.consensus_param_updates.value(); // todo - check if this is correct
auto err = next_params.validate_consensus_params();
if (err.has_value()) {
elog(fmt::format("error updating consensus_params: {}", err.value()));
if (auto ok = next_params.validate_consensus_params(); !ok) {
elog(fmt::format("error updating consensus_params: {}", ok.error()));
return {};
}

Expand Down
10 changes: 5 additions & 5 deletions src/noir/consensus/block_sync/reactor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,16 +136,16 @@ void reactor::try_sync_ticker() {
auto first_id = p2p::block_id{first->get_hash(), first_part_set_header};

// Verify the first block using the second's commit
if (auto err = verify_commit_light(chain_id, latest_state.validators, first_id, first->header.height,
if (auto ok = verify_commit_light(chain_id, latest_state.validators, first_id, first->header.height,
std::make_shared<commit>(*second->last_commit));
err.has_value()) {
elog(fmt::format("invalid last commit: height={} err={}", first->header.height, err.value()));
!ok) {
elog(fmt::format("invalid last commit: height={} err={}", first->header.height, ok.error()));

// We already removed peer request, but we need to clean up the rest
auto peer_id1 = pool->redo_request(first->header.height);
pool->send_error(err.value(), peer_id1);
pool->send_error(ok.error().message(), peer_id1);
auto peer_id2 = pool->redo_request(second->header.height);
pool->send_error(err.value(), peer_id2);
pool->send_error(ok.error().message(), peer_id2);
} else {
pool->pop_request();

Expand Down
4 changes: 2 additions & 2 deletions src/noir/consensus/consensus_reactor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,8 @@ void consensus_reactor::process_peer_msg(p2p::envelope_ptr info) {
return;

// Peer claims to have a maj23 for some block_id
if (auto err = votes->set_peer_maj23(msg.round, msg.type, ps->peer_id, msg.block_id_); err.has_value()) {
elog("${err}", ("err", err));
if (auto ok = votes->set_peer_maj23(msg.round, msg.type, ps->peer_id, msg.block_id_); !ok) {
elog(ok.error().message());
return;
}

Expand Down
2 changes: 1 addition & 1 deletion src/noir/consensus/consensus_state.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@ void consensus_state::decide_proposal(int64_t height, int32_t round) {
auto prop_block_id = p2p::block_id{block_->get_hash(), block_parts_->header()};
auto proposal_ = p2p::proposal_message{p2p::Proposal, height, round, rs.valid_round, prop_block_id, get_time()};

if (auto err = local_priv_validator->sign_proposal(local_state.chain_id, proposal_); !err.has_value()) {
if (auto ok = local_priv_validator->sign_proposal(local_state.chain_id, proposal_); ok) {
// proposal_.signature = p.signature; // TODO: no need; already updated proposal_.signature

// Send proposal and block_parts
Expand Down
13 changes: 7 additions & 6 deletions src/noir/consensus/merkle/proof.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#pragma once
#include <noir/common/for_each.h>
#include <noir/consensus/merkle/tree.h>
#include <noir/core/result.h>
#include <tendermint/crypto/proof.pb.h>

#include <optional>
Expand All @@ -18,18 +19,18 @@ struct proof {
Bytes leaf_hash{};
bytes_list aunts{};

std::optional<std::string> verify(const Bytes& root_hash, const Bytes& leaf) {
noir::Result<void> verify(const Bytes& root_hash, const Bytes& leaf) {
if (total < 0)
return "proof total must be positive";
return noir::Error("proof total must be positive");
if (index < 0)
return "proof index must be positive";
return noir::Error("proof index must be positive");
auto leaf_hash_ = leaf_hash_opt(leaf);
if (leaf_hash_ != leaf_hash)
return "invalid leaf hash";
return noir::Error("invalid leaf hash");
auto computed_hash = compute_root_hash();
if (computed_hash != root_hash)
return "invalid root hash";
return {};
return noir::Error("invalid root hash");
return success();
}

Bytes compute_root_hash() const {
Expand Down
12 changes: 6 additions & 6 deletions src/noir/consensus/merkle/test/tree_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,20 +70,20 @@ TEST_CASE("merkle_tree: Verify proof", "[noir][consensus]") {
CHECK(proof->index == i);
CHECK(proof->total == total);

auto err = proof->verify(root_hash, items[i]);
CHECK(!err.has_value());
auto ok = proof->verify(root_hash, items[i]);
CHECK(!ok.has_error());

// Trail too long should fail
auto orig_aunts = proof->aunts;
proof->aunts.push_back({static_cast<unsigned char>(i % 256)});
err = proof->verify(root_hash, items[i]);
CHECK(err.value() == "invalid root hash");
ok = proof->verify(root_hash, items[i]);
CHECK(ok.error().message() == "invalid root hash");
proof->aunts = orig_aunts;

// Trail too short should fail
proof->aunts.pop_back();
err = proof->verify(root_hash, items[i]);
CHECK(err.value() == "invalid root hash");
ok = proof->verify(root_hash, items[i]);
CHECK(ok.error().message() == "invalid root hash");
proof->aunts = orig_aunts;
}
}
14 changes: 7 additions & 7 deletions src/noir/consensus/privval/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <noir/common/types.h>
#include <noir/consensus/crypto.h>
#include <noir/consensus/types/priv_validator.h>
#include <noir/core/result.h>
#include <noir/p2p/protocol.h>
#include <filesystem>

Expand Down Expand Up @@ -172,22 +173,21 @@ struct file_pv : public noir::consensus::priv_validator {
/// \brief signs a canonical representation of the vote, along with the chainID
/// \param[in] vote_
/// \return
std::optional<std::string> sign_vote(const std::string& chain_id, noir::consensus::vote& vote) override {
noir::Result<void> sign_vote(const std::string& chain_id, noir::consensus::vote& vote) override {
if (auto ok = sign_vote_internal(chain_id, vote); !ok) {
return "error signing vote" + ok.error().message();
return noir::Error::format("error signing vote {}", ok.error());
}
return {};
return success();
}

/// \brief signs a canonical representation of the proposal, along with the chainID
/// \param[in] proposal_
/// \return
std::optional<std::string> sign_proposal(
const std::string& chain_id, noir::p2p::proposal_message& proposal) override {
noir::Result<void> sign_proposal(const std::string& chain_id, noir::p2p::proposal_message& proposal) override {
if (auto ok = sign_proposal_internal(chain_id, proposal); !ok) {
return "error signing proposal" + ok.error().message();
return noir::Error::format("error signing proposal {}", ok.error());
}
return {};
return success();
}

Result<Bytes> sign_vote_pb(const std::string& chain_id, const ::tendermint::types::Vote& v) override {
Expand Down
3 changes: 1 addition & 2 deletions src/noir/consensus/privval/test/file_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,10 @@ TEST_CASE("priv_val_file: test file_pv", "[noir][consensus]") {
auto data_vote1 = vote::vote_sign_bytes(test_chain_id, *vote::to_proto(vote_));
// std::cout << "data_vote1=" << to_hex(data_vote1) << std::endl;
// std::cout << "digest1=" << fc::Sha256::hash(data_vote1).str() << std::endl;
std::optional<std::string> sig_org;
if (st.expect_throw) {
CHECK_THROWS(file_pv_ptr->sign_vote(test_chain_id, vote_));
} else {
CHECK_NOTHROW(sig_org = file_pv_ptr->sign_vote(test_chain_id, vote_));
CHECK_NOTHROW(file_pv_ptr->sign_vote(test_chain_id, vote_));
// std::cout << "sig=" << std::string(vote_.signature.begin(), vote_.signature.end()) << std::endl;

auto data_vote2 = vote::vote_sign_bytes(test_chain_id, *vote::to_proto(vote_));
Expand Down
2 changes: 1 addition & 1 deletion src/noir/consensus/types/block.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ bool part_set::add_part(std::shared_ptr<part> part_) {
}

// Check hash proof
if (auto err = part_->proof_.verify(get_hash(), part_->bytes_); err.has_value()) {
if (auto ok = part_->proof_.verify(get_hash(), part_->bytes_); !ok) {
elog("error part set invalid proof");
return false;
}
Expand Down
23 changes: 12 additions & 11 deletions src/noir/consensus/types/block.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <noir/consensus/merkle/proof.h>
#include <noir/consensus/tx.h>
#include <noir/consensus/version.h>
#include <noir/core/result.h>
#include <noir/crypto/rand.h>
#include <noir/p2p/protocol.h>
#include <noir/p2p/types.h>
Expand Down Expand Up @@ -327,19 +328,19 @@ struct block_header {
proposer_address = proposer_address_;
}

std::optional<std::string> validate_basic() {
noir::Result<void> validate_basic() {
if (chain_id.length() > 50)
return "chain_id is too long";
return noir::Error("chain_id is too long");
if (height < 0)
return "negative height";
return noir::Error("negative height");
else if (height == 0)
return "zero height";
return noir::Error("zero height");
// last_block_id->validate_basic();
// validate_hash(last_commit_hash);
// validate_hash(data_hash);
// validate_hash(evidence_hash);
// todo - add more
return {};
return success();
}

static Result<block_header> make_header(block_header&& h) {
Expand Down Expand Up @@ -369,8 +370,8 @@ struct block_header {
h.evidence_hash = random_hash();
if (h.proposer_address.empty())
h.proposer_address = random_address();
if (auto r = h.validate_basic(); r.has_value())
return Error::format("{}", r.value());
if (auto ok = h.validate_basic(); !ok)
return ok.error();
return h;
}

Expand Down Expand Up @@ -478,11 +479,11 @@ struct block {

static std::shared_ptr<block> new_block_from_part_set(const std::shared_ptr<part_set>& ps);

std::optional<std::string> validate_basic() {
noir::Result<void> validate_basic() {
std::scoped_lock g(mtx);

if (auto err = header.validate_basic(); err.has_value())
return "invalid header: " + err.value();
if (auto ok = header.validate_basic(); !ok)
return noir::Error::format("invalid header: {}", ok.error());

// Validate last commit
// if (last_commit.height == 0)
Expand All @@ -491,7 +492,7 @@ struct block {
// if (last_commit.get_hash() != header.last_commit_hash)
// return "wrong last_commit_hash";

return {};
return success();
}

void fill_header() {
Expand Down
19 changes: 10 additions & 9 deletions src/noir/consensus/types/event_bus.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#pragma once

#include <noir/consensus/types/events.h>
#include <noir/core/result.h>
#include <appbase/application.hpp>
#include <appbase/channel.hpp>
#include <boost/uuid/uuid.hpp>
Expand Down Expand Up @@ -49,27 +50,27 @@ class event_bus {
subscription(const subscription&) = delete;
subscription& operator=(const subscription&) = delete;

std::optional<std::string> unsubscribe() {
noir::Result<void> unsubscribe() {
auto handle_ptr = handle_.lock();
auto map_ptr = map_.lock();
if (!handle_ptr || !map_ptr) {
return "invalid subscription";
return noir::Error("invalid subscription");
}
handle_ptr->unsubscribe();

auto it = map_ptr->find(subscriber);
if (it == map_ptr->end()) {
return fmt::format("invalid subscriber:{}", subscriber);
return noir::Error::format("invalid subscriber:{}", subscriber);
}
auto it2 = it->second.find(id);
if (it2 == it->second.end()) {
return fmt::format("invalid subscription id: {}", id);
return noir::Error::format("invalid subscription id: {}", id);
}
it->second.erase(id);
if (it->second.size() == 0) { // if all elements are deleted
map_ptr->erase(subscriber);
}
return {};
return success();
}

private:
Expand Down Expand Up @@ -105,20 +106,20 @@ class event_bus {
return std::move(ret);
}

std::optional<std::string> unsubscribe(subscription& handle) {
noir::Result<void> unsubscribe(subscription& handle) {
return handle.unsubscribe();
}

std::optional<std::string> unsubscribe_all(const std::string& subscriber) {
noir::Result<void> unsubscribe_all(const std::string& subscriber) {
auto it = subscription_map_->find(subscriber);
if (it == subscription_map_->end()) {
return fmt::format("invalid subscriber:{}", subscriber);
return noir::Error::format("invalid subscriber:{}", subscriber);
}
for (auto it2 = it->second.begin(); it2 != it->second.end(); ++it2) {
it2->second->unsubscribe();
}
subscription_map_->erase(subscriber);
return {};
return success();
}

void publish(const std::string& event_value, const tm_event_data& data) {
Expand Down
5 changes: 2 additions & 3 deletions src/noir/consensus/types/genesis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,8 @@ bool genesis_doc::validate_and_complete() {
if (!cs_params.has_value()) {
cs_params = consensus_params::get_default();
} else {
auto err = cs_params->validate_consensus_params();
if (err.has_value()) {
elog(err.value());
if (auto ok = cs_params->validate_consensus_params(); !ok) {
elog(ok.error().message());
return false;
}
}
Expand Down
7 changes: 4 additions & 3 deletions src/noir/consensus/types/height_vote_set.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <noir/consensus/types/node_id.h>
#include <noir/consensus/types/validator.h>
#include <noir/consensus/types/vote.h>
#include <noir/core/result.h>
#include <noir/p2p/types.h>

namespace noir::consensus {
Expand Down Expand Up @@ -79,14 +80,14 @@ struct height_vote_set {
round = round_;
}

std::optional<std::string> set_peer_maj23(
noir::Result<void> set_peer_maj23(
int32_t round, p2p::signed_msg_type vote_type, std::string peer_id, p2p::block_id block_id_) {
std::scoped_lock g(mtx);
if (vote_type != p2p::Prevote && vote_type != p2p::Precommit)
return "setPeerMaj23: Invalid vote type";
return noir::Error("setPeerMaj23: Invalid vote type");
auto vote_set_ = get_vote_set(round, vote_type);
if (vote_set_ == nullptr)
return {};
return success();
return vote_set_->set_peer_maj23(peer_id, block_id_);
}

Expand Down
11 changes: 6 additions & 5 deletions src/noir/consensus/types/params.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
//
#pragma once
#include <noir/codec/protobuf.h>
#include <noir/core/result.h>
#include <noir/crypto/hash.h>
#include <noir/p2p/types.h>
#include <tendermint/types/params.pb.h>
Expand Down Expand Up @@ -56,18 +57,18 @@ struct consensus_params {
validator_params validator;
version_params version;

std::optional<std::string> validate_consensus_params() const {
noir::Result<void> validate_consensus_params() const {
if (block.max_bytes <= 0)
return "block.MaxBytes must be greater than 0.";
return noir::Error("block.MaxBytes must be greater than 0.");
if (block.max_bytes > max_block_size_bytes)
return "block.MaxBytes is too big.";
return noir::Error("block.MaxBytes is too big.");
if (block.max_gas < -1)
return "block.MaxGas must be greater or equal to -1.";
return noir::Error("block.MaxGas must be greater or equal to -1.");
// check evidence // todo - necessary?
// if (validator.pub_key_types.empty())
// return "validator.pub_key_types must not be empty.";
// check if key_type is known // todo
return {};
return success();
}

static consensus_params get_default() {
Expand Down
Loading