Bitcoin SPV, Funding Proof and Electrum Client #8

nkuba · 2019-07-24T11:14:29Z

This PR contains:

Bitcoin SPV implementation in JS,
Electrum client to use to get data from bitcoin chain,
Bitcoin transaction parser,
Scripts to get funding proof details required by tBTC.

Closes: #4
Closes: #9

JS implementation of merkle.py script from [summa-tx/bitcoin-spv] repository.

Changed manual parsing of transaction to usage of bcoin libraries. This gives more flexibility and greater cases coverage.

Each library is an independent module located in lib/ directory. They can later be extracted to a separate repositories if needed. client/ directory contains dapp facing functions. Dapp should be calling functions exposed in the client/.

Converter to calculate an address for given chain (testnet, mainnet) from a 64 byte public key.

liamzebedee

😄 awesome turnaround with this

liamzebedee · 2019-07-24T12:59:44Z

.eslintrc

@@ -0,0 +1,17 @@
+{


FYI @Shadowfiend this was another reason why the linting is setup like this. Separate .eslint configs, because the frontend uses funky React syntax (JSX).

liamzebedee · 2019-07-24T13:00:54Z

client/package.json

+    "version": "0.0.1",
+    "description": "",
+    "scripts": {
+        "test": "CONFIG_FILE=${npm_package_config_file} mocha --timeout 5000"


Don't forget the lint commands! You can use the same lint config from tbtc:

"js:lint": "eslint ${npm_package_config_eslintPaths}", "js:lint:fix": "eslint --fix ${npm_package_config_eslintPaths}", }, "config": { "eslintPaths": "test/ migrations/ demo/" },

Also make sure to only specify the eslintPaths where these changes lie :). The frontend code won't be linted yet, there's some additional work with parsing JSX.

liamzebedee · 2019-07-24T13:02:32Z

client/test/FundingProofTest.js

+
+  it('getProof', async () => {
+    const proofFile = fs.readFileSync('./test/data/proof.json', 'utf8')
+    expectedResult = JSON.parse(proofFile)


expectedResult referenced before definition? Add a const?

liamzebedee · 2019-07-24T13:05:18Z

lib/bitcoin-spv/index.js

+const ProofVerification = require('./src/ProofVerification')
+
+module.exports = BitcoinSPV
+module.exports.verifyProof = ProofVerification


Preferred syntax:

module.exports = { BitcoinSPV, verifyProof: ProofVerification }

It would be better not to merge namespaces/objects, since JS is so loosely typed.

It would be better not to merge namespaces/objects, since JS is so loosely typed.

What do you mean? How can we improve?

So JS is a terrible language for doing magic with, principally because it just keeps chugging along no matter how illogical something seems. This is why the whole "immutable" movement has been gaining a lot of steam - JS has different semantics for getting properties, setting properties.

I'll give some examples:

let obj = {} // differences in error semantics // fail silently obj.xyz === undefined obj['xyz'] === undefined // fail properly obj.xyz.abc // VM70:1 Uncaught TypeError: Cannot read property 'abc' of undefined // at <anonymous>:1:9 let obj2 = { abc: 1 } obj.xyz = obj2 // reference semantics obj.xyz.abc === 1 obj2.xyz = 2 obj.xyz.abc === 2 // value semantics // this will probably surprise you obj2 = { abc: 5 } obj.xyz.abc === 2

In the frontend world there are two major trends:

Making things declarative, rather than imperative

Making things immutable by default

module.exports = { BitcoinSPV, verifyProof: ProofVerification }

^^ the above is an example of embodying the first principle, and making way for the second. It declares the exports once, which makes the code declarative (there's one simple place where it all happens at once).

liamzebedee · 2019-07-24T13:08:09Z

lib/bitcoin-spv/src/BitcoinSPV.js

+
+    this.close()
+
+    return Promise.resolve({


It should work with plain return {}

liamzebedee · 2019-07-24T13:39:31Z

lib/electrum-client/src/ElectrumClient.js

+  }
+
+  getMerkleRoot(blockHeight) {
+    return new Promise((resolve, reject) => {


Is there a reason that the syntax here is different to async/await above?

Yes, me learning 😉 I'll update it to async/await.

liamzebedee · 2019-07-24T13:40:27Z

lib/electrum-client/src/ElectrumClient.js

+          this.getMerkleRoot(blockHeight)
+            .then((merkleRoot) => {
+              proof = Buffer.concat([proof, fromHex(merkleRoot)])
+              resolve({ 'proof': toHex(proof), 'position': position })


Prefer prop keys without string quotes '

liamzebedee · 2019-07-24T13:42:02Z

lib/electrum-client/src/ElectrumClient.js

+          outputs.forEach((output, index) => {
+            output.scriptPubKey.addresses.forEach((a) => {
+              if (a == address) {
+                resolve(index)


This will call resolve twice potentially, which can get messy. A less brittle approach could be for(let item of array) {} and then returning from the loop.

Won't it resolve on a first hit?

Yes but it will also:

continue looping

call resolve again, potentially, which would error as "promise already resolved", probably with no context/trace

liamzebedee · 2019-07-24T13:44:39Z

lib/electrum-client/test/ElectrumClientTest.js

+const chai = require('chai')
+const assert = chai.assert
+
+const config = Config.readFile(process.env.CONFIG_FILE)


Why not just directly require the config if it's only used in this test? Seems like we've got a couple of layers of indirection going on here?

Do you mean this:
const config = require('../src/Config').readFile(process.env.CONFIG_FILE)?

Protip! if it's JSON, require will parse it for you ;) no need for readFile, just require it like a js file

Notable, require caches its results. Probably fine here, but keep in mind if you ever need to reload your config, require is not the way to go.

liamzebedee · 2019-07-24T13:47:17Z

lib/electrum-client/test/ElectrumClientTest.js

+
+    describe('findOutputForAddress', async () => {
+      afterEach(() => {
+        assert.equal(


Agree with Piotr, let's prefer putting the assertions within the it block, and only do setup in before and after. If it gets greater than one line, then extract it to a common function.

To execute tests in submodules run `npm run test` in main directory.

nkuba · 2019-07-25T10:56:11Z

@liamzebedee thank you for your valuable inputs! I've learned a lot!

Decided to merge libs into one package for easier development. These are really small modules. We can consider extracting them in the future.

nkuba · 2019-07-29T06:53:47Z

@liamzebedee code updated.

Replaced electrum-client dependency with electrum js.

- extract config file to a variable instead of requiring it as a parameter - add docs

liamzebedee

Good stuff, only minor things.

lib/tbtc-helpers/src/Address.js

liamzebedee · 2019-07-29T10:16:00Z

lib/tbtc-helpers/src/ElectrumClient.js

@@ -1,4 +1,4 @@
-const ElectrumCli = require('electrum-client')
+const electrumjs = require('electrumjs')


I was searching for the docs on this pkg. Seems quite early:

Note that this is a work in progress and it is not ready for production whatsoever. The package has not even been published to NPM yet. Proceed at your own risk.

It was last updated 18 days ago on NPM. Code looks good, they're generating API's from a template. Nonetheless, I'm slightly worried they might push and break it accidentally. Would it be a good idea to fork it to keep-network, and install from git:// instead of NPM?

Yeah, we will fork it for sure. We already forked a previous electrum-client and I discovered that it doesn't meat all our needs. I didn't want to fork another one before testing that it really covers what we want in all steps.

lib/tbtc-helpers/src/ElectrumClient.js

client/src/Deposit.js

liamzebedee · 2019-07-29T10:25:04Z

client/src/FundingTransaction.js

@@ -0,0 +1,24 @@
+// PAGE 3: Pay BTC


getAddress is one I've started implementing.

None of the BTC functions are implemented, only ETH-related ones. But I have started building a little skeleton since there is crossover in the flow.

Mentioning this because while it's not necessarily related to the review, it's easier (?) to communicate information here with context than in Flowdock with no code. :)

Get address will be implemented in #14

Nice one with implementing ETH part!
We can use it to get a public key and then call https://github.com/keep-network/tbtc-dapp/blob/funding/lib/tbtc-helpers/src/Address.js#L18 to convert a public key to a BTC address.

client/test/FundingProofTest.js

client/src/FundingProof.js

nkuba · 2019-07-29T15:25:35Z

@liamzebedee ready for a next round.

lib/tbtc-helpers/src/ElectrumClient.js

nkuba · 2019-07-30T12:48:26Z

@liamzebedee ready for a next round.

Arc Reactor: Migrate tbtc.js to the latest tBTC contracts

nkuba added 14 commits July 24, 2019 11:02

dapp initial package config

ac441e2

ElectrumX JS client

18b8a7e

Bitcoin SPV JS implementation

3ef3857

JS implementation of merkle.py script from [summa-tx/bitcoin-spv] repository.

Bitcoin transaction parser

8cfd877

Script for Funding Proof

2651ff8

Initial main script

811af2d

Utils scripts

38a74a6

Bitcoin TX test data

be35a6a

Update dependency to electrum-client version

79cede8

Used bcoin libraries for transaction parsing

faeb58b

Changed manual parsing of transaction to usage of bcoin libraries. This gives more flexibility and greater cases coverage.

Add dependencies to bcoin and bufio

7899a95

Added case for legacy tx to parser test

95c81ff

Reorganized structure of the modules

c909fa8

Each library is an independent module located in lib/ directory. They can later be extracted to a separate repositories if needed. client/ directory contains dapp facing functions. Dapp should be calling functions exposed in the client/.

Added .gitignore

c82cc2c

nkuba mentioned this pull request Jul 24, 2019

Bitcoin SPV, Funding Proof and Electrum Client - all in JS keep-network/tbtc#220

Closed

1 task

nkuba requested a review from a team July 24, 2019 11:17

nkuba assigned liamzebedee Jul 24, 2019

Public key to address converter

5eccbea

Converter to calculate an address for given chain (testnet, mainnet) from a 64 byte public key.

liamzebedee suggested changes Jul 24, 2019

View reviewed changes

nkuba added 7 commits July 25, 2019 09:34

Add eslint to each package

15b2517

Main package with submodules

4502cd7

To execute tests in submodules run `npm run test` in main directory.

Refacotr BitcoinSPV to drop class

1700fcf

Remove unneded file

05c262a

ChainType -> Network

852f091

Update electrum client after review

9838f60

Update bitcoin utils exports

e22ecc9

nkuba requested a review from liamzebedee July 25, 2019 10:56

nkuba added 2 commits July 25, 2019 13:01

Removed Config script

b69025b

Merged libs into one package

56b1602

Decided to merge libs into one package for easier development. These are really small modules. We can consider extracting them in the future.

liamzebedee mentioned this pull request Jul 26, 2019

Web3 integration #11

Closed

nkuba added 3 commits July 29, 2019 08:42

Remove commas

bf4e03c

Return from loop in find output for address

129d115

Comment about confirmations value

db7e7f9

nkuba added 8 commits July 29, 2019 10:36

Replace dependency electrum-client with electrumjs

7cf4471

Replaced electrum-client dependency with electrum js.

Connection details to tbtc electrum server

68b0fe0

Scripts stub interfaces

9ea6d93

Updates to funding proof script

cbe5d6c

- extract config file to a variable instead of requiring it as a parameter - add docs

Update proof test data

253756e

rewire dependency for tests

c0917b4

Update Address documentation

d7400c2

Docs for Bitcoin SPV

4aa6a47

liamzebedee suggested changes Jul 29, 2019

View reviewed changes

Documentation for electrum client

5b2860e

liamzebedee suggested changes Jul 29, 2019

View reviewed changes

lib/tbtc-helpers/src/ElectrumClient.js Outdated Show resolved Hide resolved

nkuba mentioned this pull request Jul 30, 2019

Consider adaptive required accumulated difficulty keep-network/tbtc#223

Open

build->built

2b65fd8

nkuba requested a review from liamzebedee July 30, 2019 12:48

liamzebedee approved these changes Jul 30, 2019

View reviewed changes

liamzebedee merged commit 3e9e0ac into master Jul 30, 2019

Shadowfiend deleted the funding branch July 30, 2019 13:56

liamzebedee mentioned this pull request Jul 31, 2019

Setup dApp skeleton #7

Closed

nkuba mentioned this pull request Aug 3, 2019

Make a deposit (contract code) #24

Merged

pdyraga assigned nkuba Aug 5, 2019

liamzebedee added a commit that referenced this pull request Apr 20, 2020

Merge pull request #8 from keep-network/arc-reactor

5331a6b

Arc Reactor: Migrate tbtc.js to the latest tBTC contracts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bitcoin SPV, Funding Proof and Electrum Client #8

Bitcoin SPV, Funding Proof and Electrum Client #8

nkuba commented Jul 24, 2019 •

edited

Loading

liamzebedee left a comment

liamzebedee Jul 24, 2019

liamzebedee Jul 24, 2019

liamzebedee Jul 24, 2019

liamzebedee Jul 24, 2019

nkuba Jul 25, 2019

liamzebedee Jul 25, 2019 •

edited

Loading

liamzebedee Jul 24, 2019

liamzebedee Jul 24, 2019

nkuba Jul 25, 2019

liamzebedee Jul 24, 2019

liamzebedee Jul 24, 2019

nkuba Jul 25, 2019

liamzebedee Jul 25, 2019

liamzebedee Jul 24, 2019

nkuba Jul 25, 2019

liamzebedee Jul 25, 2019

Shadowfiend Jul 25, 2019

liamzebedee Jul 24, 2019

nkuba commented Jul 25, 2019

nkuba commented Jul 29, 2019

liamzebedee left a comment

liamzebedee Jul 29, 2019

nkuba Jul 29, 2019

liamzebedee Jul 29, 2019

liamzebedee Jul 29, 2019

nkuba Jul 29, 2019

nkuba Jul 29, 2019

nkuba commented Jul 29, 2019

nkuba commented Jul 30, 2019 •

edited

Loading

		@@ -1,4 +1,4 @@
		const ElectrumCli = require('electrum-client')
		const electrumjs = require('electrumjs')

Bitcoin SPV, Funding Proof and Electrum Client #8

Bitcoin SPV, Funding Proof and Electrum Client #8

Conversation

nkuba commented Jul 24, 2019 • edited Loading

liamzebedee 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

liamzebedee Jul 25, 2019 • 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

nkuba commented Jul 25, 2019

nkuba commented Jul 29, 2019

liamzebedee 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

nkuba commented Jul 29, 2019

nkuba commented Jul 30, 2019 • edited Loading

nkuba commented Jul 24, 2019 •

edited

Loading

liamzebedee Jul 25, 2019 •

edited

Loading

nkuba commented Jul 30, 2019 •

edited

Loading