-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Transaction parsing #5
Changes from all commits
063ce75
c1cce13
00e4c46
913fd8d
7d44250
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// | ||
var bcoin = require('bcoin'); | ||
var TrustIsRisk = require('./trust_is_risk'); | ||
|
||
class FullNode extends bcoin.fullnode { | ||
|
||
|
||
constructor(options ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's going on with the whitespace overall? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't check in compiled code. Instead, provide the script to compile it as part of your There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
super(options); | ||
this.trust = new TrustIsRisk(this); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
} | ||
|
||
module.exports = FullNode; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,5 @@ | ||
// | ||
var bcoin = require('bcoin'); | ||
|
||
class fullnode extends bcoin.fullnode { | ||
constructor(options ) { | ||
super(options); | ||
} | ||
} | ||
|
||
module.exports = { | ||
fullnode | ||
FullNode: require('./full_node'), | ||
TrustIsRisk: require('./trust_is_risk') | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,202 @@ | ||
// | ||
var bcoin = require('bcoin'); | ||
var assert = require('assert'); | ||
var Address = bcoin.primitives.Address; | ||
|
||
// base58 bitcoin address | ||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
class TrustIsRisk { | ||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
|
||
constructor(node ) { | ||
this.node = node; | ||
this.directTrust = {}; | ||
this.TXToTrust = {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comments for what these hold. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
this.node.on('tx', this.addTX.bind(this)); | ||
} | ||
|
||
getDirectTrust(from , to ) { | ||
if (!this.directTrust.hasOwnProperty(from)) return 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Always use { } even if your body is one line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if (!this.directTrust[from].hasOwnProperty(to)) return 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use in everywhere There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return this.directTrust[from][to]; | ||
} | ||
|
||
// Attempts to parse a bitcoin transaction as a trust change and adds it to the trust network | ||
// if successful. | ||
// Returns true if the transaction is a TIR transaction and was successfully added to the | ||
// network, false otherwise. | ||
// Throws an error if the transaction was processed earlier. | ||
addTX(tx ) { | ||
var txHash = tx.hash().toString('hex'); | ||
if (this.TXToTrust.hasOwnProperty(txHash)) { | ||
throw new Error('Duplicate TX: Transaction with hash ' + txHash + ' has been seen again.'); | ||
} | ||
|
||
var trustChanges = this.getTrustChanges(tx); | ||
if (trustChanges.length === 0) return false; | ||
else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid else-after-return, especially in initial error-checking cases like this. Maybe run jslint and enable the rules you'd like. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
trustChanges.map(this.applyTrustChange.bind(this)); | ||
return true; | ||
} | ||
} | ||
|
||
// Returns a list of trust changes that a transaction causes, which will be one of the following: | ||
// * An empty list (for non-TIR transactions). | ||
// * A list containing a single trust increase (for trust-increasing transactions). | ||
// * A list containing one or more trust decreases (for trust-decreasing transactions). | ||
getTrustChanges(tx ) { | ||
var trustIncrease = this.parseTXAsTrustIncrease(tx); | ||
if (trustIncrease !== null) { | ||
return [trustIncrease]; | ||
} else { | ||
return this.getTrustDecreases(tx); | ||
} | ||
} | ||
|
||
applyTrustChange(trustChange ) { | ||
if (!this.directTrust.hasOwnProperty(trustChange.from)) this.directTrust[trustChange.from] = {}; | ||
if (!this.directTrust[trustChange.from].hasOwnProperty(trustChange.to)) { | ||
this.directTrust[trustChange.from][trustChange.to] = 0 | ||
} | ||
|
||
this.directTrust[trustChange.from][trustChange.to] += trustChange.amount; | ||
|
||
if (this.directTrust[trustChange.from][trustChange.to] > 0) { | ||
this.TXToTrust[trustChange.txHash] = { | ||
from: trustChange.from, | ||
to: trustChange.to, | ||
amount: this.directTrust[trustChange.from][trustChange.to], | ||
txHash: trustChange.txHash, | ||
outputIndex: trustChange.outputIndex | ||
}; | ||
} | ||
} | ||
|
||
parseTXAsTrustIncrease(tx ) { | ||
if (tx.inputs.length !== 1) return null; | ||
if (tx.inputs[0].getType() !== 'pubkeyhash') return null; // TODO: This is unreliable | ||
if (this.TXToTrust[tx.inputs[0].prevout.hash.toString('hex')]) return null; | ||
var sender = tx.inputs[0].getAddress().toBase58(); | ||
|
||
if (tx.outputs.length === 0 || tx.outputs.length > 2) return null; | ||
|
||
var trustOutputs = this.searchForDirectTrustOutputs(tx, sender); | ||
if (trustOutputs.length !== 1) return null; | ||
|
||
var changeOutputCount = tx.outputs.filter((o) => this.isChangeOutput(o, sender)).length | ||
if (changeOutputCount + 1 !== tx.outputs.length) return null; | ||
|
||
return trustOutputs[0]; | ||
} | ||
|
||
getTrustDecreases(tx ) { | ||
var inputTrusts = this.getInputTrusts(tx.inputs); | ||
return inputTrusts.map(this.getTrustDecrease.bind(this, tx)); | ||
} | ||
|
||
getInputTrusts(inputs ) { | ||
return inputs.map((input) => { | ||
var trust = this.TXToTrust[input.prevout.hash.toString('hex')] | ||
if (trust && trust.outputIndex === input.prevout.index) return trust; | ||
else return null; | ||
}).filter(Boolean); | ||
} | ||
|
||
getTrustDecrease(tx , prevTrust ) { | ||
var txHash = tx.hash().toString('hex'); | ||
var nullifyTrust = { | ||
from: prevTrust.from, | ||
to: prevTrust.to, | ||
amount: -prevTrust.amount, | ||
txHash, | ||
outputIndex: null | ||
}; | ||
|
||
if (tx.inputs.length !== 1) return nullifyTrust; | ||
|
||
var trustOutputs = this.searchForDirectTrustOutputs(tx, prevTrust.from, prevTrust.to); | ||
if (trustOutputs.length != 1) return nullifyTrust; | ||
var nextTrust = trustOutputs[0]; | ||
|
||
assert(nextTrust.from === prevTrust.from); | ||
assert(nextTrust.to === prevTrust.to); | ||
|
||
var trustAmountChange = nextTrust.amount - prevTrust.amount; | ||
assert(trustAmountChange <= 0); | ||
return { | ||
from: nextTrust.from, | ||
to: nextTrust.to, | ||
amount: trustAmountChange, | ||
txHash, | ||
outputIndex: nextTrust.outputIndex | ||
} | ||
} | ||
|
||
// Looks for direct trust outputs that originate from a sender in an array of bitcoin outputs. | ||
// If the recipient parameter is set, it will limit the results only to the outputs being sent to | ||
// the recipient. | ||
searchForDirectTrustOutputs(tx , sender , recipient ) { | ||
var directTrusts = tx.outputs.map((output, outputIndex) => | ||
this.parseOutputAsDirectTrust(tx, outputIndex, sender) | ||
).filter(Boolean); | ||
|
||
if (recipient) { | ||
directTrusts.filter((trust) => trust.to === recipient); | ||
} | ||
|
||
return directTrusts; | ||
} | ||
|
||
isChangeOutput(output , sender ) { | ||
return (output.getType() === 'pubkeyhash') | ||
&& (output.getAddress().toBase58() === sender); | ||
} | ||
|
||
parseOutputAsDirectTrust(tx , outputIndex , sender ) | ||
{ | ||
var txHash = tx.hash().toString('hex'); | ||
var output = tx.outputs[outputIndex]; | ||
if (output.getType() !== 'multisig') return null; | ||
|
||
var addressA = Address.fromHash(bcoin.crypto.hash160(output.script.get(1))).toBase58() | ||
var addressB = Address.fromHash(bcoin.crypto.hash160(output.script.get(2))).toBase58(); | ||
|
||
if (addressA === addressB) return null; | ||
|
||
var recipient; | ||
if (addressA === sender) recipient = addressB; | ||
else if (addressB === sender) recipient = addressA; | ||
else return null; | ||
|
||
return { | ||
from: sender, | ||
to: recipient, | ||
amount: Number(output.value), | ||
txHash, | ||
outputIndex | ||
}; | ||
} | ||
|
||
} | ||
|
||
module.exports = TrustIsRisk; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems bad, can you debug why the testcases are running so long by building a minimal testcase? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
"prepublish": "npm run build", | ||
"typecheck": "flow check" | ||
}, | ||
|
@@ -27,7 +27,10 @@ | |
"devDependencies": { | ||
"flow-bin": "^0.45.0", | ||
"flow-remove-types": "^1.2.0", | ||
"should": "^11.2.1" | ||
"mocha": "^3.4.2", | ||
"should": "^11.2.1", | ||
"should-sinon": "0.0.5", | ||
"sinon": "^2.2.0" | ||
}, | ||
"dependencies": { | ||
"bcoin": "^1.0.0-beta.12" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
// @flow | ||
var bcoin = require('bcoin'); | ||
var TrustIsRisk = require('./trust_is_risk'); | ||
|
||
class FullNode extends bcoin.fullnode { | ||
trust : TrustIsRisk | ||
|
||
constructor(options : Object) { | ||
super(options); | ||
this.trust = new TrustIsRisk(this); | ||
} | ||
} | ||
|
||
module.exports = FullNode; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,5 @@ | ||
// @flow | ||
var bcoin = require('bcoin'); | ||
|
||
class fullnode extends bcoin.fullnode { | ||
constructor(options : Object) { | ||
super(options); | ||
} | ||
} | ||
|
||
module.exports = { | ||
fullnode | ||
FullNode: require('./full_node'), | ||
TrustIsRisk: require('./trust_is_risk') | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the file name, not the npm package name.