Replace Insight with Bitcoin Core in BlockchainWriter #408

lautarodragan · 2018-09-06T07:52:07Z

Fixes #24
Fixes #93
Partly addresses #293
needed-by #22
needed-by #25
needed-by #27

Release

Semantic release is enabled for this repository. Make sure you follow the right commit message convention.
We're using semantic-release's default — Angular Commit Message Conventions.

Description of Changes

This PR replaces the Insight API dependency of the BlockchainWriter module with the Bitcoin Core Wallet, finishing the migration.

Docker Compose

A service for regtest was added.
The testnet service was renamed and its ports were fixed

Dependencies

bitcore-lib and @types/bitcore-lib were uninstalled.

Deleted Tests

Placeholder tests that were introduced to bypass an error in coverage were deleted.

Rename

This PR started replacing timestamp with anchor. See #293.

Configuration

The default configuration now points to regtest.
insightUrl, bitcoinAddress and bitcoinAddressPrivateKey were removed.
No new configuration entries were needed. The onces added in Replace Insight with Bitcoin Core in BlockchainReader #383 sufficed.

Fee Estimation

The previous could would always send a fee of 0.001. This new code is using the fundrawtransaction RPC, which estimates fees based on recent blocks.

How to Test

This is how I tested this change:

sudo docker-compose up -d bitcoin-regtest
db.dropDatabase()
npm run build
npm start
npm run test:integration
GET http://localhost:18080/works
Verify that response contains a few items.
After a configured amount of time these items should show timestamp.ipfsFileHash, timestamp.ipfsDirectoryHash and transactionId. These three properties will appear one by one. Keep refreshing until the show.
bitcoin-cli generate 1
Wait the configured amount of time until BlockchainReader picks up the generated block
GET http://localhost:18080/works again
The works in the response should now contain a full timestamp, with blockHash, blockHeight, etc.

I ran this test both in regtest and testnet.

One ideal test I haven't done is node syncing: deploy this code and the dependencies to two different machines and confirm they sync correctly.

Things Being Left Out

I'm leaving out of this PR a few things since it's actually working as it is and it's blocking several other PRs.

Update documentation
BlockchainWriter README #157
Unit tests
Possible refactors or code improvements

lautarodragan · 2018-09-08T11:51:48Z

src/BlockchainWriter/ClaimController.ts


-    if (!utxo || !utxo.length)
-      throw new Error(`Wallet seems to be empty. Check funds for ${this.configuration.bitcoinAddress}`)
+  private anchorData = async (data: string) => {


I experimented with pipeP for this but decided not to use it yet.

I've left the experiments in another branch.

Agreed, it probably requires to much refactoring to use it nicely atm.

kennylavender · 2018-09-10T16:41:21Z

src/BlockchainWriter/Bitcoin.ts

@@ -0,0 +1,2 @@
+export const getData = (prefix: string, version: ReadonlyArray<number>) => (message: string) =>


Can/Should this be unit tested?

Yes. Will add in other PR

kennylavender

Missing 1 unit test possibly above src/BlockchainWriter/Bitcoind.ts

I was unable to manually test, I was getting Insufficient Funds when trying to use regtest mode to manually test transactions.

Could use some documentation on adding funds to the regest docker container and anything else that needs to be done with regtest to successfully read/write.

Remaining code changes look good. Il will take a second look soon to make sure I didn't miss anything.

lautarodragan · 2018-09-10T18:46:09Z

Will add proper docs in a new PR.

For now: in regtest, generate should give you funds. Remember to generate at least 101 blocks since mined/coinbase bitcoins cannot be used until they have 100 confirmations.

geoffturk · 2018-09-10T19:59:32Z

docker-compose.yml

@@ -41,21 +41,30 @@ services:
        sleep 10;
        npm start;
      "
-  poet-bitcoind:
+  bitcoin-testnet:


I don't see the need to stand up a testnet docker instance anymore now that we are using regtest. If that is correct, can we either comment this out or remove it? Otherwise when I run docker up --build I am starting up a testnet instance and syncing with the testnet blockchain, thus using up bandwidth and storage for no real reason.

Good point @geoffturk. I can't think of a situation that I would still want a testnet docker instance locally.

Actually, now that I thought about it more, I think we do want a mainnet/testnet instance actually for people to run the node on their machines. Maybe we just need a simpler way of expressing which instance we run instead of running all of them, but that can be tackled in a future PR.

That's correct, I'll split it apart in different docker-compose files. We still want to test stuff in testnet if possible, but docker up shouldn't cause you to download the whole testnet.

kennylavender

N/A

Documentation

Just succesfully manual tested 2 claims from write to read! Just the unit test.

kennylavender

Approving so you can add the unit tests then merge tonight hopefully.

jwicks31 · 2018-09-11T00:41:15Z

docker-compose.yml

@@ -36,27 +36,20 @@ services:
      - poet-mongo
      - poet-rabbit
      - poet-ipfs
+      - bitcoin


should this be bitcoin-regtest?

yes! good catch.

lautarodragan · 2018-09-11T03:03:15Z

@kennylavender @geoffturk @jwicks31 merging now not to block other PRs. I think I've addressed your reviews, but please reach out / raise issues if there's changes you think should be done to this PR.

poet-ci · 2018-09-11T03:10:00Z

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

lautarodragan added 7 commits September 6, 2018 04:46

delete useless tests

764ad6c

more bitcoin-core typings

4bb4ac7

docker-compose bitcoin-regtest

b8c1c01

UnspentOutput interface

416e841

basic but actually working thing

a6c50c3

npm uninstall bitcore-lib

ed25213

npm uninstall @types/bitcore-lib

07451fc

lautarodragan added don't merge in progress labels Sep 6, 2018

lautarodragan requested a review from a team as a code owner September 6, 2018 07:52

lautarodragan added 8 commits September 6, 2018 16:56

lint

d81974d

estimate fee wip

6d4c7f9

revert ramda.d.ts

cdbe35a

improve bitcoin-core.d.ts

e47b399

default bitcoin rpc config to regtest

d9a4884

use fundrawtransaction + small refactor

9a71119

Merge branch 'master' into bitcoin-core-writer

e7656b2

rename poet-bitcoind to bitcoin-testnet in docker-compose, fix ports

35c5926

lautarodragan removed don't merge in progress labels Sep 8, 2018

lautarodragan commented Sep 8, 2018

View reviewed changes

kennylavender reviewed Sep 10, 2018

View reviewed changes

Merge branch 'master' into bitcoin-core-writer

cece185

geoffturk reviewed Sep 10, 2018

View reviewed changes

kennylavender reviewed Sep 10, 2018

View reviewed changes

geoffturk assigned lautarodragan Sep 10, 2018

kennylavender approved these changes Sep 10, 2018

View reviewed changes

Merge branch 'master' into bitcoin-core-writer

b4619ae

split docker-compose.testnet

e90d89a

jwicks31 reviewed Sep 11, 2018

View reviewed changes

lautarodragan added 3 commits September 10, 2018 21:47

add TODO

9ff3b1e

add unit tests for Bitcoin.getData

a1ce206

mini fix to docker-compose.yml

8c231fa

lautarodragan mentioned this pull request Sep 11, 2018

Bitcoin Core: Documentation #419

Closed

Merge branch 'master' into bitcoin-core-writer

0927e0c

lautarodragan merged commit 6547ee4 into master Sep 11, 2018

lautarodragan deleted the bitcoin-core-writer branch September 11, 2018 03:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Insight with Bitcoin Core in BlockchainWriter #408

Replace Insight with Bitcoin Core in BlockchainWriter #408

lautarodragan commented Sep 6, 2018 •

edited

Loading

lautarodragan Sep 8, 2018

kennylavender Sep 10, 2018

kennylavender Sep 10, 2018

lautarodragan Sep 10, 2018

kennylavender left a comment

lautarodragan commented Sep 10, 2018

geoffturk Sep 10, 2018

kennylavender Sep 10, 2018

kennylavender Sep 10, 2018

lautarodragan Sep 10, 2018

kennylavender left a comment •

edited

Loading

kennylavender left a comment

jwicks31 Sep 11, 2018

lautarodragan Sep 11, 2018

lautarodragan Sep 11, 2018

lautarodragan commented Sep 11, 2018

poet-ci commented Sep 11, 2018

		@@ -0,0 +1,2 @@
		export const getData = (prefix: string, version: ReadonlyArray<number>) => (message: string) =>

Replace Insight with Bitcoin Core in BlockchainWriter #408

Replace Insight with Bitcoin Core in BlockchainWriter #408

Conversation

lautarodragan commented Sep 6, 2018 • edited Loading

Release

Description of Changes

Docker Compose

Dependencies

Deleted Tests

Rename

Configuration

Fee Estimation

How to Test

Things Being Left Out

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

kennylavender left a comment

Choose a reason for hiding this comment

lautarodragan commented Sep 10, 2018

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

kennylavender left a comment • edited Loading

Choose a reason for hiding this comment

kennylavender 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

lautarodragan commented Sep 11, 2018

poet-ci commented Sep 11, 2018

lautarodragan commented Sep 6, 2018 •

edited

Loading

kennylavender left a comment •

edited

Loading