MST Pending Status #1649

Akvinikym · 2018-08-14T13:05:09Z

Description of the Change

This PR adds a MST_PENDING status for transactions, which were sent for further processing into MST processor.

Benefits

Now users will be able to understand, if their transaction is being processed by MST.

Possible Drawbacks

None.

Signed-off-by: Akvinikym <[email protected]>

muratovv · 2018-08-17T07:54:45Z

irohad/torii/processor/impl/transaction_processor_impl.cpp

@@ -142,6 +142,11 @@ namespace iroha {
      if (boost::size(transaction->signatures()) < transaction->quorum()) {
        log_->info("waiting for quorum signatures");
        mst_processor_->propagateTransaction(transaction);
+        status_bus_->publish(


maybe move the code to separate function?

This status publishing is done in such way throughout this file, and I think it will be unnecessary hard to move it into a separate function, because we need to put the status somehow, and it's done through the builder. In case of separate function we can pass the builder, but why?

So, after a little discuss, I decided that this task actually is about creating something like status factory, which should be done outside this PR

Offline discussion:
Move status_bus::publish to the separate method and add a task for refactoring this method in a separate PR

muratovv · 2018-08-17T08:13:07Z

shared_model/interfaces/transaction_responses/mst_pending_response.hpp

+     * signatories
+     */
+    class MstPendingResponse : public AbstractTxResponse<MstPendingResponse> {
+     private:


I don't see any reason to do it in private. Maybe change it with the public instead?

This method is private in all similar files for other responses. Should I thus touch them as well with that change?

Well, in this case, add a task for refactoring this with the lowest priority.

muratovv · 2018-08-17T08:19:42Z

irohad/torii/processor/impl/transaction_processor_impl.cpp

+            shared_model::builder::DefaultTransactionStatusBuilder()
+                .mstPending()
+                .txHash(transaction->hash())
+                .build());
        return;
      }


Also, there is absent a code for setting status in another one, for example, stateless valid or maybe introduce a new one like prepared to consensus.

Signed-off-by: Akvinikym <[email protected]>

Fixed

l4l · 2018-08-27T09:22:01Z

shared_model/backend/protobuf/transaction_responses/proto_tx_response.hpp

+       *    -  0, if it's equal
+       *    -  1, if it's greater
+       */
+      int comparePriorities(const TransactionResponse &other) const noexcept {


Use enum class instead

What do you mean?

enum class Result { LE, EQ, GT, }; Result comparePriorities(const T&) ...

OK, I implemented this method looking at old C functions, strcmp etc, where it's done in this way :) will change it

Maybe relay operator<, >, == to transactionResponse from priority directly?

l4l · 2018-08-27T09:22:51Z

irohad/torii/impl/command_service.cpp

-                  .getTransport();
-            }())));
+    ResponsePtrType initial_status = cache_->findItem(hash).value_or([&] {
+      log_->debug("tx not received: {}", hash.toString());


tx is not ...

l4l · 2018-08-27T09:24:58Z

shared_model/backend/protobuf/transaction_responses/proto_tx_response.hpp

+       * @return priority this transaction response
+       */
+      int priority() const noexcept {
+        return iroha::visit_in_place(


return variant_->which() ?

No, this won't work, because then we should place classes in variant according to their priorities, and this is not as obvious as this visit_in_place function IMHO

l4l · 2018-08-27T09:26:54Z

test/integration/pipeline/multisig_tx_pipeline_test.cpp

+        ASSERT_NO_THROW(boost::apply_visitor(
+            SpecifiedVisitor<interface::MstPendingResponse>(), resp.get()));
+      };
+//  auto checkMstPassedTxStatus =


Maybe delete commented lines?

muratovv · 2018-08-27T09:49:44Z

irohad/torii/processor/impl/transaction_processor_impl.cpp

@@ -142,6 +142,11 @@ namespace iroha {
      if (boost::size(transaction->signatures()) < transaction->quorum()) {
        log_->info("waiting for quorum signatures");
        mst_processor_->propagateTransaction(transaction);
+        status_bus_->publish(


Offline discussion:
Move status_bus::publish to the separate method and add a task for refactoring this method in a separate PR

muratovv · 2018-08-27T09:50:08Z

irohad/torii/processor/impl/transaction_processor_impl.cpp

@@ -142,6 +142,11 @@ namespace iroha {
      if (boost::size(transaction->signatures()) < transaction->quorum()) {
        log_->info("waiting for quorum signatures");
        mst_processor_->propagateTransaction(transaction);
+        status_bus_->publish(


Offline discussion:
Move status_bus::publish to the separate method and add a task for refactoring this method in a separate PR

muratovv · 2018-08-27T09:52:27Z

shared_model/interfaces/transaction_responses/mst_pending_response.hpp

+     * signatories
+     */
+    class MstPendingResponse : public AbstractTxResponse<MstPendingResponse> {
+     private:


Well, in this case, add a task for refactoring this with the lowest priority.

kamilsa · 2018-08-28T06:46:36Z

test/integration/pipeline/multisig_tx_pipeline_test.cpp

+        ASSERT_NO_THROW(boost::apply_visitor(
+            SpecifiedVisitor<interface::MstPendingResponse>(), resp.get()));
+      };
+//  auto checkEnoughSignaturesCollectedStatus =


Did you forget to delete that?

kamilsa · 2018-08-28T06:52:56Z

shared_model/backend/protobuf/transaction_responses/proto_tx_response.hpp

@@ -85,6 +109,23 @@ namespace shared_model {
      // stub hash
      const Lazy<crypto::Hash> hash_{
          [this] { return crypto::Hash(this->proto_->tx_hash()); }};
+
+      /**
+       * @return priority this transaction response


Maybe explain what priority means?

Signed-off-by: Akvinikym <[email protected]>

Fixed

Signed-off-by: Akvinikym <[email protected]>

muratovv · 2018-08-31T08:55:17Z

irohad/torii/impl/command_service.cpp

-                  .build()
-                  .getTransport();
-            }())));
+    ResponsePtrType initial_status = cache_->findItem(hash).value_or([&] {


Better to use auto instead of type.

muratovv · 2018-08-31T09:09:07Z

shared_model/backend/protobuf/transaction_responses/proto_tx_response.hpp

+       *    -  0, if it's equal
+       *    -  1, if it's greater
+       */
+      int comparePriorities(const TransactionResponse &other) const noexcept {


Maybe relay operator<, >, == to transactionResponse from priority directly?

muratovv · 2018-08-31T09:22:21Z

shared_model/backend/protobuf/transaction_responses/proto_tx_response.hpp

+       * @return priority of this transaction response; transaction response can
+       * only be replaced with one with higher priority
+       */
+      int priority() const noexcept {


Seems, this table should be in the interface, not it proto backend.

muratovv · 2018-08-31T09:31:14Z

shared_model/backend/protobuf/transaction_responses/proto_tx_response.hpp

+            [](const StatelessFailedTxResponse &) { return 6; },
+            [](const StatefulFailedTxResponse &) { return 7; },
+            [](const MstExpiredResponse &) { return 8; },
+            [](const NotReceivedTxResponse &) { return 9; });


Currently, statuses are misleading. For example, why MstExpired is greater than committed?

I suggest reworking priorities with final status priority, which means maximal priority and reflects the impossibility of achieving another status for the transaction. Also, take care of intermediate statuses.

If there will be some troubles, we may arrange an offline meeting.

Signed-off-by: Akvinikym <[email protected]>

Fixed

muratovv · 2018-09-08T07:04:51Z

irohad/torii/impl/command_service.cpp

@@ -57,11 +57,10 @@ namespace torii {
      auto tx_hash = proto_response->transactionHash();
      auto cached_tx_state = cache_->findItem(tx_hash);
      if (cached_tx_state
-          and proto_response->getTransport().tx_status()
-              <= cached_tx_state->tx_status()) {
+          and proto_response->comparePriorities(**cached_tx_state) <= 0) {


What is 0 means? Use status constants instead.

It was a value, which compare function gave, so that <= 0 meant that first value is less or equal, than the second one, C-style. But as it's going to be replaced by enum class, doesn't matter

irohad/torii/processor/impl/transaction_processor_impl.cpp

muratovv · 2018-09-08T07:16:08Z

irohad/torii/processor/impl/transaction_processor_impl.cpp


      mst_processor_->onPreparedBatches().subscribe([this](auto &&batch) {
        log_->info("MST batch prepared");
        // TODO: 07/08/2018 @muratovv rework interface of pcs::propagate batch
        // and mst::propagate batch IR-1584
+        for (const auto &tx : batch->transactions()) {


Seems, code here and at line 128is duplicated. Maybe refactor it with one method instead?

muratovv · 2018-09-08T07:19:57Z

shared_model/backend/protobuf/transaction_responses/proto_tx_response.hpp

+            [](const StatefulValidTxResponse &) { return 4; },
+            // following types are the final ones
+            [](const CommittedTxResponse &) { return max_priority; },
+            [](const StatelessFailedTxResponse &) { return max_priority; },


Statuses

StatelessFailedTxResponse

StatefulFailedTxResponse

MstExpiredResponse
Are not final.

By your request, I can provide situations where it happens.

muratovv · 2018-09-08T07:22:59Z

shared_model/interfaces/transaction_responses/tx_response.hpp

+       *    -  0, if it's equal
+       *    -  1, if it's greater
+       */
+      int comparePriorities(const ModelType &other) const noexcept {


Use enum class for return type instead.

test/module/irohad/torii/processor/transaction_processor_test.cpp

Signed-off-by: Akvinikym <[email protected]>

Fixed

Signed-off-by: Akvinikym <[email protected]>

muratovv

Please check new comments and fix or decline them

Signed-off-by: Akvinikym <[email protected]>

Response is added, and one of tests is upgraded

ccdf52a

Signed-off-by: Akvinikym <[email protected]>

Akvinikym requested review from muratovv and kamilsa August 14, 2018 13:05

Test fix

1fae587

Signed-off-by: Akvinikym <[email protected]>

muratovv previously requested changes Aug 17, 2018

View reviewed changes

Akvinikym added 5 commits August 17, 2018 12:54

Fixed something, but a lot to do more

d43cbfe

Signed-off-by: Akvinikym <[email protected]>

Continued

11d38fb

Signed-off-by: Akvinikym <[email protected]>

Priorities in transactions responses now work as needed

3e1df87

Signed-off-by: Akvinikym <[email protected]>

Merged with develop

c4c5848

Signed-off-by: Akvinikym <[email protected]>

Nearly completed

2b3aac8

Signed-off-by: Akvinikym <[email protected]>

l4l added the mst multisignature transactions label Aug 27, 2018

Renamed the status and fixed tests

192ef2b

Signed-off-by: Akvinikym <[email protected]>

Akvinikym requested a review from muratovv August 27, 2018 09:25

l4l previously requested changes Aug 27, 2018

View reviewed changes

muratovv previously requested changes Aug 27, 2018

View reviewed changes

kamilsa previously requested changes Aug 28, 2018

View reviewed changes

Akvinikym added 3 commits August 29, 2018 14:28

More review issues

3507d06

Signed-off-by: Akvinikym <[email protected]>

Merged with dev

5d08d38

Signed-off-by: Akvinikym <[email protected]>

Fixed some issues after merge

f5bad03

Signed-off-by: Akvinikym <[email protected]>

Akvinikym requested review from kamilsa and muratovv August 29, 2018 11:45

Akvinikym changed the base branch from develop to dev August 29, 2018 11:46

Akvinikym added 2 commits August 29, 2018 16:14

Maybe fixed build

d78baa5

Signed-off-by: Akvinikym <[email protected]>

Fixed test

851b341

Signed-off-by: Akvinikym <[email protected]>

kamilsa approved these changes Aug 31, 2018

View reviewed changes

muratovv previously requested changes Aug 31, 2018

View reviewed changes

Akvinikym added 3 commits September 3, 2018 11:38

Moved some to interface

ee05cf5

Signed-off-by: Akvinikym <[email protected]>

Merge branch 'dev' into feature/mst-pending-status

4054831

Used NVI

ca620cd

Signed-off-by: Akvinikym <[email protected]>

Akvinikym requested a review from muratovv September 4, 2018 10:37

muratovv previously requested changes Sep 8, 2018

View reviewed changes

Akvinikym added 2 commits September 9, 2018 09:26

Couple review issues

b60ecda

Signed-off-by: Akvinikym <[email protected]>

Completed review issues

f6650c8

Signed-off-by: Akvinikym <[email protected]>

Akvinikym requested a review from muratovv September 9, 2018 07:13

Akvinikym added 2 commits September 9, 2018 10:32

Little change with statuses

4c55dce

Signed-off-by: Akvinikym <[email protected]>

Fix Linux build

9c23d61

Signed-off-by: Akvinikym <[email protected]>

muratovv approved these changes Sep 10, 2018

View reviewed changes

Akvinikym merged commit ae2aa68 into dev Sep 10, 2018

Akvinikym deleted the feature/mst-pending-status branch September 10, 2018 07:57

Akvinikym restored the feature/mst-pending-status branch September 10, 2018 07:58

Akvinikym deleted the feature/mst-pending-status branch September 10, 2018 07:59

nickaleks pushed a commit that referenced this pull request Sep 10, 2018

MST Pending Status (#1649)

791beaa

Signed-off-by: Akvinikym <[email protected]>

kamilsa mentioned this pull request Nov 1, 2018

Hyperledger Iroha v1.0 beta-5 version #1825

Closed

bakhtin pushed a commit that referenced this pull request Nov 2, 2018

MST Pending Status (#1649)

f298564

Signed-off-by: Akvinikym <[email protected]>

neewy mentioned this pull request Nov 2, 2018

Hyperledger Iroha v1.0 beta-5 version #1832

Closed

kamilsa mentioned this pull request Dec 12, 2018

Hyperledger Iroha v1.0 Release Candidate 1 #1941

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MST Pending Status #1649

MST Pending Status #1649

Akvinikym commented Aug 14, 2018

muratovv Aug 17, 2018

Akvinikym Aug 17, 2018

Akvinikym Aug 27, 2018

muratovv Aug 27, 2018

muratovv Aug 27, 2018

muratovv Aug 17, 2018

Akvinikym Aug 17, 2018

muratovv Aug 27, 2018

muratovv Aug 17, 2018

l4l Aug 27, 2018

Akvinikym Aug 27, 2018

l4l Aug 27, 2018

Akvinikym Aug 28, 2018

muratovv Aug 31, 2018

l4l Aug 27, 2018

l4l Aug 27, 2018

Akvinikym Aug 27, 2018

l4l Aug 27, 2018

muratovv Aug 27, 2018

muratovv Aug 27, 2018

muratovv Aug 27, 2018

kamilsa Aug 28, 2018

kamilsa Aug 28, 2018

muratovv Aug 31, 2018

muratovv Aug 31, 2018

muratovv Aug 31, 2018

muratovv Aug 31, 2018

muratovv Sep 8, 2018

Akvinikym Sep 8, 2018

muratovv Sep 8, 2018

muratovv Sep 8, 2018

muratovv Sep 8, 2018

muratovv left a comment

MST Pending Status #1649

MST Pending Status #1649

Conversation

Akvinikym commented Aug 14, 2018

Description of the Change

Benefits

Possible Drawbacks

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

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

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

muratovv left a comment

Choose a reason for hiding this comment