Add blockchain update mechanism, add request timeout to Peer #115

bfbachmann · 2017-07-28T06:13:43Z

Status

Ready for review

Related Issue

#111

Description

This adds a built-in timeout mechanism for peer requests. Now, when we call peer.Request and no response comes back after the default timeout period, we call the callback passed to peer.Request with a ProtocolError containing the RequestTimeout error code and message. This was users of that function don't have to implement their own timeout mechanisms as it is built into the protocol.
Added SyncBlockchain function to iteratively request the next block from random peers. It is called when we boot with a target peer to connect to (download blockchain), and when we received a new block via a push message that is not valid wrt our blockchain. This way, if we are on the wrong fork of the blockchain, we miss a block, or we go offline for a while an then come back we will recognize that our blockchain is out of date and update it.
A subtle but notable change: we now disconnect from a peer if we get 3 errors in reading it's messages over the entire course of our communication (they no longer need to be consecutive).
Pulled globals in App into the App struct
Updated getLocalChain to just give us a new blockchain with a genisis block. Note that we always start with a genisis block, and if a target peer is provided we roll back our blockchain to an empty state and add the genisis block given to us by the peer.
Added mining integration to App, as well as a mine flag. Mining is turned off my default. The miner restarts every time the blockchain is updated.
Fixed a few bugs to do with decoding json messages
Added miner command to the console for starting, stopping, and checking the status of the miner at will.
Updated conn to use a better mock of net.Conn called BufConn
Fixed some bugs in Cloudbase transaction validation
Added BadRequest (400) error code to message
Broke peer testing into peer_test.go and peerstore_test.go

WIKI Updates

TODO

Todos

General:

Tests
Wiki

coveralls · 2017-07-28T06:15:22Z

Coverage decreased (-3.8%) to 67.211% when pulling 503d4d9 on 111-missed-blocks into 5e0015e on dev.

coveralls · 2017-07-30T00:47:36Z

Coverage increased (+4.8%) to 75.489% when pulling 0832026 on 111-missed-blocks into 7eee1b7 on dev.

coveralls · 2017-07-30T20:17:14Z

Coverage increased (+4.2%) to 74.925% when pulling 03de519 on 111-missed-blocks into 7eee1b7 on dev.

chadlagore · 2017-07-31T05:20:20Z

app/app.go

+
+// SyncBlockchain updates the local copy of the blockchain by requesting missing
+// blocks from peers. Returns error if we are not connected to any peers.
+func (a *App) SyncBlockchain() error {


Nitpick, lets use SyncBlockChain? (since the blockchain itself is pascal case)

chadlagore · 2017-07-31T05:21:29Z

app/app.go

+		Head:   blockchain.NilHash,
+	}
+
+	genisisBlock := blockchain.Genesis(user.HotWallet.Wallet.Public(),


Im not in love with the User API at the moment. I have some work to do there, this works for now.

chadlagore · 2017-07-31T05:30:08Z

app/app.go

+		}
+
+		// Pick a peer to send the request to (this won't always be the same one)
+		p := a.PeerStore.Get(peerAddrs[0])


Without diving deep into the peer package, its very difficult to appreciate what this line is supposed to be doing. For example, how is it that this generates a different peer each loop? Shouldn't the addresses be abstracted away at this level? Why do I care which peer I'm connecting to (or what their address is)? I just want to send block requests out to the network.

Yea I agree, I think there should just be a GetRandom function for this. But we do need to select a peer to send the request to, because broadcasting a request doesn't really make much sense.

chadlagore · 2017-07-31T05:31:28Z

app/app.go

+				continue
+			}
+
+			valid, validationCode := consensus.VerifyBlock(a.Chain, newBlock)


👏 thorough!

chadlagore

Drive by comments. Looking good so far 🎉

One general note about the approach we're using here to download the block chain, it seems like an unfortunately serial way to do it. I totally buy the idea of looking for the next block based on hash, but we basically do zero asynchronous torrenting from multiple peers like this (which was the goal I believe?)

chadlagore · 2017-07-31T05:33:39Z

blockchain/block.go

@@ -43,6 +43,16 @@ func (bh *BlockHeader) Marshal() []byte {
 	return buf
 }

+// Equal returns true if all the fields (other than ExtraData) in each of
+// the BlockHeaders match, and false otherwise.
+func (bh *BlockHeader) Equal(otherHeader *BlockHeader) bool {


Reads nicer as Equals

I was going to make it Equals but that's not the Go way 🤷‍♂️.

chadlagore · 2017-07-31T05:34:11Z

blockchain/blockchain.go

@@ -69,7 +69,7 @@ func (bc *BlockChain) ContainsTransaction(t *Transaction, start, stop uint32) (b
 	return false, 0, 0
 }

-// CopyLocalBlockByIndex returns a copy of a block in the local chain by index.
+// CopyBlockByIndex returns a copy of a block in the local chain by index.


🤦‍♂️ 🙏

chadlagore · 2017-07-31T05:36:04Z

app/app.go

+	errChan := make(chan *msg.ProtocolError)
+
+	// Define a handler for responses to our block requests
+	blockResponseHandler := func(blockResponse *msg.Response) {


This for example can be stubbed out for easy testing.

chadlagore · 2017-07-31T05:36:11Z

app/app.go

+
+// SyncBlockchain updates the local copy of the blockchain by requesting missing
+// blocks from peers. Returns error if we are not connected to any peers.
+func (a *App) SyncBlockchain() error {


This is a huge function and very difficult to test. Is it possible to break it up a bit?

chadlagore · 2017-07-31T05:36:53Z

peer/peer.go

@@ -159,11 +161,50 @@ func (p *Peer) Dispatch() {

 // Request sends the given request over this peer's Connection and registers the
 // given response hadnler to be called when the response arrives at the dispatcher.


handler* - realize its old stuff

chadlagore · 2017-07-31T05:38:42Z

peer/peerstore_test.go

@@ -0,0 +1,253 @@
+package peer


In general, I think its a good idea to move towards the testify package and use assert statements. They really help with debugging, giving useful explanations of the failure (especially helpful when other people try to make changes in your code!)

Moved globals in the App package into the App struct. Changed CopyBlockByIndex() to CopyBlockByLastBlockHash() Change User to have Wallet instread of HotWallet Added mining integration to App Updated SyncBlockChain() Added App tests Updated tests and test_utils Added miner command to console for checking and toggling miner status Added mine flag to Cobra config

coveralls · 2017-08-03T05:25:45Z

Coverage increased (+2.6%) to 73.311% when pulling 7b7ad06 on 111-missed-blocks into 7eee1b7 on dev.

coveralls · 2017-08-05T17:30:24Z

Coverage increased (+4.03%) to 74.751% when pulling 8060ab9 on 111-missed-blocks into 7eee1b7 on dev.

App: - Add blockchain download then target is specified - Fix bugs in RequestHandler - Fix block decoding bugs in SyncBlockChain Blockchain: - Add DecodeBlockJSON - Change CopyBlockByBlockHash to GetBlockByBlockHash Cmd/Conf: - Change no-miner to miner, so we don't mine by default Conn: - Replace TestConn with BufConn Consensus: - Fix Cloudbase transaction validation bugs Message: - Add BadRequest protocol error - Update decoding so it handles big numbers

coveralls · 2017-08-12T19:41:06Z

Coverage increased (+4.2%) to 75.014% when pulling a660454 on 111-missed-blocks into b6d5e37 on dev.

jordanschalm

Very nice work.

jordanschalm · 2017-08-12T22:00:32Z

app/app.go

@@ -204,11 +231,19 @@ func (a *App) PushHandler(push *msg.Push) {
 }

 // getLocalChain returns an instance of the blockchain.
-func getLocalChain() *blockchain.BlockChain {
+func getLocalChain(user *User) *blockchain.BlockChain {


Why is user a param?

It seems like the function is overloaded with two separate situations:

we want to start a new chain with a genesis block

we want to continue an old chain by restoring from storage

We should have two separate functions for these two situations.

jordanschalm · 2017-08-12T22:03:34Z

app/app.go

+	genisisBlock := blockchain.Genesis(user.Wallet.Public(),
+		consensus.CurrentTarget(), consensus.StartingBlockReward, []byte{})
+
+	bc.AppendBlock(genisisBlock)


typo genisis -> genesis

Sad thing is I meant to write it like that... I thought that was how it was spelled :(.

jordanschalm · 2017-08-12T22:05:22Z

app/app.go

+		chainChanged, err := a.SyncBlockChain()
+		if chainChanged && miner.IsMining() {
+			miner.StopMining()
+			go a.Mine()


worthwhile to have a RestartMining method? out of scope for this PR but make an issue for it if it's worth the time

jordanschalm · 2017-08-12T22:10:00Z

app/app.go

+// blocks from peers. Returns true if the blockchain changed as a result of
+// calling this function, false if it didn't and an error if we are not connected
+// to any peers.
+func (a *App) SyncBlockChain() (bool, error) {


Can probably just have this return false if no peers and omit the error return
I'd also recommend checking for no peers before doing anything else and exiting early if we aren't connected to anything

It's possible, whoever unlikely, that all our peers disconnect from us while this function is running, in which case the blockchain might have been partially updated, but the update is incomplete. Checking if we are connected to peers before calling this would not guarantee that we have peers to make requests to throughout the duration of the function. So I think we still need both return values here.

jordanschalm · 2017-08-12T22:18:22Z

peer/peerstore.go

+func (ps *PeerStore) GetRandom() *Peer {
+	ps.lock.RLock()
+	defer ps.lock.RUnlock()
+	for _, p := range ps.peers {


coveralls · 2017-08-13T00:58:12Z

Coverage increased (+4.4%) to 75.218% when pulling 4fc353b on 111-missed-blocks into b6d5e37 on dev.

chadlagore

Wow, lots going on here! Couple notes:

assert everywhere, literally saves you 50 lines in this pr and saves everyone else some serious head scratching. There is no reason not to use it at this point.
Long functions are hard to test, especially in the app package which already suffers from low coverage (it makes it really hard for others to make changes to your code in the future, the Run function is totally untested, and now the SyncBlockChain logic appears to be going the same way).
The scope is so far blown out, I don’t even see where this PR addresses ticket 111? Three separate PRs here would be better than one if you want legit feedback - its actually difficult to sign off on all of this because it takes so long to grok it all! Plus you expose yourself to too many TODO’s from onlookers. Takes forever to merge.

Theres lots about this PR that is sweet, but I 🙏 to see two things before we merge:

Reduce the complexity of SyncBlockChain so that it doesn't go forever untested. Test it piece by piece.
Use assert. It would take 10mins to swap these t.FailNow's out.

EDIT: Im noticing that this is probably more geared towards issue 118? In that case the scope makes more sense! I thought this was literally random coding lol.

chadlagore · 2017-08-13T01:38:44Z

app/app.go

+	Pool             *pool.Pool
+	blockQueue       chan *blockchain.Block
+	transactionQueue chan *blockchain.Transaction
+	quitChan         chan bool


chadlagore · 2017-08-13T01:41:23Z

app/app.go

@@ -80,6 +87,11 @@ func Run(cfg conf.Config) {
 	// stream in. Kick off a worker to handle requests and pushes.
 	go a.HandleWork()

+	if config.Mine {


This strikes me as a user-level configuration, but now I'm actually leaning towards one config file to rule them all. Perhaps we can move the BlockSize variable into config as well, then we can maybe do away with the user abstraction altogether 🤔 (Another PR)

The Cobra library we're using already supports config files by default.

chadlagore · 2017-08-13T01:42:52Z

app/app.go

+		log.Debug("Received block request")
+
+		// Block is requested by block hash.
+		hashBytes, err := json.Marshal(req.Params["lastBlockHash"])


chadlagore · 2017-08-13T01:44:03Z

app/app.go

-	// TODO: Look for local chain on disk. If doesn't exist, go rummaging
+// createBlockchain returns a new instance of a blockchain with only a genesis
+// block.
+func createBlockchain(user *User) *blockchain.BlockChain {
 	// around on the internets for one.


Might just blow this line away? 🔫

chadlagore · 2017-08-13T01:50:02Z

app/app.go

+func (a *App) Mine() {
+	log.Info("Starting miner")
+	for {
+		// Make a new block form the transactions in the transaction pool


chadlagore · 2017-08-13T02:08:44Z

app/test_utils.go

@@ -7,9 +7,9 @@ import (
 	"github.com/ubclaunchpad/cumulus/pool"
 )

-func createNewTestBlockRequest(lastBlock interface{}) *msg.Request {
+func createNewTestBlockRequest(lastBlockHash interface{}) *msg.Request {


chadlagore · 2017-08-13T02:10:42Z

blockchain/block_test.go

+	}
+
+	block2 := NewTestBlock()
+	if (&block1.BlockHeader).Equal(&block2.BlockHeader) {


We should use assert. Its less lines, easier to read, easier to get other people to work with your unit tests. Just far superior!

chadlagore · 2017-08-13T02:11:37Z

blockchain/blockchain.go

-		b.Transactions = make([]*Transaction, len(blk.Transactions))
-		copy(b.Transactions, blk.Transactions)
-		return &b, nil
+// GetBlockByLastBlockHash returns a copy of the block in the local chain that


chadlagore · 2017-08-13T02:11:58Z

blockchain/blockchain.go

+
+// RollBack removes the last block from the blockchain. Returns the block that
+// was removed from the end of the chain, or nil if the blockchain is empty.
+func (bc *BlockChain) RollBack() *Block {


Interesting

chadlagore · 2017-08-13T02:15:35Z

blockchain/blockchain.go

+		return nil
+	}
+	prevHead := bc.LastBlock()
+	bc.Blocks = bc.Blocks[:len(bc.Blocks)-1]


Fancy golang pop:

prevHead, bc.Blocks = bc.Blocks[len(bc.Blocks)-1], bc.Blocks[:len(bc.Blocks)-1]

https://github.com/golang/go/wiki/SliceTricks

Yours is probably more readable 😆

chadlagore · 2017-08-13T17:35:15Z

app/app.go

@@ -343,15 +338,22 @@ func (a *App) Mine() {
 	}
 }

+// RestartMiner restarts the miner only if it is running when this is called.
+func (a *App) RestartMiner() {


chadlagore

Looks great!

coveralls · 2017-08-13T17:54:45Z

Coverage increased (+5.5%) to 76.821% when pulling 72c93e2 on 111-missed-blocks into 25241c2 on dev.

coveralls · 2017-08-13T22:39:17Z

Coverage increased (+6.0%) to 77.273% when pulling 72da9f1 on 111-missed-blocks into 25241c2 on dev.

coveralls · 2017-08-14T01:22:21Z

Coverage increased (+5.5%) to 76.735% when pulling 9228558 on 111-missed-blocks into 25241c2 on dev.

Add blockchain update mechanism, add request timeout to Peer

503d4d9

bfbachmann added 3 commits July 29, 2017 17:36

Add Peer tests, remove Glide grabage

2cc7efe

Merge branch 'dev' into 111-missed-blocks

19d2bcd

Fix merge issue in dev

0832026

Update getLocalChain, update SyncBlockchain, refactor peer tests

03de519

chadlagore reviewed Jul 31, 2017

View reviewed changes

app/app.go Outdated

continue

}

valid, validationCode := consensus.VerifyBlock(a.Chain, newBlock)

Copy link

Contributor

chadlagore Jul 31, 2017

Choose a reason for hiding this comment

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

👏 thorough!

chadlagore reviewed Jul 31, 2017

View reviewed changes

bfbachmann changed the title ~~Add blockchain update mechanism, add request timeout to Peer~~ [WIP] Add blockchain update mechanism, add request timeout to Peer Aug 1, 2017

Add missed block detection to HandleBlock()

8060ab9

bfbachmann changed the title ~~[WIP] Add blockchain update mechanism, add request timeout to Peer~~ Add blockchain update mechanism, add request timeout to Peer Aug 12, 2017

Merge branch 'dev' into 111-missed-blocks

a660454

jordanschalm requested changes Aug 12, 2017

View reviewed changes

Update getLocalChain, update tests, add UpToDate for blockchain sync

4fc353b

chadlagore reviewed Aug 13, 2017

View reviewed changes

Refactor SyncBlockChain, update/add tests, change Mine() -> RunMiner()

c26ae6a

chadlagore reviewed Aug 13, 2017

View reviewed changes

chadlagore approved these changes Aug 13, 2017

View reviewed changes

Merge branch 'dev' into 111-missed-blocks

72c93e2

Add target to new blocks so we can mine

72da9f1

Clean up app, fix bug in peer handshake

9228558

bfbachmann merged commit 836f6e7 into dev Aug 15, 2017

jordanschalm deleted the 111-missed-blocks branch August 30, 2017 21:10

		@@ -159,11 +161,50 @@ func (p *Peer) Dispatch() {

		// Request sends the given request over this peer's Connection and registers the
		// given response hadnler to be called when the response arrives at the dispatcher.

Add blockchain update mechanism, add request timeout to Peer #115

Add blockchain update mechanism, add request timeout to Peer #115

Conversation

bfbachmann commented Jul 28, 2017 • edited Loading

Status

Related Issue

Description

WIKI Updates

Todos

coveralls commented Jul 28, 2017

coveralls commented Jul 30, 2017

coveralls commented Jul 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadlagore Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadlagore 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

coveralls commented Aug 3, 2017

coveralls commented Aug 5, 2017

coveralls commented Aug 12, 2017

jordanschalm 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

coveralls commented Aug 13, 2017

chadlagore 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

chadlagore left a comment

Choose a reason for hiding this comment

coveralls commented Aug 13, 2017

coveralls commented Aug 13, 2017

coveralls commented Aug 14, 2017

bfbachmann commented Jul 28, 2017 •

edited

Loading

chadlagore Jul 31, 2017 •

edited

Loading

chadlagore left a comment •

edited

Loading