Consensus sim flatten distributed by jmmcgee · Pull Request #3 · bachase/rippled

jmmcgee · 2017-08-30T20:26:56Z

Changes

csv files use | as separator to remove need for quoting
Added python script to visualize simulation data in pdfs
Added threading to DistributedValidators
- known to be currently broken, working on fix
Tx submission follows exponential distribution
Delays for messages in network are randomly generated

bachase

I've only focused on the randomized link changes in the last commit so far. I think there are some issues with dangling references. As a result, that makes me believe having DurationDistributionRef not hold a reference but instead hold a full copy so that it ends up owning the instance of the distrubution. This matches your comment that a distribution should be light-weight and copyable. If a user wants shared state between links, they can use a shared_ptr inside their distribution.

bachase · 2017-08-30T20:35:23Z

src/test/csf/Peer.h

    {
-        return net.connect(this, &o, dur);
+        return connect(o,
+                DurationDistributionRef{ConstantDuration{delay}});


If DurationDistributionRef takes its argument by reference, won't that reference be dangling since ConstantDuration{Delay} is a local temporary?

bachase · 2017-08-30T20:36:08Z

src/test/csf/timers.h

+        operator()();
+    };
+
+    Note T should be light-weight, trivially copyable and movable, etc.


What does T refer to?

bachase · 2017-08-30T20:40:46Z

src/test/csf/BasicNetwork.h

+    connect(
+            Peer const& from,
+            Peer const& to,
+            DurationDistributionRef delayGen);


Since the link constructor is templated on DurationDistribution, should this be too? I like that idea for all the various layers of connect; template all the way down except the last layer in connect, which stores things into the link as a type-erased holder.

bachase · 2017-08-30T20:42:09Z

src/test/csf/timers.h

+
+    template <class T>
+    explicit
+    DurationDistributionRef(T&& t) : impl_{new Any<T>(t)}


I think this constructor might not do as you expect. Since Any<T> is taking its argument by reference, it will end up referring to something that no longer exists.

These are changes to the generic consensus code to support the redesign of the consensus simulation framework (CSF). They are in a standalone changeset to simplify reviewing that the changes are safe. As a result, the motivation for some changes may not be clear. Changes include - Rename Consenus::relay to Consensus::share in preparation for routing layer in CSF. - Define ledger sequence as a type (Ledger::Seq) in preparation for using tagged integers in CSF. In this changeset, it remains a simple type alias. - Use a flat_map for tracking DisputedTx votes. This gives a significant performance improvement in simulations. - Single getPreferredLedger for determing when to switch ledgers

* Adds an event/collector framework for monitoring invariants and doing calculating statistics. * Allows distinct network and trust connections between Peers * Adds a simple routing strategy to support broadcasting arbitrary messages * Adds a common directed graph (`Digraph`) class for representing network and trust topologies * Adds a `PeerGroup` class for simpler specification of the trust and network topologies * Adds a `LedgerOracle` class to ensure distinct ledger histories and simplify branch checking * Adds a `Submitter` to send transactions in at fixed or random intervals to fixed or random peers Co-authored-by: Brad Chase <bchase@ripple.com>

codecov-io · 2017-09-05T22:21:12Z

Codecov Report

❗ No coverage uploaded for pull request base (consensus-sim-flatten@ad62c13). Click here to learn what that means.
The diff coverage is 89.47%.

@@                   Coverage Diff                    @@
##             consensus-sim-flatten       #3   +/-   ##
========================================================
  Coverage                         ?   70.06%           
========================================================
  Files                            ?      689           
  Lines                            ?    50744           
  Branches                         ?        0           
========================================================
  Hits                             ?    35556           
  Misses                           ?    15188           
  Partials                         ?        0

Impacted Files	Coverage Δ
src/ripple/app/consensus/RCLConsensus.h	`82.14% <ø> (ø)`
src/ripple/app/consensus/RCLCxLedger.h	`80% <ø> (ø)`
src/ripple/basics/tagged_integer.h	`94.73% <ø> (ø)`
src/ripple/consensus/LedgerTiming.h	`95.83% <100%> (ø)`
src/ripple/consensus/Validations.h	`98.47% <100%> (ø)`
src/ripple/consensus/DisputedTx.h	`97.26% <100%> (ø)`
src/ripple/app/consensus/RCLConsensus.cpp	`65.57% <25%> (ø)`
src/ripple/consensus/Consensus.h	`88.88% <90%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad62c13...88a8094. Read the comment docs.

bachase

Just a few comments questions. I think this will be added in the next set of changes after the existing PR for ripple/rippled is complete.

bachase · 2017-09-13T19:32:26Z

src/test/consensus/DistributedValidatorsSim_test.cpp

                ledgerLog(prefix + "_ledger.csv", std::ofstream::app);

        // title
+        mutex.lock();


Consider using lock_guard instead of explicit lock and unlock throughout this simulation.

I looked into lock_guards before. I removed them because I had them in their own scope and so everything was indented, and I didn't like that for purely ascetic reasons. But that's not really a justification I suppose. I'll add the lock_guards.

bachase · 2017-09-13T19:38:26Z

src/test/csf/BasicNetwork.h

    time_point const sent = scheduler.now();
    scheduler.in(
-        link->delay,
+        std::max(0ns, link->delayGen()),


Should we also enforce that messages are always sent in order? I believe the following is currently possible

t = 1 : send(a,b, mesg1); with random delay of 50 seconds t = 10 : send(a,b,mesg2); with random delay of 30 seconds ... t= 40: mesg2 delivered t= 51: mesg1 delievered

This might be an important degree of freedom to model, but I'm not sure whether reordering should be explicitly modeled instead. Curious what you think.

I think things make sense the way they are currently. In the future, if we want to model messages being reordered by a different policy/distribution than that which generates the delays, then we can consider constraining messages to be sent in order (since messages being sent out of order only makes sense if the modeling of reordering is wrapped into the generated delay). But then, in order to segment out the effects of the network on message delivery, we would almost need an intermediate entity to intercept all messages and apply some non-ideal policy (reordering, congestion, attacks, etc.). I think that would eventually be a useful feature, but I think it requires a lot more thought.

I agree. I believe we use TCP for all the peer protocol layer, so probably not worth modeling reordering at this stage. In that case, I think we still need to change the proposed implementation to prevent reordering. I think we can just store a SimTime nextDelivery in the link and then ensure that

SimTime when = now() + std::max(0ns, link->delayGen()); when = std::max(when, link->nextDelivery + 1ns); link->nextDelivery = when; scheduler.at(when,...)

bachase · 2017-09-13T19:39:24Z

src/test/csf/timers.h

 // of simulation-specific details.
+/*
+    We should consider making DurationDistribution conform to
+    RandomNumberDistribution concept and moving to beast.


What do you mean by move to beast? Is there something there we can reuse?

When I was originally constructing the DurationDistribution concept, I had it as templated, such that the object returned by operator() could be of any type. The idea was to have a concept:

template <class T> Distribution { T operator(); };

that could represent a random distribution of any object. But I didn't go through with that level of genericness in order to focus on getting things working.

I do think, however, that the idea of having the concept of a random (the precise sense in which it is random is not too important) distribution, that any type of object can be sampled from is very powerful and worth generalizing either in beast or in the C++ standard.

But I'm not expert and I didn't look very hard so It's entirely possible that something like that already exists and I just didn't look hard enough.

bachase · 2017-09-13T19:40:41Z

bin/visualize_sim.py

+        pdf.savefig(fig)
+    pdf.close()
+
+txStats = (


Is it worth inferring the keys from the file itself?

Definitely worth it and possible with a bit more effort. I'll look into it.

- '|' is separator in csv output to allow for json field

jmmcgee · 2017-09-15T18:17:32Z

Sorry about the constant rebasing. I suppose I don't have much else to do.

bachase reviewed Aug 30, 2017

View reviewed changes

bachase and others added 8 commits September 5, 2017 09:07

[FOLD] Separate Scheduler from BasicNetwork

6ce5330

[FOLD] Remove tagged_integer multi-inheritance

cf05599

[FOLD] Add Digraph

ab85692

[FOLD] Use Digraph in BasicNetwork

2f5a582

[FOLD] Add CSF readme

ed0c326

[FOLD] Address PR comments

d5d2227

bachase force-pushed the consensus-sim-flatten branch from 1d946c3 to 53d4bda Compare September 5, 2017 13:41

[FOLD] Use proper closeTime when accepting ledger

73dba2e

bachase force-pushed the consensus-sim-flatten branch from 53d4bda to 73dba2e Compare September 5, 2017 15:31

jmmcgee force-pushed the consensus-sim-flatten-distributed branch 2 times, most recently from abc8fe6 to c3af030 Compare September 5, 2017 21:40

jmmcgee force-pushed the consensus-sim-flatten-distributed branch 4 times, most recently from cbc612d to 8ee281b Compare September 6, 2017 17:45

[FOLD] Address PR comments

1bc30ed

bachase reviewed Sep 13, 2017

View reviewed changes

jmmcgee force-pushed the consensus-sim-flatten-distributed branch from 13ad5b0 to 6320998 Compare September 14, 2017 18:10

[FOLD] python script to visualize simulation data

8712d36

jmmcgee force-pushed the consensus-sim-flatten-distributed branch from 6320998 to 7f39dce Compare September 15, 2017 17:32

jmmcgee added 4 commits September 15, 2017 10:35

[FOLD] Added threading to DistributedValidators:

fa89ca7

- '|' is separator in csv output to allow for json field

[FOLD] Tx submission follows exponential distribution

4be10f6

[FOLD] Add random delay generation

6c4d436

[FOLD] Specify duration at runtime

88a8094

jmmcgee force-pushed the consensus-sim-flatten-distributed branch from 7f39dce to 88a8094 Compare September 15, 2017 18:11

bachase force-pushed the consensus-sim-flatten branch from 8bc1c62 to 212bd2d Compare September 26, 2017 12:20

bachase force-pushed the consensus-sim-flatten branch from 212bd2d to 59f28e6 Compare October 25, 2017 14:02

bachase force-pushed the consensus-sim-flatten branch from 59f28e6 to ad62c13 Compare November 30, 2017 22:14

Conversation

jmmcgee commented Aug 30, 2017

Uh oh!

bachase left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bachase left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmmcgee Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmmcgee Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmmcgee commented Sep 15, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-io commented Sep 5, 2017 •

edited

Loading

jmmcgee Sep 13, 2017 •

edited

Loading

jmmcgee Sep 13, 2017 •

edited

Loading