Initial implementation #7

christosporios · 2017-05-29T19:39:04Z

This PR implements the following:

Transaction parsing: All trust transactions seen by the node are translated into direct trusts.
Maximum Flow Trust Inference: Indirect trust can be calculated between any two entities.
Transaction Creation: Trust increasing, trust decreasing and trust stealing transaction can be created.

Unit and E2E tests for the above are included.

dionyziz · 2017-06-01T16:48:21Z

type annotations in tests

dionyziz · 2017-06-01T15:49:19Z

.eslintrc.json

+			"onlyFilesWithFlowAnnotation": true
+		}
+	}
+}


dionyziz · 2017-06-01T15:49:30Z

.travis.yml

+script:
+  - npm run lint
+  - npm run typecheck
+  - npm test


npm run check

dionyziz · 2017-06-01T15:50:17Z

package.json

@@ -25,11 +27,19 @@
  },


dionyziz · 2017-06-01T16:26:22Z

test/helpers.js

+    return node.getBlock(node.chain.tip.hash);
+  },
+
+  time: async (milliseconds) => {


rename to delay

dionyziz · 2017-06-01T16:33:31Z

test/helpers.js

+    "0503054CF7EBB4E62191AF1D8DE97945178D3F465EE88EF1FB4E80A70CB4A49A",
+    "878DFE5B43AC858EA37B3A9EEBA9E244F1848A30F78B2E5AC5B3EBDE81AC7D45",
+    "1349A1318B1426E6F724CBFE7ECD2C46008A364A96C4BD20C83FC1C4EBB2EB4A"
+  ].map((key) => KeyRing.fromPrivate(new Buffer(key, "hex"))),


Move to fixture file

dionyziz · 2017-06-01T23:46:50Z

src/trust_is_risk.js

+    var trustIncrease = this.parseTXAsTrustIncrease(tx);
+    if (trustIncrease !== null) {
+      return [trustIncrease];
+    } else {


avoid else-after-return

i also wonder why programmers do this

Same as above. Throughout the code I omit the else only when it doesn't help convey the meaning of the code, for example when guarding against error cases. Here there's two equally valid cases: either it's a trust increase or it's not.

@gubatron your comment is not very constructive.

dionyziz · 2017-06-01T23:50:34Z

src/trust_is_risk.js

+      ]
+    });
+
+    var changeAmount = coin.value - trustAmount - fee;


assert(changeAmount >= 0)

Why? It's okay to have no change.

Assert that change is not negative

dionyziz · 2017-06-01T23:58:42Z

src/trust_is_risk.js

+    var mtx = new MTX({
+      outputs: [
+        new Output({
+          script: bcoin.script.fromMultisig(1, 2, [originPubKey, dest]),


check that origin != dest

dionyziz · 2017-06-02T00:01:05Z

src/trust_is_risk.js

+    var directTrusts = this.db.getSpendableDirectTrusts(originAddress, destAddress);
+    return directTrusts.map((directTrust) => {
+      var decrease = Math.min(trustDecreaseAmount, directTrust.amount);
+      if (decrease === 0) return null;


Perhaps a for loop with a break here would be more readable, also avoiding the filter(Boolean) part – and as a side effect it will also avoid unnecessary iterations.

I'm having trouble making flow type the resulting array correctly when I do that. Will investigate further.

Apparently flow isn't that smart, can't get anything other than filter(Boolean) to work.

dionyziz · 2017-06-02T00:17:42Z

src/trust_is_risk.js

+      ],
+      outputs: [new Output({
+        script: bcoin.script.fromPubkeyhash(Address.fromBase58(payee).hash),
+        value: decreaseAmount - fee


Check that this is positive. If not, make this zero? Throwing an exception may not be appropriate: If multiple txs are spent to achieve the decrease, the last one may be borderline below the fee.

dionyziz · 2017-06-08T11:18:59Z

src/trust_db.js

+  }
+
+  getDirectTrustByOutpoint(outpoint : bcoin$Outpoint) : (DirectTrust | null) {
+    var trust = this.txToDirectTrust.get(outpoint.hash.toString("hex"));


Use getDirectTrustByTx

dionyziz · 2017-06-08T11:23:37Z

src/trust_db.js

+    return trusts;
+  }
+
+  getGraphWeightMatrix() : number[][] {


If more readable, then move graphWeightMatrix to class variable and updated it when add is called.

dionyziz · 2017-06-08T11:50:28Z

src/trust_is_risk.js

+    if (tx.inputs.length !== 1) return null;
+    var input = tx.inputs[0];
+    if (input.getType() !== "pubkeyhash") return null; // TODO: This is unreliable
+    if (this.db.isTrustOutput(input.prevout.hash.toString("hex"), input.prevout.index)) return null;


This condition is unnecessary, because if null is returned here, then also null is returned in line 191.

dionyziz · 2017-06-08T11:50:45Z

src/trust_is_risk.js

+    var input = tx.inputs[0];
+    if (input.getType() !== "pubkeyhash") return null; // TODO: This is unreliable
+    if (this.db.isTrustOutput(input.prevout.hash.toString("hex"), input.prevout.index)) return null;
+    var origin = tx.inputs[0].getAddress().toBase58();


Explain the cases this getAddress can return (p2pkh or something else) in a comment

dionyziz · 2017-06-08T11:54:42Z

src/trust_is_risk.js

+    if (this.db.isTrustOutput(input.prevout.hash.toString("hex"), input.prevout.index)) return null;
+    var origin = tx.inputs[0].getAddress().toBase58();
+
+    if (tx.outputs.length === 0 || tx.outputs.length > 2) return null;


unnecessary

It breaks the following test if I remove it, which makes sense:

.addTX() with a trust increasing transaction which has two change outputs does not change trust

Are you saying that we could relax this condition and allow trust increasing transactions with more than one change outputs?

dionyziz · 2017-06-08T11:59:12Z

src/trust_is_risk.js

+    ).filter(Boolean);
+
+    if (recipient) {
+      directTrusts.filter((trust) => trust.dest === recipient);


filter doesn't modify underlying array – please add more regression tests

Good catch, this was a major bug. I've added a test to protect against it and fixed the problem.

dionyziz · 2017-06-08T12:05:33Z

src/trust_is_risk.js

+
+  getInputTrusts(inputs : bcoin$Input[]) : DirectTrust[] {
+    return inputs.map((input) => {
+      var trust = this.db.getDirectTrustByOutpoint(input.prevout);


Think about & create github issue about malleability

dionyziz · 2017-06-08T12:05:47Z

src/trust_is_risk.js

+  getInputTrusts(inputs : bcoin$Input[]) : DirectTrust[] {
+    return inputs.map((input) => {
+      var trust = this.db.getDirectTrustByOutpoint(input.prevout);
+      if (trust && trust.outputIndex === input.prevout.index) return trust;


trust.outputIndex === input.prevout.index unnecessary

dionyziz · 2017-06-08T12:06:10Z

src/trust_is_risk.js

+    return inputs.map((input) => {
+      var trust = this.db.getDirectTrustByOutpoint(input.prevout);
+      if (trust && trust.outputIndex === input.prevout.index) return trust;
+      else return null;


You can just return trust;

Nice, done.

dionyziz · 2017-06-08T12:10:45Z

src/trust_is_risk.js

+
+    nextTrust.prev = prevTrust;
+
+    assert(nextTrust.amount - prevTrust.amount <= 0);


more readable as nextTrust.amount <= prevTrust.amount

…into tx-creation

Some minor change requests left. Opened issue #14.

christosporios · 2017-09-11T23:38:15Z

Most CR comments have been addressed, let's merge this ASAP so that Orfeas can continue. The remaining comments are mostly readability related and I've opened #14 for them.

OrfeasLitos · 2017-09-20T17:00:33Z

src/trust_is_risk.js

+  }
+
+  getTrustDecreasingMTX(directTrust : DirectTrust, decreaseAmount : number, payee : ?Entity,
+      signingKeyRing : bcoin$KeyRing, fee : ?number) {


Add return type
: bcoin$MTX

dionyziz · 2017-09-26T20:27:00Z

.eslintrc.json

+	"extends": [
+    "eslint:recommended",
+    "plugin:flowtype/recommended"
+  ],


nit: indentation issues

OrfeasLitos · 2017-09-26T20:27:19Z

.eslintrc.json

@@ -0,0 +1,40 @@
+{


Replace tabs with spaces in file

christosporios added 14 commits May 26, 2017 00:39

Test that transactions are being processed

063ce75

Implement trust-increasing transaction parsing

c1cce13

Implement TrustIsRisk.prototype.getDirect()

00e4c46

Implement trust decreasing transaction parsing

913fd8d

Implement MaxFlow-based indirect trust inference

14b48e5

Implement .getTrustIncreasingMTX()

e7e4f84

Use conscise should syntax

7d44250

Implement .getTrustDecreasingMTXs()

23d9a4b

Implement trust stealing

bdee7b0

Implement a complete end to end regtest test

885d11f

Use ESLint, fix style issues

eeff57a

Refactor: use consistent variable names

d28d1b6

Add Travis CI

22fa3c4

Merge branch 'parse-tx' into tx-creation

a863ee7

christosporios mentioned this pull request May 29, 2017

Transaction parsing #5

Closed

Fix indentation in .eslintrc

0c7d90d

christosporios requested review from dionyziz and OrfeasLitos May 29, 2017 20:00

christosporios mentioned this pull request May 29, 2017

Travis #6

Closed

dionyziz requested changes Jun 2, 2017

View reviewed changes

dionyziz previously requested changes Jun 8, 2017

View reviewed changes

christosporios added 7 commits July 11, 2017 19:13

Code review responses

9e2f03c

Merge branch 'tx-creation' of github.com:decrypto-org/TrustIsRisk.js …

2feb00a

…into tx-creation

Move fixtures to separate fixtures file.

d9d073a

Remove compiled files from repo; Minor code review fixes

8df2ff5

More CR responses.

86a512c

More CR fixes.

8f25118

Fix bcoin version 1.0.0-beta.12

ba908f5

christosporios force-pushed the tx-creation branch from 259958b to ba908f5 Compare September 11, 2017 23:30

christosporios mentioned this pull request Sep 11, 2017

Respond to the remaining comments of PR #7 #14

Open

OrfeasLitos reviewed Sep 20, 2017

View reviewed changes

OrfeasLitos mentioned this pull request Sep 22, 2017

Adapt for SPV #16

Closed

dionyziz requested changes Sep 26, 2017

View reviewed changes

.eslintrc.json

"extends": [

"eslint:recommended",

"plugin:flowtype/recommended"

],

Copy link

Member

dionyziz Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation issues

OrfeasLitos reviewed Sep 26, 2017

View reviewed changes

.eslintrc.json

@@ -0,0 +1,40 @@

{

Copy link

Member

OrfeasLitos Sep 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace tabs with spaces in file


		nextTrust.prev = prevTrust;

		assert(nextTrust.amount - prevTrust.amount <= 0);

Initial implementation #7

Are you sure you want to change the base?

Initial implementation #7

Conversation

christosporios commented May 29, 2017 • edited Loading

dionyziz commented Jun 1, 2017

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

christosporios Jul 11, 2017 • edited Loading

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

christosporios commented Sep 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christosporios commented May 29, 2017 •

edited

Loading

christosporios Jul 11, 2017 •

edited

Loading