[INDY-1642] Discarded field to bitmask #895

lampkin-diet · 2018-08-27T15:52:22Z

Signed-off-by: Andrew Nikitin andrew.nikitin@dsr-corporation.com

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

ashcherbakov · 2018-08-28T06:59:44Z

plenum/common/messages/node_messages.py

@@ -141,7 +141,7 @@ class PrePrepare(MessageBase):
        (f.PP_TIME.nm, TimestampField()),
        (f.REQ_IDR.nm, IterableField(LimitedLengthStringField(
            max_length=DIGEST_FIELD_LIMIT))),
-        (f.DISCARDED.nm, NonNegativeNumberField()),
+        (f.DISCARDED.nm, SerializedValueField()),
        (f.DIGEST.nm, LimitedLengthStringField(max_length=DIGEST_FIELD_LIMIT)),
        (f.LEDGER_ID.nm, LedgerIdField()),
        (f.STATE_ROOT.nm, MerkleRootField(nullable=True)),


What about the fields Sergey K. asked about for future PrePrepare enhancements?

ashcherbakov · 2018-08-28T07:01:47Z

plenum/server/replica.py

            ledger_id, self.viewNo,
            pp_seq_no)
-        if not (validReqs or inValidReqs):
+        if discarded_mask.length() == 0:


Maybe check for emptiness of reqs?

ashcherbakov · 2018-08-28T07:05:50Z

plenum/server/replica.py

+
+    def _pack_discarded_mask(self, value):
+        discarded_mask = bitarray()
+        discarded_mask.pack(b''.fromhex(value))


why not bytes.fromhex?

ashcherbakov · 2018-08-28T07:08:01Z

plenum/server/replica.py

@@ -809,7 +820,7 @@ def create3PCBatch(self, ledger_id):
            pp_seq_no,
            tm,
            [req.digest for req in reqs],
-            len(validReqs),
+            self._unpack_discarded_mask(discarded_mask),


Can we do an optimization so that if there are no discarded requests (all passed dynamic validation), then we just use an empty (None or zero) mask.

ashcherbakov · 2018-08-28T07:40:32Z

setup.py

@@ -50,7 +50,7 @@
                      'six==1.11.0', 'psutil==5.4.3', 'intervaltree==2.1.0',
                      'msgpack-python==0.4.6', 'indy-crypto==0.4.3',
                      'python-rocksdb==0.6.9', 'python-dateutil==2.6.1',
-                      'pympler==0.5'],
+                      'pympler==0.5', 'bitarray==0.8.1'],


Need to have a deb package as well?

No, there is python3-bitarray=0.8.1 package in the xenial repo

ashcherbakov · 2018-08-28T07:44:52Z

plenum/test/replica/test_the_same_order_reqidr_in_preprepare.py

+        replica.requests.set_finalised(req)
+    replica.last_accepted_pre_prepare_time = int(time.time())
+    pp = replica.create3PCBatch(DOMAIN_LEDGER_ID)
+    assert pp.reqIdr == [res.key for res in reqs]


Please check the discarded value as well. Maybe we should make dynamic validation deterministic here, so that we can deterministically check discarded value?

ashcherbakov · 2018-08-28T07:45:51Z

plenum/server/replica.py

@@ -1749,9 +1771,11 @@ def order_3pc_key(self, key):
            )

        self.addToOrdered(*key)
+        valid_reqs, invalid_reqs = self._apply_bitmask_to_list(pp.reqIdr,
+                                                               self._pack_discarded_mask(pp.discarded))
        ordered = Ordered(self.instId,


We need to have invalid reqs in Ordered msg to pass them to the monitor

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

ashcherbakov · 2018-08-28T08:00:34Z

plenum/common/messages/node_messages.py

@@ -141,7 +141,7 @@ class PrePrepare(MessageBase):
        (f.PP_TIME.nm, TimestampField()),
        (f.REQ_IDR.nm, IterableField(LimitedLengthStringField(
            max_length=DIGEST_FIELD_LIMIT))),
-        (f.DISCARDED.nm, NonNegativeNumberField()),
+        (f.DISCARDED.nm, SerializedValueField()),


Please fix Input validation tests test_pre_prepare_message

ashcherbakov · 2018-08-28T08:02:08Z

plenum/server/replica.py

+        finally:
+            reqs.append(req)
+
+    def _pack_discarded_mask(self, value):


Please add unit tests for this 2 methods. Should we make the methods static, and maybe even move to some utils?

ashcherbakov · 2018-08-28T08:02:13Z

plenum/server/replica.py

@@ -1738,6 +1749,17 @@ def doOrder(self, commit: Commit):
        self.logger.debug("{} ordering COMMIT {}".format(self, key))
        return self.order_3pc_key(key)

+    def _apply_bitmask_to_list(self, reqIdrs: List, mask: bitarray):


Should we make the methods static, and maybe even move to some utils?

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

…EPREPARE message Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

ashcherbakov · 2018-08-29T14:33:07Z

plenum/server/node.py

@@ -2427,6 +2427,10 @@ def processOrdered(self, ordered: Ordered):
                           'does not exist'.format(self, ordered.instId))
            return False

+        valid_reqs = [self.requests[request_id].finalised


This is not valid requests, this is both valid and invalid ones.

ashcherbakov · 2018-08-29T14:34:42Z

plenum/server/replica.py

@@ -1749,14 +1808,17 @@ def order_3pc_key(self, key):
            )

        self.addToOrdered(*key)
+        valid_reqs, invalid_reqs = self._apply_bitmask_to_list(pp.reqIdr,


Why do we call it here?

I see, if use this info in logging further, then probably pass valid_reqs and invalid_reqs to Ordered msg to avoid the same calculation in node.py?

ashcherbakov · 2018-08-29T14:36:34Z

plenum/server/replica.py

+        finally:
+            reqs.append(req)
+
+    def _pack_discarded_mask(self, value):


pack_discarded_mask, unpack_discarded_mask, drop_discarded_mask, apply_bitmask_to_list, get_valid_reqs, get_invalid_reqs, get_invalid_count needs to be moved to a separate utility class (not in Replica)

ashcherbakov · 2018-08-29T14:48:23Z

plenum/test/monitoring/test_invalid_reqs_in_monitor.py

+        assert len(reqs_count) == 1
+
+
+def test_invalid_reqs(looper,


Let's make the test more predictable:
send 1 valid and 2 invalid reqs, and check that numOrderedRequests in monitor is equal to 3.

sdk_send_random_and_check(...., 1) <patch dynamic validation ot always return false> with pytest,raises(RequestRejectedException) sdk_send_random_and_check(...., 1) with pytest,raises(RequestRejectedException) sdk_send_random_and_check(...., 1) check_count_reqs(nodes, 3)

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] Discarded field to bitmask

745d80d

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

lampkin-diet changed the title ~~[INDY-1642] Discarded field to bitmask~~ [WIP][INDY-1642] Discarded field to bitmask Aug 27, 2018

[INDY-1642] Move discarded to bitmask

82c51cd

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

ashcherbakov reviewed Aug 28, 2018

View reviewed changes

[INDY-1642] fix tests

738407d

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

ashcherbakov reviewed Aug 28, 2018

View reviewed changes

Andrew Nikitin added 12 commits August 28, 2018 13:19

[INDY-1642] add 'sub_seq_no' and 'final' fields into PrePrepare message

7b44f1f

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] fix tests after adding sub_seq_no and final fields for PR…

8934138

…EPREPARE message Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] fix test

dde1ab4

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1643] Take into account invalid requests in monitoring

b06b4f7

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

Merge remote-tracking branch 'public/master' into public/indy-1642

a0df836

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] flake8

d2d9595

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1643] Add test

83896ce

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] Optimize if all requests are valid

95378ec

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] moitoring invalid requests as valid (durations, throughput)

b9ef6a9

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] fix type hint

8d94cb7

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] small improvement

a8bfe4b

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] fix revert_last_ordered

01941e3

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

ashcherbakov changed the title ~~[WIP][INDY-1642] Discarded field to bitmask~~ [INDY-1642] Discarded field to bitmask Aug 29, 2018

ashcherbakov reviewed Aug 29, 2018

View reviewed changes

Andrew Nikitin added 5 commits August 29, 2018 18:43

[INDY-1642] fix comments from PR

19d250b

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] fix some comments from PR

d6a0bf1

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] remove discarded from Ordered message

ca8b713

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] fix tests helper

5e3baf2

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

[INDY-1642] fix batch reverting

cda1478

Signed-off-by: Andrew Nikitin <andrew.nikitin@dsr-corporation.com>

ashcherbakov approved these changes Aug 30, 2018

View reviewed changes

ashcherbakov merged commit 5563712 into hyperledger:master Aug 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[INDY-1642] Discarded field to bitmask #895

[INDY-1642] Discarded field to bitmask #895

lampkin-diet commented Aug 27, 2018

ashcherbakov Aug 28, 2018

ashcherbakov Aug 28, 2018

ashcherbakov Aug 28, 2018

ashcherbakov Aug 28, 2018

ashcherbakov Aug 28, 2018

lampkin-diet Aug 28, 2018

ashcherbakov Aug 28, 2018

ashcherbakov Aug 28, 2018

ashcherbakov Aug 28, 2018

ashcherbakov Aug 28, 2018

ashcherbakov Aug 28, 2018

ashcherbakov Aug 29, 2018

ashcherbakov Aug 29, 2018

ashcherbakov Aug 29, 2018

ashcherbakov Aug 29, 2018

ashcherbakov Aug 29, 2018

[INDY-1642] Discarded field to bitmask #895

[INDY-1642] Discarded field to bitmask #895

Conversation

lampkin-diet commented Aug 27, 2018

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