110 refactor consensus and validation #112

david-julien · 2017-07-26T20:47:11Z

Related Issue

110

Description

Write a few sentences describing the overall goal of this PR.
- Refactoring the validation code.
- Merged validMinedBlock and validBlock together.
List the general components of the application that this PR will affect.
- Validation file in block chain was integrated into the consensus package.
- Errors from validation were moved into their own file in the consensus package.
- CurrentDifficulty and CurrentBlockReward were moved into their own file in the consensus package.

WIKI Updates

Consensus

Todos

General:

Tests
Documentation
Wiki

Other (links to TODOs in code):

coveralls · 2017-07-26T20:49:54Z

Coverage decreased (-0.3%) to 69.93% when pulling a5f6df4 on 110-refactor-consensus-and-validation into d630511 on dev.

coveralls · 2017-07-26T21:11:41Z

Coverage decreased (-0.3%) to 69.93% when pulling 3c926a6 on 110-refactor-consensus-and-validation into d630511 on dev.

jordanschalm

Couple minor things. Looks good!

jordanschalm · 2017-07-27T00:46:01Z

common/constants/big.go

 	// Big2Exp256 is 2^256 respresented as type "math/big"
 	Big2Exp256 = util.BigExp(2, 256)
 )

 // Commonly used max values represented as type "math/big"
 var (
+	// MaxUint256 is the maximum uint232 number represented as type "math/big"


MaxUint256 should be 232?

yah so here's the thing.... The tests will only pass if the MaxTarget is set to MaxUint256 because of this guy: proof of work in validation.
So I dunno how to be like, if you're in test mode, MaxTarget is equal to MaxUint256, otherwise please use MaxUint232. Cuz MaxUint232 is the max target for v1.

jordanschalm · 2017-07-27T00:51:14Z

consensus/consensus.go

-)
+	// Find the transaction input in the chain (by hash)
+	var input *blockchain.Transaction
+	input = bc.GetInputTransaction(t)


can do input := ...

bfbachmann

Great job 🏅

bfbachmann · 2017-07-27T02:29:59Z

consensus/consensus.go

+	}
+
+	// Check that time is not greater than current time or equal to 0.
+	if uint32(gb.Time) == 0 {


Seems like you're just checking if it's 0 here, not whether it's greater than the current time.

there's some more complex time checking stuff that @jordanschalm and I were talking about so we just stuck with this simple check for now. I should probably add a TODO though.

bfbachmann · 2017-07-27T02:33:27Z

consensus/consensus.go

+		return true, ValidBlock
+	}
+
+	// Check that block number is between 0 and max blocks.


I would change this to: "Check that block number is valid" or something similar because "max blocks" isn't really correct terminology here.

bfbachmann · 2017-07-27T02:35:14Z

consensus/consensus.go

 		return false, BadTarget
 	}

+	// Check that time is not greater than current time or equal to 0


Check current time here too.

same as above^

bfbachmann · 2017-07-27T02:38:03Z

consensus/current.go

@@ -0,0 +1,40 @@
+package consensus


I like this.

I'm glad you like this bruno 😗😗

bfbachmann · 2017-07-27T02:38:21Z

consensus/errors.go

@@ -0,0 +1,106 @@
+package consensus


I like this too.

bfbachmann · 2017-07-27T02:39:31Z

miner/miner.go

@@ -42,13 +41,6 @@ func RestartMiner(bc *blockchain.BlockChain, b *blockchain.Block) {
 // 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 {


Why'd you remove this? Are only valid blocks passed to this function?

The miner receives blocks from the pool, and the app orchestrates. No need to re-validate 👍

chadlagore

Lookin cool 🕶

chadlagore · 2017-07-27T03:30:19Z

consensus/consensus.go

+	// Verify every Transaction in the block.
+	for _, t := range b.Transactions[1:] {
+		if valid, code := VerifyTransaction(bc, t); !valid {
+			log.Errorf("Invalid Transaction, TransactionCode: %d", code)


If we want to log every bad transaction, thats cool but we could probably dry it up a bit by putting these logs at the caller.

chadlagore · 2017-07-27T03:32:48Z

miner/miner.go

@@ -42,13 +41,6 @@ func RestartMiner(bc *blockchain.BlockChain, b *blockchain.Block) {
 // 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 {


The miner receives blocks from the pool, and the app orchestrates. No need to re-validate 👍

chadlagore · 2017-07-27T03:35:15Z

miner/miner_test.go

@@ -75,11 +67,11 @@ func TestCloudBase(t *testing.T) {

 	CloudBase(b, bc, w.Public())


Its still difficult for me to appreciate that this function is mutating the block and adding a new transaction to it. Can we rename it sometime? 🙏

hahaha sure I thought I was being clever but it is sort of hard to understand isn't it 😂

chadlagore · 2017-07-27T03:36:55Z

consensus/consensus.go

+	// Test if identical transaction already exists in chain.
+	end := uint32(len(bc.Blocks))
+	start := t.Input.BlockNumber
+	if exists, _, _ := bc.ContainsTransaction(t, start, end); exists {


I still worry that we have a hole here, but that is out of scope.

See #113.

Yea I was worried that we are only checking the blockchain from the input transaction to the current block, but I guess that makes sense because the earlier of the two double spend transactions could not have come before it's own input transaction. Am I right there?

All this ContainsTransaction bit does is check if any transaction in the chain hashes to HashSum(t). It literally asserts HashSum(t) != HashSum(t0) for all intervening transactions.

If Alice has 10 and sends Bob 10 then sends Bob 9.99, I don't think we catch it because the outputs look different and that changes the hashes. We may catch it elsewhere, but I can't see it 🤔

I didn't write this haha so I wasn't trying to fix anything from 113. I think imma leave it as is, as its a re-factor and we'll handle the errors from 113 on another PR.

Yeah definitely out of scope, ignore us @david-julien 😄

chadlagore · 2017-07-27T03:40:03Z

common/constants/big.go

@@ -12,12 +12,25 @@ var (
 	Big0 = big.NewInt(0)
 	// Big1 is 1 represented as type "math/big"
 	Big1 = big.NewInt(1)
+	// Big2Exp232 is 2^232 respresented as type "math/big"


I see how these are slightly convenient, but I probably will avoid using them because they obfuscate the code.

b := big.NewInt(1) + util.BigExp(2, 232) 👈 pretty clear what I'm doing
b := Big1 + Big2Exp232 🤔

Its not just for clarity but it also helps us to avoid allocating new memory for the same values over and over again throughout the code as they are pointers.

chadlagore · 2017-07-27T03:41:07Z

consensus/consensus.go

-	BadCloudBase MinedBlockCode = iota
-)
+// VerifyTransaction tests whether a transaction valid.
+func VerifyTransaction(bc *blockchain.BlockChain,


This was my silly formatting using PEP8 on my go code from before 🤦‍♂️ . Feel free to undo it.

Haha no I like it! Trying to keep the line length under 80 chars

I know the urge 😅

chadlagore · 2017-07-27T03:46:04Z

consensus/consensus.go

+	// cumulus. Once the difficulty is dynamic, the target would need to be
+	// compared to the target calculated for that specific block.
+	target := blockchain.HashToBigInt(b.Target)
+	if target.Cmp(c.MaxTarget) == 1 ||


Doesn't this work:

if target.Cmp(blockchain.HashToBigInt(CurrentTarget())) != 0 { return false, BadTarget }

Why would the CurrentTarget() return something out of range?

good call! These range checks are code from before the re-factor that I forgot to get rid of. oops 😅

chadlagore · 2017-07-27T04:12:16Z

Sorry for the merge conflict, happy to see the validation finally move out of the blockchain package!

coveralls · 2017-07-27T17:53:56Z

Coverage decreased (-0.3%) to 70.718% when pulling 56ffabf on 110-refactor-consensus-and-validation into 5e0015e on dev.

david-julien added 2 commits July 26, 2017 16:38

Initial refactor, moving all validation into consensus

6e1c89b

current_test.go

a5f6df4

fixed CurrentBlockReward bug and created current block reward test

3c926a6

david-julien requested a review from jordanschalm July 26, 2017 21:15

jordanschalm approved these changes Jul 27, 2017

View reviewed changes

bfbachmann approved these changes Jul 27, 2017

View reviewed changes

chadlagore reviewed Jul 27, 2017

View reviewed changes

david-julien added 2 commits July 27, 2017 00:31

PR fixes

3e4c2a4

pulled dev

56ffabf

jordanschalm merged commit 7eee1b7 into dev Jul 29, 2017

jordanschalm deleted the 110-refactor-consensus-and-validation branch July 29, 2017 19:33

		@@ -75,11 +67,11 @@ func TestCloudBase(t *testing.T) {

		CloudBase(b, bc, w.Public())

110 refactor consensus and validation #112

110 refactor consensus and validation #112

Conversation

david-julien commented Jul 26, 2017 • edited Loading

Related Issue

Description

WIKI Updates

Todos

coveralls commented Jul 26, 2017

coveralls commented Jul 26, 2017

jordanschalm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-julien Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

bfbachmann Jul 27, 2017 • edited Loading

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

chadlagore Jul 27, 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

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 commented Jul 27, 2017

coveralls commented Jul 27, 2017

david-julien commented Jul 26, 2017 •

edited

Loading

david-julien Jul 27, 2017 •

edited

Loading

bfbachmann Jul 27, 2017 •

edited

Loading

chadlagore Jul 27, 2017 •

edited

Loading