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

Claim and enrollment TX verification fixes #696

Merged
merged 10 commits into from
Feb 27, 2020
Merged

Conversation

roman-khimov
Copy link
Member

Problem

Missing some checks for these transactions. Broken SpentCoin tracking logic.

That should be done for Enrollment transactions.
We don't need a pointer here and this change makes this field compatible with
Transaction.Inputs which is useful in many scenarios.
Everywhere in this code prevHash == input.PrevHash, thus we can easily move
some common code out of the loop saving on DB accesses and
serialization/deserialization.
prevHash == input.PrevHash, so make less DB accesses and more real work. Fix
some bugs along the way:
 * spentCoins structure may already be present in the DB when persisting TX,
   there is nothing wrong with that and we shouldn't overwrite it
 * it's only used for NEO and only to check for claim validity. Thus, when
   processing claim tx the corresponding spentCoins should always be present
   in the DB
@roman-khimov roman-khimov added this to the v0.74.0 milestone Feb 27, 2020
pkg/core/blockchain.go Outdated Show resolved Hide resolved
pkg/core/transaction/input.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

Merging #696 into master will increase coverage by 0.02%.
The diff coverage is 34.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
+ Coverage   65.73%   65.75%   +0.02%     
==========================================
  Files         128      128              
  Lines       10827    10883      +56     
==========================================
+ Hits         7117     7156      +39     
- Misses       3432     3450      +18     
+ Partials      278      277       -1
Impacted Files Coverage Δ
pkg/core/transaction/claim.go 100% <ø> (ø) ⬆️
pkg/core/transaction/transaction.go 63.44% <ø> (+3.84%) ⬆️
pkg/core/blockchain.go 30.12% <10.12%> (-0.34%) ⬇️
pkg/core/transaction/input.go 100% <100%> (ø) ⬆️
pkg/core/dao.go 66.55% <11.76%> (-3.31%) ⬇️
pkg/core/interop_neo.go 53.39% <40%> (-0.11%) ⬇️
pkg/core/spent_coin_state.go 100% <0%> (+20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56f87cd...00d199e. Read the comment docs.

Which allows to use it for ClaimTX. Make it also a bit more efficient as maps
are expensive.
We don't need a map here, use simpler structures.
pkg/core/blockchain.go Outdated Show resolved Hide resolved
pkg/core/transaction/input.go Outdated Show resolved Hide resolved
…etter

Claim transactions should _add_ claims scripthashes to the standard list, not
replace them. And this code is actually very reusable.
Refactor HaveInputsDuplicate() out of the core and Blockchain, it doesn't
depend on the Blockchain state. Make it more efficient.
Add doesn't change the variable state. Thanks to GolangCI for catching this.
@roman-khimov roman-khimov merged commit 7d59fa0 into master Feb 27, 2020
@roman-khimov roman-khimov deleted the tx-verification-fixes branch February 27, 2020 09:45
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