Bifrost - alpha version #81

bartekn · 2017-10-02T16:02:43Z

Demo app
https://bifrost.stellar.org/

First you need some ETH on Ethereum testnet.
Create an account at https://www.myetherwallet.com/, then switch Network (top-right dropdown) to "Ropsten (MyEtherWallet)". Write down/copy your Ethereum address somewhere.
Use https://faucet.ropsten.be:3001/ to send 3 ETH to your testnet account.
Now open Bifrost demo: https://bifrost.stellar.org/ It will display address where to send your test ETH.
Go back to MyEtherWallet and send ETH to the address displayed by Bifrost. Sometimes Ropsten network is overloaded so monitor the Etherscan if you tx was included in a block. If not, increated gas price (this can be done in "Send Offline" tab).
Switch back to Bifrost demo and check the progress.

Work in progress: do not use in production, do not merge.

This is MVP version of Bifrost [design doc]. Bifrost accepts coins from other blockchains and issues them on Stellar blockchain.

TODO (random order):

poliha

Looks good in general. Just had my first glance over it, will continue to read the code and I guess we will improve on it as we fix issues.
Quick question is setting blocknumber to 0 in BTC/ETH equivalent to setting cursor to "now" in stellar?

poliha · 2017-11-13T17:17:27Z

services/bifrost/main.go

+
+var checkKeysCmd = &cobra.Command{
+	Use:   "check-keys",
+	Short: "Displays a few public keys derivied using master public keys",


typo derived

poliha · 2017-11-13T17:19:03Z

services/bifrost/main.go

+	Use:   "version",
+	Short: "Print the version number",
+	Run: func(cmd *cobra.Command, args []string) {
+		fmt.Println("version")


Version number is not printed

OK, tracking in #158

poliha · 2017-11-13T17:20:15Z

services/bifrost/main.go

+		}
+
+		ethereumListener.Enabled = true
+		ethereumListener.NetworkID = "3"


Should the NetworkID be from the config file? In case the user wants to use a different test network?

This code is responsible for stress tests so it should always be testnet.

poliha · 2017-11-13T17:21:49Z

services/bifrost/bitcoin/listener.go

+				TxOutIndex: index,
+				ValueSat:   output.Value,
+				To:         addresses[0].EncodeAddress(),
+			}


Not sure about this, but I think addresses can be more than one. This only picks the first address

There can be more than one address for multisig outputs. Bifrost receiving addresses are normal P2PKH addresses so always only one address.

poliha · 2017-11-13T17:24:28Z

services/bifrost/database/postgres.go

@@ -0,0 +1,440 @@
+package database


Can these functions also be used when we implement support for another DB type?
If so maybe we shouldn't just name this postgres.go? or maybe move it into main.go?
What do you think here?

I don't think there are plans to use other DB engines.

poliha · 2017-11-13T17:25:52Z

services/bifrost/server/bitcoin_rail.go

+// the transactions queue for StellarAccountConfigurator to consume.
+//
+// Transaction added to transactions queue should be in a format described in
+// queue.Queue (especialy amounts). Pooling service should not have to deal with any


I think this should be queue.Transaction? Also check ethereum_rail.go

poliha · 2017-11-13T17:26:59Z

services/bifrost/queue/main.go

+// entry with the same ID was added.
+// This is a critical requirement! Otherwise ETH/BTC may be sent twice to Stellar account.
+// If you don't know what to do, use default AWS SQS FIFO queue or DB queue.
+type Queue interface {


Question, has this been implemented?

Yes, Database implementation Queue works as expected. No implementation for SQS FIFO yet but it's not urgent as we have DB one.

bartekn · 2017-11-14T15:38:27Z

Quick question is setting blocknumber to 0 in BTC/ETH equivalent to setting cursor to "now" in stellar?

Exactly.

nikhilsaraf

Added first pass of comments, most of them are code style comments.
Will go more in depth into reviewing the architecture and other parts in the next pass.

nikhilsaraf · 2017-11-22T22:38:32Z

services/bifrost/README.md

+
+## Going to production
+
+* Remember than everyone with master public key and **any** child private key can recover your **master** private key. Do not share your master public key and obviously any private keys. Treat your master public key as if it was a private key. Read more in BIP-0032 [Security](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#security) section.


type? (Remember than -> Remember that)

nikhilsaraf · 2017-11-22T22:40:10Z

services/bifrost/README.md

+## Going to production
+
+* Remember than everyone with master public key and **any** child private key can recover your **master** private key. Do not share your master public key and obviously any private keys. Treat your master public key as if it was a private key. Read more in BIP-0032 [Security](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#security) section.
+* Make sure "Sell [your token] for BTC" and/or "Sell [your token] for ETH" exist in Stellar production network.


can you provide more information about what this "Sell [your token] for BTC/Ethereum" means (purpose, where to find it, is this referring to a mange offer)?

In the last stage of the flow BTC/ETH tokens will be exchanged to [your token] on SDEX. Yes, it's manage_offer.

I think we should update this to reflect that the manage_offer should exist. I completely misinterpreted this to expect that I should look for that text somewhere.

nikhilsaraf · 2017-11-22T22:45:20Z

services/bifrost/README.md

+* It's a good idea to set "Authorization required" and "Authorization revocable" [flags](https://www.stellar.org/developers/guides/concepts/accounts.html#flags) on issuing account during ICO stage to remove trustlines to accounts with lost keys.
+* Monitor bifrost logs and react to all WARN and ERROR entries.
+* Make sure you are using geth >= 1.7.1 and bitcoin-core >= 0.15.0.
+* Turn off horizon rate limiting.


can you add a reason for why they should turn off rate limiting? (shouldn't it be on to help prevent any DDOS attacks?)

nikhilsaraf · 2017-11-22T22:49:34Z

services/bifrost/README.md

+* Make sure you are using geth >= 1.7.1 and bitcoin-core >= 0.15.0.
+* Turn off horizon rate limiting.
+* Make sure you configured minimum accepted value for Bitcoin and Ethereum transactions to the value you really want.
+* Make sure you start from a fresh DB in production. If Bifrost was running, you stopped it and then started again all the block mined during that that will be processed what can take a lot of time.


does blocks mining refer to bitcoin blocks here?
If they start from a fresh db in production then will it not try to catchup to the bitcoin network anyways?

typo? (If Bifrost was running, you stopped it bitcoin-core and then started it again then all the blocks mined during that ~~that~~ time will be processed ~~what~~ which can take a lot of time.

does blocks mining refer to bitcoin blocks here?

Bitcoin and Ethereum.

If they start from a fresh db in production then will it not try to catchup to the bitcoin network anyways?

I meant Bifrost DB. The Bitcoin and Ethereum nodes should stay in sync all the time. Will fix this.

nikhilsaraf · 2017-11-22T22:52:34Z

services/bifrost/bifrost.cfg

+
+[database]
+type="postgres"
+dsn="postgres:https://bartek@localhost/bifrost?sslmode=disable"


should we remove sslmode=disable here to encourage encrypted db connections?

nikhilsaraf · 2017-11-23T00:40:59Z

services/bifrost/common/main.go

+	logger := log.DefaultLogger
+
+	if serviceName != "" {
+		logger = logger.WithField("service", serviceName)


why save in logger, could just return here and then always return log.DefaultLogger below instead of creating above.

nikhilsaraf · 2017-11-23T00:48:52Z

services/bifrost/database/migrations/01_init.sql

+  pooled boolean NOT NULL DEFAULT false,
+  PRIMARY KEY (id),
+  UNIQUE (transaction_id, asset_code),
+  CONSTRAINT valid_asset_code CHECK (char_length(asset_code) >= 3),


can we not have an asset_code that is less then 3 characters long?

Currently we only support BTC and ETH. We will update this CHECK if there is new asset with less or more than 3 chars. Changed >= to =.

nikhilsaraf · 2017-11-23T00:56:46Z

services/bifrost/ethereum/listener.go

+	return nil
+}
+
+func (l *Listener) processBlocks(blockNumber uint64) {


this function seems common between btc and ethereum implementation. Can we extract this into it's own component that takes in the logInterval (20 minutes vs. 3 minutes), logIntervalString (string representation of logInterval), and Storage? -- see comment in Storage for interface change suggestion to support this extraction

nikhilsaraf · 2017-11-23T00:59:27Z

services/bifrost/ethereum/main.go

+
+// Storage is an interface that must be implemented by an object using
+// persistent storage.
+type Storage interface {


this interface is serving the same purpose as that of the bitcoin Storage. They can both be the same interface by generalizing the method names on this interface and we can have one BitcoinStorage implementation and another EthereumStorage implementation. Will make it possible to then extract the logic in processBlocks up above.

We should address this issue since there's a fair amount of code duplication going on here via copy-paste, which can make code maintenance a lot more costly later on.

nikhilsaraf · 2017-11-23T01:01:27Z

services/bifrost/ethereum/main.go

+	masterPublicKey *bip32.Key
+}
+
+func EthToWei(eth string) (*big.Int, error) {


this is also common, can extract to an object that takes in the multiplier (weiInEth or satInBtc) and also unitName (satoshi / wei) for the error message.

…onfig file Close #129 Close #128

Close #117 Close #118 Close #122

bartekn · 2017-12-05T18:11:12Z

I think the code is good for Alpha version for developers and community to test. @nikhilsaraf can you approve?

nikhilsaraf

Approving as an alpha version to let the ball roll on this and to not be a blocker since this has already been used in production and we should get it out asap.

Please address some of the leftover comments (particularly design comments) when you get back to this code.

nikhilsaraf · 2017-12-05T19:06:07Z

services/bifrost/main.go

+	return db, err
+}
+
+func createServer(cfg config.Config, stressTest bool) *server.Server {


this function is very long, for the next change, can you break it up? -- maybe split up the for loop blocks into their own functions.

nikhilsaraf · 2017-12-05T19:07:10Z

services/bifrost/main.go

+	Short: "Starts stress test",
+	Long: `During stress test bitcoin and ethereum transaction will be generated randomly.
+This command will create 3 server.Server's listening on ports 8000-8002.`,
+	Run: func(cmd *cobra.Command, args []string) {


here too, could split up this function and move to it's own named function rather than inline since it's quite large

nikhilsaraf · 2017-12-05T19:08:05Z

services/bifrost/main.go

@@ -0,0 +1,399 @@
+// Skip this file in Go <1.8 because it's using http.Server.Shutdown


why is the architecture.png image here? -- I thought it was only for the design doc?

bartekn added the in progress label Oct 2, 2017

bartekn force-pushed the bifrost branch from 821f0dd to d62555d Compare October 2, 2017 16:22

bartekn self-assigned this Oct 3, 2017

bartekn force-pushed the bifrost branch from d399dea to 89e81e4 Compare October 13, 2017 16:19

bartekn requested review from nullstyle, jedmccaleb, tomerweller, nikhilsaraf, poliha and breznikov7 October 13, 2017 16:44

bartekn force-pushed the bifrost branch from 89e81e4 to c98cb65 Compare October 20, 2017 13:35

bartekn added the bifrost label Oct 20, 2017

bartekn mentioned this pull request Oct 26, 2017

Forced account_merge stellar/stellar-protocol#56

Closed

bartekn mentioned this pull request Nov 6, 2017

Proposal: BIP32 (Hierarchical Deterministic Wallet) Support stellar/stellar-protocol#61

Closed

bartekn changed the title ~~Bifrost~~ [WIP] Bifrost Nov 8, 2017

poliha reviewed Nov 13, 2017

View reviewed changes

nikhilsaraf reviewed Nov 23, 2017

View reviewed changes

bartekn mentioned this pull request Nov 28, 2017

Link for BiFrost in Readme.md is bad stellar-deprecated/bifrost-js-sdk#1

Closed

bartekn force-pushed the bifrost branch from 33cb9cc to 3a31d86 Compare December 5, 2017 16:53

bartekn and others added 10 commits December 5, 2017 18:09

Bifrost

0ecdf8c

Bitcoin listener, updates, README

281edfa

Stress tests, bug fixes

61527d3

Fix trailing slash bug in horizon.Client

a1d2b3f

Fix SSE streaming in multiserver environment

4a54225

Allow transaction from only one chain, minimum BTC and ETH value in c…

89812bd

…onfig file Close #129 Close #128

Updates

c359b96

Close #117 Close #118 Close #122

wip: recovery transaction

88b25a6

update bifrost migrations

b041238

update config

9a2c546

poliha and others added 12 commits December 5, 2017 18:09

use allowed URL config

3c029be

change config variable.

d68257a

AccessControlAllowOriginHeader default

114e894

fix errors

a656f80

Trust line authorization

5195e3e

Fix recovery

db5513f

Changes for @poliha comments

6caf13f

Fix testnet bitcoin /events

0e0fa80

Update readme

f6fb767

StatusBadGateway -> StatusBadRequest

ccc52d0

Updates

35bd82e

Updates

ded793d

bartekn force-pushed the bifrost branch from 3a31d86 to 06500e3 Compare December 5, 2017 17:14

Skip server tests in Go <1.8

c6be61c

bartekn force-pushed the bifrost branch from 06500e3 to c6be61c Compare December 5, 2017 17:21

bartekn changed the title ~~[WIP] Bifrost~~ Bifrost - alpha version Dec 5, 2017

bartekn mentioned this pull request Dec 5, 2017

[WIP] Process queued transaction within DB transaction #156

Closed

nikhilsaraf previously approved these changes Dec 5, 2017

View reviewed changes

Fix README

14553a4

bartekn dismissed nikhilsaraf’s stale review via 14553a4 December 5, 2017 19:19

nikhilsaraf approved these changes Dec 5, 2017

View reviewed changes

bartekn merged commit da7a365 into master Dec 5, 2017

bartekn removed the in progress label Dec 5, 2017


		## Going to production

		* Remember than everyone with master public key and any child private key can recover your master private key. Do not share your master public key and obviously any private keys. Treat your master public key as if it was a private key. Read more in BIP-0032 [Security](https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#security) section.

		@@ -0,0 +1,399 @@
		// Skip this file in Go <1.8 because it's using http.Server.Shutdown

Bifrost - alpha version #81

Bifrost - alpha version #81

Conversation

bartekn commented Oct 2, 2017 • edited Loading

poliha left a comment • 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

bartekn commented Nov 14, 2017

nikhilsaraf 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

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

bartekn commented Dec 5, 2017

nikhilsaraf 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

bartekn commented Oct 2, 2017 •

edited

Loading

poliha left a comment •

edited

Loading