Proposal gate #1623

lebdron · 2018-08-01T15:21:54Z

Description of the Change

YAC proposal gate implementation:

Use hash gate for consensus
Hash provider with round number as round indentifier and optional with vote

Unrelated changes note

Bind operator operator| was updated to allow non-copyable objects. "Legacy" code in old model and iroha-cli was not compliant with changes and had to be updated.

kamilsa · 2018-08-02T14:45:27Z

test/module/irohad/consensus/yac/yac_proposal_gate_test.cpp

+  // generate order of peers
+  EXPECT_CALL(*peer_orderer, getOrdering(_)).WillOnce(Return(order));
+
+  // ya consensus


Since YAC stands for Yet Another Consensus, "yac consensus" is yet another consensus consensus ^^

kamilsa · 2018-08-03T10:43:51Z

irohad/network/proposal_gate.hpp

+       * Vote for proposal
+       * @param vote - structure with optional proposal and round number
+       */
+      virtual void vote(ProposalVote vote) = 0;


Can it be const ref?

Probably yes, but semantic will be different. Because vote contains a unique pointer, value argument means that vote should be moved inside proposal gate. It is done because vote is stored inside proposal gate for commit comparison.

muratovv · 2018-08-01T18:30:31Z

irohad/consensus/yac/impl/yac_proposal_gate_impl.cpp

+  auto order = orderer_->getOrdering(hash);
+  if (not order) {
+    log_->error("ordering doesn't provide peers => pass round");
+    return;


What is the execution flow in failure case? Will Iroha available to vote after it?

This behavior is defined by the caller. Method will return Result, which wil allow to distinguish bad outcomes.

muratovv · 2018-08-10T12:47:49Z

irohad/consensus/yac/impl/yac_proposal_gate_impl.cpp

+        [&](RejectMessage reject) -> ProposalOutcomeType {
+          return ProposalReject{model_hash.round};
+        });
+    current_proposal_.reset();


What is the point to reset here? As I expected, we have to change current_proposal on commit\reject of the block and reject of proposals, isn't it?

Probably naming is not the best, because it represents "last voted value", which is reset after consensus outcome is received.

muratovv · 2018-08-10T12:51:47Z

irohad/consensus/yac/yac_proposal_hash_provider.hpp

+         * @param hash - for converting
+         * @return proposal information
+         */
+        virtual ProposalInfo toModelHash(const YacHash &hash) const = 0;


Model here a little bit misleading. Maybe rename it somehow?

muratovv · 2018-08-10T12:54:07Z

irohad/network/proposal_gate.hpp

+namespace iroha {
+  namespace network {
+
+    struct ProposalVote {


Add documentation for this structure, the structure below and type name.

muratovv · 2018-08-10T12:56:43Z

libs/common/types.hpp

-                              decltype(f(*t))>::type {
-    if (t) {
-      return f(*t);
+  auto operator|(T &&t, Transform &&f) -> std::enable_if_t<


Nowadays, operator bind seems very complicated. I suggest to add documentation about sfinae here.

muratovv · 2018-08-10T13:03:36Z

test/module/irohad/consensus/yac/yac_proposal_hash_provider_test.cpp

+
+/**
+ * @given non-empty proposal optional and round number
+ * @when proposal, round -> yac hash -> proposal info


-> is not so understandable here, maybe rework it with a verb?

Changes applied

muratovv · 2018-08-15T08:24:18Z

irohad/consensus/yac/impl/yac_proposal_gate_impl.cpp

  auto hash = hash_provider_->makeHash(vote);
  log_->info("vote for proposal ({}, {}, {})",
             vote.proposal ? vote.proposal.value()->hash().toString() : "''",
             vote.round.first,
             vote.round.second);
  auto order = orderer_->getOrdering(hash);
  if (not order) {
-    log_->error("ordering doesn't provide peers => pass round");
-    return;
+    return expected::makeError("ordering doesn't provide peers => pass round");


"=> pass round" is redundant because a strategy is provided by outer scope.

muratovv · 2018-08-15T08:24:59Z

irohad/consensus/yac/impl/yac_proposal_gate_impl.cpp

  auto hash = hash_provider_->makeHash(vote);
  log_->info("vote for proposal ({}, {}, {})",
             vote.proposal ? vote.proposal.value()->hash().toString() : "''",
             vote.round.first,
             vote.round.second);
  auto order = orderer_->getOrdering(hash);
  if (not order) {
-    log_->error("ordering doesn't provide peers => pass round");
-    return;
+    return expected::makeError("ordering doesn't provide peers => pass round");


"=> pass round" is redundant because a strategy is provided by outer scope.

Signed-off-by: Andrei Lebedev <[email protected]>

sorabot · 2018-08-17T14:27:10Z

SonarQube analysis reported 2 issues

interactive_common_cli.cpp#L43: Return value of function to_string() is not used.
yac_gate_impl.hpp#L68: Unused private function: 'YacGateImpl::copySignatures'

lebdron requested review from muratovv and kamilsa August 1, 2018 15:21

kamilsa reviewed Aug 2, 2018

View reviewed changes

kamilsa approved these changes Aug 3, 2018

View reviewed changes

muratovv previously requested changes Aug 10, 2018

View reviewed changes

lebdron requested a review from muratovv August 14, 2018 14:01

lebdron added needs-review pr awaits review from maintainers ordering labels Aug 14, 2018

lebdron force-pushed the feature/proposal-gate branch 2 times, most recently from 6a98ae4 to 4581ff0 Compare August 14, 2018 14:06

muratovv approved these changes Aug 15, 2018

View reviewed changes

lebdron force-pushed the feature/proposal-gate branch from 4d8a994 to 9f56b34 Compare August 16, 2018 12:31

lebdron changed the base branch from trunk/bft_os to trunk/bft-os August 16, 2018 12:31

lebdron force-pushed the feature/proposal-gate branch from 9f56b34 to 11b8362 Compare August 16, 2018 13:21

lebdron force-pushed the trunk/bft-os branch from 1f6c56e to ed7b375 Compare August 17, 2018 13:54

Proposal gate and proposal hash provider

a5b00be

Signed-off-by: Andrei Lebedev <[email protected]>

lebdron force-pushed the feature/proposal-gate branch from 11b8362 to a5b00be Compare August 17, 2018 13:59

lebdron merged commit c89bd5f into trunk/bft-os Aug 17, 2018

lebdron deleted the feature/proposal-gate branch August 17, 2018 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proposal gate #1623

Proposal gate #1623

lebdron commented Aug 1, 2018 •

edited

Loading

kamilsa Aug 2, 2018

lebdron Aug 3, 2018

kamilsa Aug 3, 2018

lebdron Aug 14, 2018

muratovv Aug 1, 2018

lebdron Aug 14, 2018

muratovv Aug 10, 2018

lebdron Aug 14, 2018

muratovv Aug 10, 2018

muratovv Aug 10, 2018

muratovv Aug 10, 2018

muratovv Aug 10, 2018

muratovv Aug 15, 2018

muratovv Aug 15, 2018

sorabot commented Aug 17, 2018

Proposal gate #1623

Proposal gate #1623

Conversation

lebdron commented Aug 1, 2018 • edited Loading

Description of the Change

Unrelated changes note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorabot commented Aug 17, 2018

lebdron commented Aug 1, 2018 •

edited

Loading