Closed olgavrou closed 4 years ago
Merging #553 into master will decrease coverage by
0.02%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #553 +/- ##
==========================================
- Coverage 78.56% 78.54% -0.02%
==========================================
Files 140 140
Lines 10508 10558 +50
==========================================
+ Hits 8255 8292 +37
- Misses 2253 2266 +13
Flag | Coverage Δ | |
---|---|---|
#e2e_BFT | 50.9% <88.57%> (+0.12%) |
:arrow_up: |
#e2e_CFT | 73.4% <88.57%> (-0.07%) |
:arrow_down: |
#unit_BFT | ? |
|
#unit_CFT | 72.84% <95.83%> (-0.09%) |
:arrow_down: |
Impacted Files | Coverage Δ | |
---|---|---|
src/node/networktables.h | 100% <ø> (ø) |
:arrow_up: |
src/kv/genericserialisewrapper.h | 89.93% <ø> (ø) |
:arrow_up: |
src/enclave/enclave.h | 98.99% <100%> (ø) |
:arrow_up: |
src/host/main.cpp | 98.32% <100%> (+0.08%) |
:arrow_up: |
src/kv/kvtypes.h | 60% <100%> (+7.06%) |
:arrow_up: |
src/node/history.h | 55.35% <100%> (ø) |
:arrow_up: |
src/enclave/ccf_v.h | 74.36% <100%> (+0.67%) |
:arrow_up: |
src/kv/kv.h | 87.67% <100%> (+0.51%) |
:arrow_up: |
src/node/networkstate.h | 100% <100%> (ø) |
:arrow_up: |
src/host/enclave.h | 52.05% <100%> (+0.67%) |
:arrow_up: |
... and 3 more |
As discussed with @achamayou and @ashamis, the PR will be updated so that the derived write set is also passed through to replicate()
. That way when we do catchup in PBFT, or when we are replaying the ledger, we don't need to execute the requests.
@olgavrou not sure if I'm reading too much into that, but it looks like the last few cimetrics agree on a fairly consistent -5% on KV serialise. It's not enough that I'm worried about it, but it may be interesting to do some local kv_bench runs to check if that's accurate or not.
@achamayou regarding the metrics running the bench mark for some time (master vs my PR) I get the below results (roughly):
@olgavrou thanks for checking, that looks like it’s within noise range.
Regarding issue #539 addressing action points 1 and 2. Regarding issue #562 addressing two middle check points (runtime consensus switch, passing switch to enclave/nodestate)
Map
has a boolean fieldreplicated
that indicates if the map is a replicated or a derived one. By default if we are using Raft this will be set totrue
. If we are in PBFT it will be set tofalse
, so any PBFT replicated tables need to be explicitly defined as replicated onesTxView
so thatTx::serialise()
knows if the tx view over a map it serialising is replicated or derivedTx::serialise()
returns aSerialisedMaps
struct that contains 2 byte-vectors, one for replicated maps and one for derived maps, instead of returning a single byte-vectorTx::serialise()
creates two serializers, one for each type of table and returns the raw data from each. Each serializer continues to serialize the data based on the domain (i.e. first the data from the public tables and then the private ones)GenericSerialiseWrapper
to check that there are actual data written in the serializer, and if not return an empty vector. This is needed since in the constructor we pass in the version to be serialised. Since now we create two serializers, one can be empty if there are no replicated or derived maps. However because the serializer has been created,get_raw_data()
would return a vector with size 1, since we have written the version on constructionreplicate()
is called inStore::commit()
but history now gets both the replicated and derived write sets and can decide what to do with them. For now we just concatenate the two vectors and use them to update the merkle tree. The other option would be to haveSerialisedMaps
contain a vector over replicated maps and a vector over all-maps (instead of replicated and derived), and then have the merkle tree updated with the all-maps vector. But then we either serialize and encrypt the replicated maps twice or have to find a workaround to serialize replicated maps only once but reference them twice