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

ValidTransaction and ValidBlock #28

Merged
merged 32 commits into from
May 28, 2017
Merged

Conversation

chadlagore
Copy link
Contributor

@chadlagore chadlagore commented May 17, 2017

Status: Ready to merge.

What Changed

  • Implementing ValidTransaction and ValidBlock.

Opening this up for visibility early! ☺️ Not done yet.

I've changed the base branch to dev

TODO

  • Write tests.
  • Signature verification.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 82.126% when pulling 9ba1b5a on 15-implement-verification into 793d893 on 19-blockchain-tests.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.2%) to 81.1% when pulling df84394 on 15-implement-verification into 793d893 on 19-blockchain-tests.

@jordanschalm jordanschalm added this to the Sprint 5 milestone May 27, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+6.4%) to 88.713% when pulling 7dbd2f4 on 15-implement-verification into 793d893 on 19-blockchain-tests.

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.

Looks good. Couple extra things to check in validation (maybe add a test case for them) and we should add an issue for figuring out how to handle spending mined coins.

// Find the transaction input (I) in the chain (by hash)
var I *Transaction
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use input rather than I

if inAmount != outAmount {
return false
}

Copy link
Member

Choose a reason for hiding this comment

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

We also need to check that input transaction hasn't been spent between when it was created and now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🥇 I've added a test case.

return false
}

// Verify every Transaction in the block.
for _, t := range b.Transactions {
if !bc.ValidTransaction(t) {
Copy link
Member

Choose a reason for hiding this comment

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

What if two transactions in b.Transactions reference the same input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🥇 I've added a test case.

return false
}

// Verify every Transaction in the block.
for _, t := range b.Transactions {
Copy link
Member

Choose a reason for hiding this comment

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

We also need to discuss how to spend coins that were mined, since there is no transaction for them to refer to. Do we have the miner insert a special transaction for their mined coins, or modify the Transaction struct to be able to refer to block rewards as inputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like setting the Transaction.Input to some special value is a good way to go. Perhaps then we could get away with treating it as a normal block during verification (ie. I'd like to avoid special cases in the VerifyX functions). By convention, we could put the block reward in Transactions[0].

Copy link
Member

@jordanschalm jordanschalm May 28, 2017

Choose a reason for hiding this comment

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

Added an issue for this

@chadlagore chadlagore changed the title [WIP] ValidTransaction and ValidBlock ValidTransaction and ValidBlock May 28, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+6.4%) to 88.713% when pulling 46358ab on 15-implement-verification into 793d893 on 19-blockchain-tests.

@coveralls
Copy link

Coverage Status

Coverage increased (+6.4%) to 88.713% when pulling 4b24424 on 15-implement-verification into 793d893 on 19-blockchain-tests.

@chadlagore chadlagore changed the base branch from 19-blockchain-tests to master May 28, 2017 06:01
@chadlagore chadlagore changed the base branch from master to dev May 28, 2017 06:02
@coveralls
Copy link

coveralls commented May 28, 2017

Coverage Status

Coverage increased (+8.6%) to 88.61% when pulling 61f8166 on 15-implement-verification into 3389f7a on dev.

@jordanschalm
Copy link
Member

Going to merge this now @chadlagore

@jordanschalm jordanschalm merged commit c61ff55 into dev May 28, 2017
@jordanschalm jordanschalm deleted the 15-implement-verification branch May 28, 2017 22:40
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

3 participants