128 synchronized blockchain #129
Conversation
| t.Fail() | ||
| } | ||
| DecodeBlockChain(buf) | ||
| // TODO: fix this |
There was a problem hiding this comment.
Oh right, that started failing when I added the lock to the blockchain for some strange reason. I didn't want to spend time figuring out what the problem was so I just commented it out. I think we can just fix this when we implement saving the blockchain to disk.
miner/miner.go
Outdated
| defer currentlyMiningLock.Unlock() | ||
| currentlyMining = true | ||
| // StopMining causes the miner to abort the current mining job immediately. | ||
| func StopMining() { |
There was a problem hiding this comment.
I would like to see these incorporated into a struct like we did with app. Package level state is an antipattern, even if you only expect that state to be instantiated once
miner/miner.go
Outdated
| return &MiningResult{ | ||
| Complete: false, | ||
| Info: MiningHalted, | ||
| if stateChanged() { |
There was a problem hiding this comment.
the stateChanged variable is probably unnecessary. Have you benchmarked this?
There was a problem hiding this comment.
Good point, will refactor.
miner/miner.go
Outdated
| @@ -9,9 +9,6 @@ import ( | |||
| "github.com/ubclaunchpad/cumulus/consensus" | |||
| ) | |||
|
|
|||
| // MinerState represents the state of the miner | |||
| type MinerState int | |||
|
|
|||
| const ( | |||
| // Paused represents the MinerState where the miner is not running but the | |||
| // previously running mining job can be resumed or stopped. | |||
There was a problem hiding this comment.
Could define these values as type MinerState (I know it's not your code 😝 )
miner/miner.go
Outdated
| // MiningResult contains the result of the mining operation. | ||
| type MiningResult struct { | ||
| Complete bool | ||
| Info int |
There was a problem hiding this comment.
Similarly, could define type for this
app/app.go
Outdated
| @@ -295,32 +308,34 @@ func (a *App) HandleTransaction(txn *blockchain.Transaction) { | |||
| // HandleBlock handles new instance of BlockWork. | |||
| func (a *App) HandleBlock(blk *blockchain.Block) { | |||
| log.Info("Received new block") | |||
| wasMining := a.Miner.PauseIfRunning() | |||
|
|
|||
| a.Chain.Lock.Lock() | |||
There was a problem hiding this comment.
Can we just add a Lock method to the chain?
| @@ -116,30 +116,42 @@ func connect(ctx *ishell.Context, a *App) { | |||
|
|
|||
There was a problem hiding this comment.
This is a nice interface 🎉
| defer currentlyMiningLock.Unlock() | ||
| currentlyMining = true | ||
| // setState synchronously sets the current state of the miner to the given state. | ||
| func (m *Miner) setState(state MinerState) { |
miner/miner_test.go
Outdated
| m := New() | ||
|
|
||
| go m.Mine(b) | ||
| time.Sleep(time.Second / 2) |
There was a problem hiding this comment.
Just do time.Millisecond * 50 - this type of stuff really slows our test suite down.
Related Issue
#128
Description
blockchain.gomake use of this lock. That's because with almost everything we do with the blockchain we check if certain conditions to do with the blockchain are true, do some validation, and then append a block, so locking each one of those steps individually wouldn't work - they all need to be done as one atomic operation.minerso that it can be paused and resumed during normal operation.WIKI Updates
TODO
Todos
General:
Other (links to TODOs in code):
TestEncodeDecodeBlockchain()