Transaction parsing #5

christosporios · 2017-05-16T19:57:22Z

For every TX, checks whether the transaction is a TIR transaction, parses it into a TrustChange as discussed in the spec document and applies it to the trust graph.

Unit tests for the public methods as well as an E2E test that verifies that the TIR full node is processing all transactions are included. Some tests for complicated TX cases are missing and are TODO.

dionyziz · 2017-05-24T07:26:14Z

Squash your refactoring changes into atomic commits: Each PR should have many commits like you have, but if a future commit undoes something of a previous commit, the commits should be squashed. If you have big commits that do many things, they should be split to several commits where each does one thing (but the code should work after every commit).

dionyziz · 2017-05-24T07:27:57Z

Rewrite your commit messages based on Chris Beam's recommendations. In particular, do not end your commits in "." and limit the length of your commit messages so that they don't wrap around (you can use the extensive description for that)

dionyziz · 2017-05-24T07:28:14Z

lib/full_node.js

+class FullNode extends bcoin.fullnode {
+
+
+  constructor(options         ) {


What's going on with the whitespace overall?

The flow-type annotated source code in src/ is compiled to lib/ by flow-remove-types, which replaces flow syntax with whitespace to make the code runnable.

Do not review the code in lib.

Don't check in compiled code. Instead, provide the script to compile it as part of your package.json.

The build script is already in package.json.

It's common to check in the compiled source code in compiled javascript libraries. I assume the reason is that a compiled javascript program is still cross-platform (unlike a binary), and that it's a bit more convenient to not have to install build tools (in this case Flow and flow-remove-types) when trying to run an older version of the library. On the other hand it does take up some space and looks ugly. I think it would be ideal if there were a way for git to treat the lib/ folder like it treats binaries (e.g. calculate no diffs).

dionyziz · 2017-05-24T07:29:32Z

lib/full_node.js

@@ -0,0 +1,14 @@
+//      
+var bcoin = require('bcoin');
+var TrustIsRisk = require('./trust_is_risk');


Popular JS libraries use - in their names. Let's use "trust-is-risk" for the npm package name? This would imply a similar name here.

This is the file name, not the npm package name.

dionyziz · 2017-05-24T07:36:09Z

lib/full_node.js

+
+  constructor(options         ) {
+    super(options);
+    this.trust = new TrustIsRisk(this);


Trust is risk requires a full node instance? Can you discuss this architectural decision? It seems that the TIR library may need less than that.

Possibly, however I thought we agreed that the initial version will be bitcoin full node. This is in the spec and was discussed in the review meeting.

Please note that you don't have to use the full node class provided here, you can just use the TIR class alone and connect it to any other type of node manually. We can provide a spv_node implementation after the initial version is working.

dionyziz · 2017-05-24T07:37:58Z

lib/trust_is_risk.js

+
+  getDirectTrust(from         , to         )          {
+    if (!this.directTrust.hasOwnProperty(from)) return 0;
+    if (!this.directTrust[from].hasOwnProperty(to)) return 0;


Use in everywhere

I think it's clearer to use hasOwnProperty here, because I only want to check if the object has the property set on itself, not in its prototype chain.

Practically however and in this specific case this would be the same, so I don't feel very strongly about this.

dionyziz · 2017-05-24T07:39:30Z

lib/trust_is_risk.js

+
+    var trustChanges = this.getTrustChanges(tx);
+    if (trustChanges.length === 0) return false;
+    else {


Avoid else-after-return, especially in initial error-checking cases like this. Maybe run jslint and enable the rules you'd like.

I disagree with this: I use else after return only sometimes, when it enhances readability. This is usually when I want to distinguish between two cases, none of which are erroneous. I don't use else after return in initial error checking cases.

In this particular case, I'm using else exactly because an empty trust changes list is not error, but one of the two things that can happen: either this tx is a TIR transaction, or it's not.

dionyziz · 2017-05-24T07:40:19Z

lib/trust_is_risk.js

+  }
+
+  getDirectTrust(from         , to         )          {
+    if (!this.directTrust.hasOwnProperty(from)) return 0;


Always use { } even if your body is one line.

I'm not sure if the risk outweights the readability gain. The risk of accidentally causing unintentional behavior that isn't very easily caught in tests and code reviews is very low. Also one-line if statements are allowed in many style rules (e.g. google style guide), and bcoin uses them as well.

dionyziz · 2017-05-24T07:42:37Z

src/trust_is_risk.js

@@ -0,0 +1,202 @@
+// @flow


Why is this source code checked in twice?

See comment above.

dionyziz · 2017-05-24T07:44:40Z

lib/trust_is_risk.js

+  constructor(node                 ) {
+    this.node = node;
+    this.directTrust = {};
+    this.TXToTrust = {};


Comments for what these hold.

There's type declarations in the source file under src/, which describe what types these will hold. If you still think that something is unclear and requires a comment, please comment on the specific member variable.

dionyziz · 2017-05-24T07:53:31Z

package.json

-    "should": "^11.2.1"
+    "should": "^11.2.1",
+    "should-sinon": "0.0.5",
+    "sinon": "^2.2.0"


dionyziz · 2017-05-24T07:54:42Z

package.json

@@ -5,7 +5,7 @@
  "main": "lib/index.js",
  "scripts": {
    "build": "flow-remove-types -d lib/ src/",
-    "test": "npm run build && mocha",
+    "test": "npm run build && mocha -t 5000",


This seems bad, can you debug why the testcases are running so long by building a minimal testcase?

The E2E regtest test takes a while to run in particular (~1.5s on my machine), so I'm setting the time limit to something higher to avoid flakiness on slower machines.

As we discussed in a meeting, this is because apparently I have to wait (sleep) until some bcoin event handlers fire. I haven't been able to find a better solution, and I think overall it's better to have the tests than remove them because they take a bit longer to run. We can discuss this again tonight.

dionyziz · 2017-05-24T07:59:38Z

test/helpers.js

+        + `0x${address.hash.toString('hex')} OP_EQUALVERIFY OP_CHECKSIG`);
+
+    return new bcoin.primitives.Output({script, value});
+  }


I'm surprised this helper isn't available as part of the standard bcoin js library. Perhaps we should patch it instead? It seems like it could belong there.

There is this: https://github.com/bcoin-org/bcoin/blob/master/lib/script/script.js#L1677 which I probably should have used to create the script, and then you can create an output from the script.

There doesn't seem to be a way to create a P2PKH output directly.

dionyziz · 2017-05-24T08:04:40Z

test/full_node.js

+
+  it('should call trust.addTX() on every transaction', async function() {
+    var sender = await testHelpers.getWallet(walletDB, 'sender');
+    var receiver = await testHelpers.getWallet(walletDB, 'receiver');


2nd parameter seems unused?

That parameter is the ID parameter of the wallet and needs to be unique. Here I'm setting it to something descriptive.

dionyziz · 2017-05-24T08:05:10Z

test/full_node.js

+    // Make the coin spendable.
+    consensus.COINBASE_MATURITY = 0;
+
+    await testHelpers.time(100);


Why do we need this and the next timeout? Why isn't it a timeout of 0?

We need this timeout because even though the node has been mined at this point, the next call will fail with "not enough balance" if we don't wait long enough.

We need the next timeout because the 'tx' event doesn't fire until after a few milliseconds on the bcoin.fullnode object.

Sleeping for 0 milliseconds would run all the events that are currently in the event queue and return to the test right after. This is not sufficient however, as the event handlers for those events may emit more events that will be added to the end of the event queue, and therefore processed after the test finishes.

dionyziz · 2017-05-24T08:07:11Z

test/trust_is_risk.js

+      should(tir.getDirectTrust(alice, bob)).equal(0);
+      should(tir.getDirectTrust(bob, alice)).equal(0);
+      should(tir.getDirectTrust("1JDfVQkZxMvRwM3Lc6LkDrpX55Ldk3JqND", alice)).equal(0);
+      should(tir.getDirectTrust(alice, "1JDfVQkZxMvRwM3Lc6LkDrpX55Ldk3JqND")).equal(0);


Use charlie instead of this string, since you already defined him as a constant

dionyziz · 2017-05-24T08:08:56Z

test/trust_is_risk.js

+    it('returns zero for two arbitary parties that do not trust each other', () => {
+      should(tir.getDirectTrust(alice, bob)).equal(0);
+      should(tir.getDirectTrust(bob, alice)).equal(0);
+      should(tir.getDirectTrust("1JDfVQkZxMvRwM3Lc6LkDrpX55Ldk3JqND", alice)).equal(0);


This is written in a more readable manner:

tir.getDirectTrust("1JDfVQkZxMvRwM3Lc6LkDrpX55Ldk3JqND", alice).should.equal(0)

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

Superseded by #7.

christosporios self-assigned this May 16, 2017

christosporios requested review from dionyziz and OrfeasLitos May 16, 2017 19:57

christosporios changed the title ~~Parse trust-increasing transactions~~ Transaction parsing May 19, 2017

dionyziz reviewed May 24, 2017

View reviewed changes

package.json Outdated

"should": "^11.2.1"

"should": "^11.2.1",

"should-sinon": "0.0.5",

"sinon": "^2.2.0"

Copy link

Member

dionyziz May 24, 2017

Choose a reason for hiding this comment

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

+mocha

dionyziz reviewed May 24, 2017

View reviewed changes

christosporios force-pushed the parse-tx branch from 5bf97b8 to e94fe00 Compare May 26, 2017 00:37

christosporios added 3 commits May 26, 2017 00:39

Test that transactions are being processed

063ce75

Implement trust-increasing transaction parsing

c1cce13

Implement TrustIsRisk.prototype.getDirect()

00e4c46

christosporios force-pushed the parse-tx branch from e94fe00 to 619ad9f Compare May 26, 2017 00:40

Implement trust decreasing transaction parsing

913fd8d

christosporios force-pushed the parse-tx branch from 619ad9f to 913fd8d Compare May 26, 2017 00:53

Use conscise should syntax

7d44250

christosporios closed this May 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transaction parsing #5

Transaction parsing #5

christosporios commented May 16, 2017 •

edited

Loading

dionyziz commented May 24, 2017

dionyziz commented May 24, 2017

dionyziz May 24, 2017

christosporios May 24, 2017

dionyziz May 24, 2017

christosporios May 24, 2017

dionyziz May 24, 2017

christosporios May 24, 2017

dionyziz May 24, 2017

christosporios May 24, 2017

dionyziz May 24, 2017 •

edited

Loading

christosporios May 24, 2017

dionyziz May 24, 2017 •

edited

Loading

christosporios May 24, 2017

dionyziz May 24, 2017 •

edited

Loading

christosporios May 26, 2017

dionyziz May 24, 2017

christosporios May 24, 2017

dionyziz May 24, 2017

christosporios May 24, 2017

dionyziz May 24, 2017

dionyziz May 24, 2017

christosporios May 24, 2017

dionyziz May 24, 2017

christosporios May 24, 2017

dionyziz May 24, 2017

christosporios May 26, 2017

dionyziz May 24, 2017

christosporios May 24, 2017 •

edited

Loading

dionyziz May 24, 2017 •

edited

Loading

dionyziz May 24, 2017

christosporios commented May 29, 2017

		class FullNode extends bcoin.fullnode {


		constructor(options ) {

Transaction parsing #5

Transaction parsing #5

Conversation

christosporios commented May 16, 2017 • edited Loading

dionyziz commented May 24, 2017

dionyziz commented May 24, 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

dionyziz May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dionyziz May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dionyziz May 24, 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

christosporios May 24, 2017 • edited Loading

Choose a reason for hiding this comment

dionyziz May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christosporios commented May 29, 2017

christosporios commented May 16, 2017 •

edited

Loading

dionyziz May 24, 2017 •

edited

Loading

dionyziz May 24, 2017 •

edited

Loading

dionyziz May 24, 2017 •

edited

Loading

christosporios May 24, 2017 •

edited

Loading

dionyziz May 24, 2017 •

edited

Loading