Improve integration tests, fix bugs in rita #34

kingoflolz · 2018-02-07T10:05:13Z

(this PR is a superset of #30)

This commit prevents `rita.sh` from doing everything as root by calling sudo where the actual privilege is needed. Also, it makes the script fetch network-lab by itself as needed rita.sh: * stop requiring root and start using sudo for the operations that actually need it * automatically download network-lab and change its path to "./network-lab/" * stop running "make install" for Babel .gitignore: * update test dependency location

# Conflicts: # integration-tests/rita.sh # rita/src/main.rs

jkilpatr

Looks good, getting way too big though @drozdziak1 @kingoflolz Rita dev priority #0 is getting this rebased and merged. No more dev on side branches until that. Otherwise this is just going to get worse and harder to review. Ping me when it's ready please and I'll take care of it.

drozdziak1

I don't like the fact that this PR introduces a ton of code with very little documentation. IMHO the sooner we focus on documentation and a thorough refactor the better

drozdziak1 · 2018-02-07T12:45:57Z

integration-tests/rita.sh

+./rita-deps.sh
+sudo python3 rita.py


Why were the test dependencies exported to another file? Can't see a purpose for standalone test-specific provisioning

good point, I'll move it back

drozdziak1 · 2018-02-07T12:48:07Z

num256/src/lib.rs

@@ -58,6 +67,13 @@ impl fmt::Display for Uint256 {
    }
 }

+impl fmt::Debug for Uint256 {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "Uint256 {}", self.to_string())


Why not just format it as 33429348759238759u256?

Could be good. I would personally prefer Uint256(3402342), but that's a matter of taste

went with Uint256(3402342) as I think its a bit more "rusty"

drozdziak1 · 2018-02-07T12:49:38Z

num256/src/lib.rs

@@ -162,6 +196,12 @@ impl fmt::Display for Int256 {
    }
 }

+impl fmt::Debug for Int256 {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "Int256 {}", self.to_string())


Why not just format as e.g. "239487234987i256"?

drozdziak1 · 2018-02-07T12:52:11Z

num256/src/lib.rs

+impl Div for Int256 {
+  type Output = Int256;
+  fn div(self, v: Int256) -> Int256 {
+    let num = self.0 / v.0;


Does the zero-division-related panic happen somewhere else? I don't mean the CheckedDiv trait, if we fail to panic on division by zero, the system will probably send us SIGFPE and I'm not sure that the Rust runtime can handle it and panic on it own - this might make debugging more time-consuming as there's a chance of the user not getting a backtrace

rust-lang/rfcs#1368
A quote from the issue above:
fwiw, I see no mention here of "thread-directed" signals. IMHO, it's reasonable to not support them as a first pass (they're a totally different thing than process-directed signals), but worth at least being explicit about what's in scope.

As an example of what these are: let's say I do a mmap of a file and try to read from it but another process has truncated the file so the page I'm reading is no longer valid. A SIGBUS is generated and directed to the thread doing the read. It can't be handled by some other thread using signalfd or the like.

Likewise, SIGFPE, SIGSEGV, SIGILL, SIGPIPE, and anything done with pthread_kill/pthread_sigqueue. The last case is sometimes used as a mechanism for CPU / wall clock profiling tools or thread cancellation.

This is a bigint, so I think it will panic instead of raising a hardware exception, but I have added div by zero checker

drozdziak1 · 2018-02-07T13:02:23Z

rita/payment_controller/src/lib.rs

        let bounty_url = if cfg!(not(test)) {
            format!("http://[{}]:8888/update", "2001::3".parse::<Ipv6Addr>().unwrap())
        } else {
-            String::from("http://127.0.0.1:1234") //TODO: This is mockito::SERVER_URL, but don't want to include the crate in a non-test build just for that string
+            String::from("http://127.0.0.1:1234/update") //TODO: This is mockito::SERVER_URL, but don't want to include the crate in a non-test build just for that string


This todo most likely should've become a GitHub issue

drozdziak1 · 2018-02-07T13:21:14Z

rita/src/main.rs

            }
+            payment_controller_input1 .send(PaymentControllerMsg::Update);


A likely unwanted space after the 1

drozdziak1 · 2018-02-07T13:22:26Z

rita/src/main.rs

+        Arc::new(Mutex::new(debt_keeper_input.clone()))
+    );
+
+    let payment_controller_input1 = payment_controller_input.clone();


Why is this variable numbered? If it's got a distinct purpose why doesn't the name express it?

It's a clone of a channel input. I would be curious what naming scheme you'd prefer. We could name it after what it was intended to be used by, but that might get very long winded.

I understand now, so there's two different components chatting with the payment controller? And shadowing the same variable name would get the borrow checke angry?

(or simply overwriting the variable)

I changed it so there is a master, which we clone, and just pass in variables which we just overwrite each time

drozdziak1 · 2018-02-07T13:23:48Z

rita/src/main.rs

    thread::spawn(move || {
        let mut ki = KernelInterface {};
        let mut tm = TunnelManager::new();
-        let mut babel = Babel::new(&"[::1]:8080".parse().unwrap()); //TODO: Do we really want [::1] and not [::0]?
+        let mut babel = Babel::new(&"[::1]:8080".parse().unwrap());


Why does Babel start on 8080? It's a well known port already widely used for user-controlled HTTP servers.

I mean, what if someone runs an exit and would like to also put up a status webpage on 80 and some unrelated program's admin panel on 8080?

To be fair, I don't really know. That's what @jtremback 's original test script was set to so I just continued using it.

drozdziak1 · 2018-02-07T13:25:45Z

rita/src/main.rs

-    let pc = PaymentController::start(&my_ident, m_tx);
-
-    let pc1 = pc.clone();
+    let payment_controller_input2 = payment_controller_input.clone();


Another numbered thing

drozdziak1 · 2018-02-07T13:29:39Z

rita/src/main.rs

-
-    for debt_adjustment in rx {
-        match dk.apply_debt(debt_adjustment.ident, debt_adjustment.amount) {
+    for msg in debt_keeper_output {


Why does this loop need to constantly roll, wouldn't it be better to block until something actually happens? That way this thread could just go to sleep on the socket within the debt keeper

Pretty sure this does block, it is iterating over the channel output

Oh, okay, my poor knowledge of Rust strikes again

jtremback and others added 30 commits January 24, 2018 22:59

implement Debug for num256

c3fdb7d

add to gitignore

31473ab

remove more useless logs

c934e20

clean up logs

3d496d0

refactor to more of a actor model

6c00bc7

WIP cleanup, fix bugs

d86e1a1

merge changes from patch

5e81251

fix bug with crediting instead of debiting neighbour

8be20f7

add bridge interface to nodes 1 and 3 in integration test

17c231a

add python script to create network topologies

578a39d

fleshing out rita.py, almost feature complete

12da134

add neighbour hello retry feature

cbd8e6c

make rita.sh use the new python script

ef57811

Make debt keeper tests pass

84957b7

fix test for payment controller

505b46e

add first test to python framework (reachability)

007f2cc

Add scaffolding for traffic accounting tests

4772f3b

Change where deps are pulled for network lab to my fork

57e3dc5

traffic gen improvements

40b57e6

Fix babel pricing quirk

7fc8b69

Refactor debtkeeper to enable easier addition of per node data

8ff0441

add fudge factor to debt calculations

e1fc00f

refix babel price quirk

1123cb4

Refactor payment controller for more consistent API

2537261

Merge branch 'fix-rita-billing' into improve-integration-tests

ad0190f

# Conflicts: # integration-tests/rita.sh # rita/src/main.rs

Send updates to bounty hunter occasionally

923957c

Add traffic test

3db79d3

Fix traffic accounting bugs and increase robustness of rita

a80f7f7

Add traffic integration tests

55e7ae0

kingoflolz added 9 commits February 5, 2018 23:19

Add more From impls to Int256

4a0cff9

Make traffic counters atomic

aeef816

Fix bugs in debt keeper and traffic watcher, make watching atomic

c81be23

updated integration test script

afff14d

implement debt buffering

524307b

update test cases

9f1e0aa

add suspension integration test

32ded9d

make travis run integration tests

2e3b074

make build more robust

d112c72

kingoflolz requested review from jtremback, drozdziak1 and jkilpatr February 7, 2018 10:05

kingoflolz mentioned this pull request Feb 7, 2018

Atomic replacement of rita traffic accounting rules #32

Closed

jkilpatr mentioned this pull request Feb 7, 2018

Fix payment loop #30

Closed

jkilpatr approved these changes Feb 7, 2018

View reviewed changes

drozdziak1 suggested changes Feb 7, 2018

View reviewed changes

fix nits

15d178b

jtremback merged commit 5f856be into master Feb 8, 2018

kingoflolz deleted the improve-integration-tests branch February 27, 2018 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve integration tests, fix bugs in rita #34

Improve integration tests, fix bugs in rita #34

kingoflolz commented Feb 7, 2018

jkilpatr left a comment

drozdziak1 left a comment

drozdziak1 Feb 7, 2018

kingoflolz Feb 7, 2018

drozdziak1 Feb 7, 2018

jtremback Feb 7, 2018

kingoflolz Feb 7, 2018

drozdziak1 Feb 7, 2018

drozdziak1 Feb 7, 2018

drozdziak1 Feb 7, 2018

kingoflolz Feb 7, 2018

drozdziak1 Feb 7, 2018

drozdziak1 Feb 7, 2018

drozdziak1 Feb 7, 2018

jtremback Feb 7, 2018

drozdziak1 Feb 7, 2018

drozdziak1 Feb 7, 2018

drozdziak1 Feb 7, 2018

kingoflolz Feb 7, 2018

drozdziak1 Feb 7, 2018

drozdziak1 Feb 7, 2018

kingoflolz Feb 7, 2018

drozdziak1 Feb 7, 2018

drozdziak1 Feb 7, 2018

jtremback Feb 7, 2018

drozdziak1 Feb 7, 2018

		}
		payment_controller_input1 .send(PaymentControllerMsg::Update);

Improve integration tests, fix bugs in rita #34

Improve integration tests, fix bugs in rita #34

Conversation

kingoflolz commented Feb 7, 2018

jkilpatr left a comment

Choose a reason for hiding this comment

drozdziak1 left a 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