Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

25 mining proof of work #66

Merged
merged 21 commits into from
Jun 21, 2017
Merged

25 mining proof of work #66

merged 21 commits into from
Jun 21, 2017

Conversation

david-julien
Copy link
Contributor

Initial mining proof of work code, not all tests written yet

Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Really good! I think there are some more simplifications we can add

@@ -1,9 +1,38 @@
package blockchain
Copy link
Member

Choose a reason for hiding this comment

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

Move all the miner stuff into its own package

// HashLen is the length in bytes of a hash.
HashLen = 32
// MaxDifficultyHex is the maximum difficulty value represented as a hex string
MaxDifficultyHex = "000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF"
Copy link
Member

Choose a reason for hiding this comment

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

Can this be replace by using math.big?

@@ -26,3 +55,43 @@ type Marshaller interface {
func HashSum(m Marshaller) Hash {
return sha256.Sum256(m.Marshal())
}

// CompareTo compares two hashes, it returns true if the operation of the first hash on the second hash specified by the comparator is true, and false otherwise
func CompareTo(h1 Hash, h2 Hash, comparator int) bool {
Copy link
Member

Choose a reason for hiding this comment

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

We will only ever be checking for hashes being less than right? Can we simplify this accordingly and maybe remove those comparator constants unless we need arbitrary comparisons?

Copy link
Contributor Author

@david-julien david-julien Jun 12, 2017

Choose a reason for hiding this comment

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

yes we need all of them for miningheader verification purposes before mining, we'll see if i can clean this up when we start using math.big

Copy link
Contributor Author

@david-julien david-julien Jun 13, 2017

Choose a reason for hiding this comment

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

math.big uses the exact same arbitrary constants for comparison, so if we wanted to compare two hashes we would have to convert to math.big and then use the same constants.

https://golang.org/src/math/big/float.go?s=43528:43561#L1637

}

// HexStringToHash converts a big endian hex string to a hash
func HexStringToHash(s string) Hash {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be replaced by math.big?


// Mine continuously increases the nonce and tries to verify the proof of work until the puzzle is solved
func (mh *MiningHeader) Mine() bool {
if mh.VerifyMiningHeader() == false {
Copy link
Member

Choose a reason for hiding this comment

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

use ! rather than comparing to false

return false
}

for mh.VerifyProofOfWork() == false {
Copy link
Member

Choose a reason for hiding this comment

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

same here

}

// AppendUint32ToSlice converts a uint32 to a byte slice and appends it to a byte slice
func AppendUint32ToSlice(s *[]byte, num uint32) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this function? (and below) seems like they're only called once

}

// SetMiningHeader sets the mining header
func (mh *MiningHeader) SetMiningHeader(lastBlock Hash, rootHash Hash, target Hash) {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than a setter can we just use composite instantiation? What does the setter buy us?

ie. mh := MiningHeader{lastBlock, rootHash, target}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it sets the time to the time variable to the current time in the miningheader struct, not quite sure how to set default values in golang structs

Copy link
Member

Choose a reason for hiding this comment

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

The default value is the zero value for the type. You can do something like:

mh := MiningHeader{
  LastBlock: lastBlock,
  ...
  timeField: time.Now(),
}

Copy link
Member

@bfbachmann bfbachmann left a comment

Choose a reason for hiding this comment

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

Great job 🥇

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.1%) to 79.248% when pulling 0666a58 on 25-mining-proof-of-work into b120ac0 on dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-7.1%) to 79.248% when pulling 0666a58 on 25-mining-proof-of-work into b120ac0 on dev.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 86.316% when pulling 9401181 on 25-mining-proof-of-work into b120ac0 on dev.

// HexString returns the hexadecimal representation of the hash
func (h Hash) HexString() string {
var hashBigEndian Hash
for i := 0; i < HashLen; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice this pattern of inverting the hex string a couple of times, perhaps a helper function?

@jordanschalm jordanschalm changed the base branch from dev to transaction-pooling June 17, 2017 19:46
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Awesome, if we can remove dependence on literal hex strings and use math.big instead that would be awesome, otherwise ready to merge with some more test coverage.

t.Fail()
}

if h.ParseString("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff") != nil {
Copy link
Member

Choose a reason for hiding this comment

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

@david-julien Is it possible to replace these huge hex strings with math.big. I don't like putting big string literal hexes all over the code.

Rather than doing hash.ParseString() to get a hash from a big number, can we use the math.big exponentiation function like z.Exp(2, 50, 0) to get 2^50.

If we need the ParseString function, do func ParseString(s string) Hash rather than having the hash as the receiver.

Copy link
Contributor

Choose a reason for hiding this comment

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

Was trying to articulate this very thing.

}

// ParseString sets the hash to the the value represented by a hexadecimal string, and returns the hash
func (h *Hash) ParseString(s string) *Hash {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to remove the need for hex strings if possible. See comment below

Copy link
Contributor

@chadlagore chadlagore left a comment

Choose a reason for hiding this comment

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

This is great 🥇 Nothing blocking but I think ParseString could be broken out into helper functions, and the API around CompareTo could be a little more clear. Up to you for changes, otherwise 👍.

"testing"
)

func TestParseString(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👏 Great coverage. But I wonder if ParseString couldn't be broken out into a set of helper functions, with a test for each so that when something breaks we have a clearer idea of what part broke?

miner/miner.go Outdated

// VerifyProofOfWork computes the hash of the MiningHeader and returns true if the result is less than the target
func (mh *MiningHeader) VerifyProofOfWork() bool {
if mh.DoubleHashSum().CompareTo(mh.Target, bc.LessThan) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking, but in terms of convention we should consider the use of bc as a namespace vs a concrete instance of a Blockchain. I've been using it as the later, but I'm game for whatever as long as we're consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also here the namespacing does not really tell me what LessThan is. I wonder if

  • hash.GreaterThan(otherHash)
  • hash.LessThan(otherHash)
  • hash.EqualTo(otherHash)

is a more clear API?

Copy link
Member

Choose a reason for hiding this comment

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

You could make this a one liner by hitting a little:

return mh.DoubleHashSum().CompareTo(mh.Target, bc.LessThan)

@@ -0,0 +1,94 @@
package miner
Copy link
Contributor

@chadlagore chadlagore Jun 17, 2017

Choose a reason for hiding this comment

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

Can we introduce an interface for MiningHeader or maybe Miner? I can imagine a scenario where we want to do an integration test where we mine a block (without waiting).

Copy link
Member

@bfbachmann bfbachmann left a comment

Choose a reason for hiding this comment

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

🥇

miner/miner.go Outdated

// VerifyProofOfWork computes the hash of the MiningHeader and returns true if the result is less than the target
func (mh *MiningHeader) VerifyProofOfWork() bool {
if mh.DoubleHashSum().CompareTo(mh.Target, bc.LessThan) {
Copy link
Member

Choose a reason for hiding this comment

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

You could make this a one liner by hitting a little:

return mh.DoubleHashSum().CompareTo(mh.Target, bc.LessThan)

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.08%) to 84.516% when pulling 8446f1b on 25-mining-proof-of-work into 17eeffd on transaction-pooling.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 84.516% when pulling ef80df5 on 25-mining-proof-of-work into 4148a01 on transaction-pooling.

@jordanschalm jordanschalm changed the base branch from transaction-pooling to dev June 21, 2017 01:06
@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 70.697% when pulling f110208 on 25-mining-proof-of-work into e6bf31e on dev.

@jordanschalm jordanschalm merged commit a988383 into dev Jun 21, 2017
@jordanschalm jordanschalm deleted the 25-mining-proof-of-work branch June 21, 2017 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants