57 & 58 Request/Push Handling Logic (cont) #87

chadlagore · 2017-07-09T19:29:08Z

What Changed

Implemented Request handling logic.
Finished Push handling logic.
- This required an update to the miner module, @david-julien please 👀 .

TODO 💡

Mine should take an interface with a callback as an argument.
We can use this interface as a way to update progress on mining, alert the app of progress etc etc.

type MinerUpdater interface {
    UpdateProgress(i int)
    UpdateFinished(result MiningResult)
}

go miner.Mine(bc, block, updater)

// As opposed to:
result := miner.Mine(bc, block)   // => Spins forever.

…uest block implementation

coveralls · 2017-07-09T19:32:23Z

Coverage increased (+1.1%) to 78.226% when pulling 9fd850b on 57-request-handling-logic into 0ce1330 on dev.

chadlagore · 2017-07-09T23:36:48Z

app/app.go

@@ -159,13 +165,17 @@ func initializeWorkers() {
 	}
 }

+// initializeChain creates the blockchain for the node.


This is TODO.

Don't know how we're all handling TODOs but should we mark this as TODO in the comment just to make sure we don't forget?

david-julien · 2017-07-10T02:57:29Z

miner/miner_test.go

+
+	// Set min difficulty to be equal to the target so that the block validation
+	// passes
+	blockchain.MaxTarget = new(big.Int).Sub(


Reset the max target to its original value before the test returns so it doesn't cause any problems for any tests in the same file written after this test.

david-julien · 2017-07-10T03:03:05Z

miner/miner.go

 	"time"

 	log "github.com/Sirupsen/logrus"
 	"github.com/ubclaunchpad/cumulus/blockchain"
 	"github.com/ubclaunchpad/cumulus/consensus"
 )

+// CurrentlyMining is a flag to control the miner.
+var CurrentlyMining bool


Should CurrentlyMining and the lock be private?

Good call. Originally I wanted to have other packages check on them, but the exported function does that now.

coveralls · 2017-07-10T03:57:33Z

Coverage increased (+1.1%) to 78.226% when pulling 3b1804f on 57-request-handling-logic into 0ce1330 on dev.

bfbachmann

Nice work

bfbachmann · 2017-07-10T01:05:22Z

app/app.go

@@ -61,8 +61,10 @@ func Run(cfg conf.Config) {
 	go peer.MaintainConnections()

 	// Request the blockchain.
-	log.Info("Requesting blockchain from peers... ")
-	RequestBlockChain()
+	if chain == nil {


I'm doing some refactoring this week such that we don't have all these global variables, but instead we have instances of App for each Cumulus "node" we are running. The chain variable will also be part of that at some point just so you're aware.

Good stuff 👍

bfbachmann · 2017-07-10T01:10:29Z

app/app.go

@@ -91,19 +93,23 @@ func ConnectAndDiscover(target string) {
 // on peers whos PushHandlers have been overridden.
 func RequestHandler(req *msg.Request) msg.Response {
 	res := msg.Response{ID: req.ID}
+	typeErr := msg.NewProtocolError(msg.InvalidResourceType,
+		"Invalid resource type")
+	paramErr := msg.NewProtocolError(msg.InvalidResourceType,


We should probably add a different error code here to be more specific since this technically isn't an invalid resource type error.

bfbachmann · 2017-07-10T01:16:30Z

app/app.go

+// initializeChain creates the blockchain for the node.
+func initializeChain() {
+	chain, _ = blockchain.NewValidChainAndBlock()
+	// Check if chain exists on disk.


I don't think you should check if the chain exists here because this function should only be called when we don't already have a blockchain right? Actually, either way, when a node boots it should always ask its peers for blocks anyway (regardless of whether it already has a blockchain stored locally) since there is no other way of knowing for sure that you have the most up-to-date blockchain.

bfbachmann · 2017-07-10T02:20:09Z

app/app_test.go

@@ -13,6 +14,15 @@ func init() {
 	log.SetLevel(log.InfoLevel)
 }

+func createNewBlockRequest(blockNumber interface{}) *msg.Request {


Why not blockNumber of type int?

bfbachmann · 2017-07-11T01:16:14Z

blockchain/blockchain.go

+
+// CopyLocalBlockByIndex returns a copy of a block in the local chain by index.
+func (bc *BlockChain) CopyBlockByIndex(i uint32) *Block {
+	blk := bc.Blocks[i]


What if this is an out-of-bounds reference? I think you can also aggregate this line and the one below into one.

bfbachmann · 2017-07-11T01:21:10Z

miner/miner.go

+var currentlyMining bool
+
+// currentlyMiningLock is a read/write lock to change the Mining flag.
+var currentlyMiningLock sync.RWMutex


I guess this might be overkill but you could put currentlyMining and currentlyMiningLock into a struct of type Miner. Then you could make the functions below receivers of the Miner type. That way we can manage multiple instances of Miner.

Edit: I see you want to make Mine() take an interface and a callback as a means of updating progress. I think using a Miner type could help with that. The only thing is, I fell like in mining there is no notion of progress since you can never know for sure how close you are to the solution.

As for Miner type, I agree. Not in scope here, but I think we should do it. It would be nice to build in the ability to scale your miners outwards instead of upwards.

By progress I more meant, how many hashes it has attempted. Progress = number_attempted / number_of_hashes_to_needed_to_likely_produce_a_match.

bfbachmann · 2017-07-11T01:22:33Z

miner/miner.go

+
+const (
+	// MiningSuccessful is returned when the miner mines a block.
+	MiningSuccessful = iota


You only need to set the first constant to iota, the compiler will fill in the following ones on its own (see https://golang.org/ref/spec#Constant_declarations and the section below).

bfbachmann · 2017-07-11T01:29:45Z

miner/miner.go

-func Mine(bc *blockchain.BlockChain, b *blockchain.Block) bool {
+// TODO: Make Mine take an interface with a callback as an arguement.
+func Mine(bc *blockchain.BlockChain, b *blockchain.Block) *MiningResult {
+	setStart()
 	if valid, _ := bc.ValidBlock(b); !valid {
 		log.Error("Invalid block")


I know you added this earlier but we should probably add more detail here. Generally when we surface errors we want to know what package they're from and what action what being taken when the error occurred.
In this case something like "Mine failed, miner received an invalid block".

Actually, I don't even think the miner should be worried about validation. Good eye 👀

bfbachmann · 2017-07-11T01:30:55Z

miner/miner.go

+)
+
+// MiningResult contains the result of the mining operation.
+type MiningResult struct {


Why have this be a type when we can just return (complete bool, info int) from Mine()? Do we use MiningResult outside this file?

bfbachmann · 2017-07-11T01:35:39Z

miner/miner.go

+}
+
+// StopMining stops the miner from mining.
+func StopMining() {


I love the idea of cancelling a mining job. Nice work, we'll definitely need to do this when we are notified of new blocks that were added to the chain that contain transactions we are trying to mine in this block.

jordanschalm

🚢

jordanschalm · 2017-07-11T20:08:02Z

@chadlagore Feel free to merge this after review comments and merge conflicts are resolved 👍

jordanschalm · 2017-07-11T20:11:24Z

closes #56
closes #57
closes #58

Let's close these and make more descriptive issues next meeting

coveralls · 2017-07-12T03:44:53Z

Coverage increased (+9.6%) to 78.226% when pulling 181e2e2 on 57-request-handling-logic into 2fef206 on dev.

coveralls · 2017-07-12T03:46:34Z

Coverage increased (+9.6%) to 78.226% when pulling 992236f on 57-request-handling-logic into 2fef206 on dev.

coveralls · 2017-07-12T03:48:53Z

Coverage increased (+9.6%) to 78.226% when pulling 992236f on 57-request-handling-logic into 2fef206 on dev.

coveralls · 2017-07-12T03:56:55Z

Coverage increased (+1.3%) to 69.978% when pulling 3f5da68 on 57-request-handling-logic into 2fef206 on dev.

coveralls · 2017-07-12T04:07:36Z

Coverage increased (+1.4%) to 69.986% when pulling 17b6ced on 57-request-handling-logic into 2fef206 on dev.

chadlagore added 3 commits July 9, 2017 11:13

added testify dep; added CopyBlockByIndex; added app tests; added req…

53318d0

…uest block implementation

added miner response to pushed blocks

8d955b6

comments

9fd850b

chadlagore requested review from jordanschalm and david-julien July 9, 2017 19:29

chadlagore changed the title ~~57 & 58 Request/Push Handling Logic.~~ 57 & 58 Request/Push Handling Logic (cont) Jul 9, 2017

chadlagore commented Jul 9, 2017

View reviewed changes

david-julien approved these changes Jul 10, 2017

View reviewed changes

remove max target from miner test; add TODO; privatized currentlyMining

3b1804f

bfbachmann reviewed Jul 11, 2017

View reviewed changes

jordanschalm approved these changes Jul 11, 2017

View reviewed changes

chadlagore force-pushed the 57-request-handling-logic branch from 992236f to 3b1804f Compare July 12, 2017 03:49

merge dev

3f5da68

incorp feedback

17b6ced

chadlagore merged commit 6788fe6 into dev Jul 12, 2017

jordanschalm deleted the 57-request-handling-logic branch July 15, 2017 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

57 & 58 Request/Push Handling Logic (cont) #87

57 & 58 Request/Push Handling Logic (cont) #87

chadlagore commented Jul 9, 2017 •

edited

Loading

coveralls commented Jul 9, 2017

chadlagore Jul 9, 2017

david-julien Jul 10, 2017 •

edited

Loading

david-julien Jul 10, 2017

david-julien Jul 10, 2017

chadlagore Jul 10, 2017

coveralls commented Jul 10, 2017

bfbachmann left a comment

bfbachmann Jul 10, 2017

chadlagore Jul 11, 2017

bfbachmann Jul 10, 2017

bfbachmann Jul 10, 2017

bfbachmann Jul 10, 2017

chadlagore Jul 11, 2017

bfbachmann Jul 11, 2017

bfbachmann Jul 11, 2017

bfbachmann Jul 11, 2017

chadlagore Jul 11, 2017

bfbachmann Jul 11, 2017

bfbachmann Jul 11, 2017

chadlagore Jul 11, 2017

bfbachmann Jul 11, 2017

bfbachmann Jul 11, 2017

jordanschalm left a comment

jordanschalm commented Jul 11, 2017

jordanschalm commented Jul 11, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

57 & 58 Request/Push Handling Logic (cont) #87

57 & 58 Request/Push Handling Logic (cont) #87

Conversation

chadlagore commented Jul 9, 2017 • edited Loading

What Changed

TODO 💡

coveralls commented Jul 9, 2017

Choose a reason for hiding this comment

david-julien Jul 10, 2017 • 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

coveralls commented Jul 10, 2017

bfbachmann 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

jordanschalm left a comment

Choose a reason for hiding this comment

jordanschalm commented Jul 11, 2017

jordanschalm commented Jul 11, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

chadlagore commented Jul 9, 2017 •

edited

Loading

david-julien Jul 10, 2017 •

edited

Loading