Merge pull request #415 from nspcc-dev/various-verification-fixes

This set of fixes allows Testnet synchronization (with verifyBlocks
set to true) to proceed up to the block number 4586, the block number
4587 fails at APPCALL invocation at the moment, but that's for the next
set of fixes.

pkg/core/block.go

@@ -46,6 +46,10 @@ func (b *Block) rebuildMerkleRoot() error {
 
 // Verify the integrity of the block.
 func (b *Block) Verify(full bool) bool {
+	// There has to be some transaction inside.
+	if len(b.Transactions) == 0 {
+		return false
+	}
 	// The first TX has to be a miner transaction.
 	if b.Transactions[0].Type != transaction.MinerType {
 		return false

pkg/core/blockchain.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"fmt"
 	"math"
+	"sort"
 	"sync/atomic"
 	"time"
 
@@ -22,6 +23,13 @@ import (
 const (
 	headerBatchCount = 2000
 	version          = "0.0.1"
+
+	// This one comes from C# code and it's different from the constant used
+	// when creating an asset with Neo.Asset.Create interop call. It looks
+	// like 2000000 is coming from the decrementInterval, but C# code doesn't
+	// contain any relationship between the two, so we should follow this
+	// behavior.
+	registeredAssetLifetime = 2 * 2000000
 )
 
 var (
@@ -200,8 +208,16 @@ func (bc *Blockchain) AddBlock(block *Block) error {
 	if expectedHeight != block.Index {
 		return fmt.Errorf("expected block %d, but passed block %d", expectedHeight, block.Index)
 	}
-	if bc.verifyBlocks && !block.Verify(false) {
-		return fmt.Errorf("block %s is invalid", block.Hash())
+	if bc.verifyBlocks {
+		if !block.Verify(false) {
+			return fmt.Errorf("block %s is invalid", block.Hash())
+		}
+		for _, tx := range block.Transactions {
+			err := bc.Verify(tx)
+			if err != nil {
+				return fmt.Errorf("transaction %s failed to verify: %s", tx.Hash().ReverseString(), err)
+			}
+		}
 	}
 	headerLen := bc.headerListLen()
 	if int(block.Index) == headerLen {
@@ -358,13 +374,14 @@ func (bc *Blockchain) storeBlock(block *Block) error {
 		switch t := tx.Data.(type) {
 		case *transaction.RegisterTX:
 			assets[tx.Hash()] = &AssetState{
-				ID:        tx.Hash(),
-				AssetType: t.AssetType,
-				Name:      t.Name,
-				Amount:    t.Amount,
-				Precision: t.Precision,
-				Owner:     t.Owner,
-				Admin:     t.Admin,
+				ID:         tx.Hash(),
+				AssetType:  t.AssetType,
+				Name:       t.Name,
+				Amount:     t.Amount,
+				Precision:  t.Precision,
+				Owner:      t.Owner,
+				Admin:      t.Admin,
+				Expiration: bc.BlockHeight() + registeredAssetLifetime,
 			}
 		case *transaction.IssueTX:
 		case *transaction.ClaimTX:
@@ -646,20 +663,20 @@ func (bc *Blockchain) GetConfig() config.ProtocolConfiguration {
 	return bc.config
 }
 
-// References returns a map with input prevHash as key (util.Uint256)
+// References returns a map with input coin reference (prevhash and index) as key
 // and transaction output as value from a transaction t.
 // @TODO: unfortunately we couldn't attach this method to the Transaction struct in the
 // transaction package because of a import cycle problem. Perhaps we should think to re-design
 // the code base to avoid this situation.
-func (bc *Blockchain) References(t *transaction.Transaction) map[util.Uint256]*transaction.Output {
-	references := make(map[util.Uint256]*transaction.Output, 0)
+func (bc *Blockchain) References(t *transaction.Transaction) map[transaction.Input]*transaction.Output {
+	references := make(map[transaction.Input]*transaction.Output)
 
 	for prevHash, inputs := range t.GroupInputsByPrevHash() {
 		if tx, _, err := bc.GetTransaction(prevHash); err != nil {
 			tx = nil
 		} else if tx != nil {
 			for _, in := range inputs {
-				references[in.PrevHash] = tx.Outputs[in.PrevIndex]
+				references[*in] = tx.Outputs[in.PrevIndex]
 			}
 		} else {
 			references = nil
@@ -867,17 +884,48 @@ func (bc *Blockchain) GetTransationResults(t *transaction.Transaction) []*transa
 	return results
 }
 
+// GetScriptHashesForVerifyingClaim returns all ScriptHashes of Claim transaction
+// which has a different implementation from generic GetScriptHashesForVerifying.
+func (bc *Blockchain) GetScriptHashesForVerifyingClaim(t *transaction.Transaction) ([]util.Uint160, error) {
+	hashes := make([]util.Uint160, 0)
+
+	claim := t.Data.(*transaction.ClaimTX)
+	clGroups := make(map[util.Uint256][]*transaction.Input)
+	for _, in := range claim.Claims {
+		clGroups[in.PrevHash] = append(clGroups[in.PrevHash], in)
+	}
+	for group, inputs := range clGroups {
+		refTx, _, err := bc.GetTransaction(group)
+		if err != nil {
+			return nil, err
+		}
+		for _, input := range inputs {
+			if len(refTx.Outputs) <= int(input.PrevIndex) {
+				return nil, fmt.Errorf("wrong PrevIndex reference")
+			}
+			hashes = append(hashes, refTx.Outputs[input.PrevIndex].ScriptHash)
+		}
+	}
+	if len(hashes) > 0 {
+		return hashes, nil
+	}
+	return nil, fmt.Errorf("no hashes found")
+}
+
 // GetScriptHashesForVerifying returns all the ScriptHashes of a transaction which will be use
 // to verify whether the transaction is bonafide or not.
 // Golang implementation of GetScriptHashesForVerifying method in C# (https://github.com/neo-project/neo/blob/master/neo/Network/P2P/Payloads/Transaction.cs#L190)
 func (bc *Blockchain) GetScriptHashesForVerifying(t *transaction.Transaction) ([]util.Uint160, error) {
+	if t.Type == transaction.ClaimType {
+		return bc.GetScriptHashesForVerifyingClaim(t)
+	}
 	references := bc.References(t)
 	if references == nil {
 		return nil, errors.New("Invalid operation")
 	}
 	hashes := make(map[util.Uint160]bool)
 	for _, i := range t.Inputs {
-		h := references[i.PrevHash].ScriptHash
+		h := references[*i].ScriptHash
 		if _, ok := hashes[h]; !ok {
 			hashes[h] = true
 		}
@@ -899,7 +947,7 @@ func (bc *Blockchain) GetScriptHashesForVerifying(t *transaction.Transaction) ([
 		if as == nil {
 			return nil, errors.New("Invalid operation")
 		}
-		if as.AssetType == transaction.DutyFlag {
+		if as.AssetType&transaction.DutyFlag != 0 {
 			for _, o := range outputs {
 				h := o.ScriptHash
 				if _, ok := hashes[h]; !ok {
@@ -918,7 +966,9 @@ func (bc *Blockchain) GetScriptHashesForVerifying(t *transaction.Transaction) ([
 
 }
 
-// VerifyWitnesses verify the scripts (witnesses) that come with a transactions.
+// VerifyWitnesses verify the scripts (witnesses) that come with a given
+// transaction. It can reorder them by ScriptHash, because that's required to
+// match a slice of script hashes from the Blockchain.
 // Golang implementation of VerifyWitnesses method in C# (https://github.com/neo-project/neo/blob/master/neo/SmartContract/Helper.cs#L87).
 // Unfortunately the IVerifiable interface could not be implemented because we can't move the References method in blockchain.go to the transaction.go file
 func (bc *Blockchain) VerifyWitnesses(t *transaction.Transaction) error {
@@ -931,6 +981,8 @@ func (bc *Blockchain) VerifyWitnesses(t *transaction.Transaction) error {
 	if len(hashes) != len(witnesses) {
 		return errors.Errorf("expected len(hashes) == len(witnesses). got: %d != %d", len(hashes), len(witnesses))
 	}
+	sort.Slice(hashes, func(i, j int) bool { return hashes[i].Less(hashes[j]) })
+	sort.Slice(witnesses, func(i, j int) bool { return witnesses[i].ScriptHash().Less(witnesses[j].ScriptHash()) })
 	for i := 0; i < len(hashes); i++ {
 		verification := witnesses[i].VerificationScript
 

pkg/core/blockchain_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/CityOfZion/neo-go/config"
 	"github.com/CityOfZion/neo-go/pkg/core/storage"
+	"github.com/CityOfZion/neo-go/pkg/core/transaction"
 	"github.com/CityOfZion/neo-go/pkg/io"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -38,9 +39,9 @@ func TestAddHeaders(t *testing.T) {
 func TestAddBlock(t *testing.T) {
 	bc := newTestChain(t)
 	blocks := []*Block{
-		newBlock(1),
-		newBlock(2),
-		newBlock(3),
+		newBlock(1, newTX(transaction.MinerType)),
+		newBlock(2, newTX(transaction.MinerType)),
+		newBlock(3, newTX(transaction.MinerType)),
 	}
 
 	for i := 0; i < len(blocks); i++ {
@@ -69,7 +70,7 @@ func TestAddBlock(t *testing.T) {
 
 func TestGetHeader(t *testing.T) {
 	bc := newTestChain(t)
-	block := newBlock(1)
+	block := newBlock(1, newTX(transaction.MinerType))
 	err := bc.AddBlock(block)
 	assert.Nil(t, err)
 

pkg/core/blockchainer.go

@@ -24,7 +24,7 @@ type Blockchainer interface {
 	GetAssetState(util.Uint256) *AssetState
 	GetAccountState(util.Uint160) *AccountState
 	GetTransaction(util.Uint256) (*transaction.Transaction, uint32, error)
-	References(t *transaction.Transaction) map[util.Uint256]*transaction.Output
+	References(t *transaction.Transaction) map[transaction.Input]*transaction.Output
 	Feer // fee interface
 	Verify(t *transaction.Transaction) error
 	GetMemPool() MemPool

pkg/network/helper_test.go

@@ -22,7 +22,7 @@ func (chain testChain) GetConfig() config.ProtocolConfiguration {
 	panic("TODO")
 }
 
-func (chain testChain) References(t *transaction.Transaction) map[util.Uint256]*transaction.Output {
+func (chain testChain) References(t *transaction.Transaction) map[transaction.Input]*transaction.Output {
 	panic("TODO")
 }
 

pkg/util/uint160.go

@@ -59,6 +59,18 @@ func (u Uint160) Equals(other Uint160) bool {
 	return u == other
 }
 
+// Less returns true if this value is less than given Uint160 value. It's
+// primarily intended to be used for sorting purposes.
+func (u Uint160) Less(other Uint160) bool {
+	for k := range u {
+		if u[k] == other[k] {
+			continue
+		}
+		return u[k] < other[k]
+	}
+	return false
+}
+
 // UnmarshalJSON implements the json unmarshaller interface.
 func (u *Uint160) UnmarshalJSON(data []byte) (err error) {
 	var js string

pkg/util/uint160_test.go

@@ -76,6 +76,21 @@ func TestUInt160Equals(t *testing.T) {
 	}
 }
 
+func TestUInt160Less(t *testing.T) {
+	a := "2d3b96ae1bcc5a585e075e3b81920210dec16302"
+	b := "2d3b96ae1bcc5a585e075e3b81920210dec16303"
+
+	ua, err := Uint160DecodeString(a)
+	assert.Nil(t, err)
+	ua2, err := Uint160DecodeString(a)
+	assert.Nil(t, err)
+	ub, err := Uint160DecodeString(b)
+	assert.Nil(t, err)
+	assert.Equal(t, true, ua.Less(ub))
+	assert.Equal(t, false, ua.Less(ua2))
+	assert.Equal(t, false, ub.Less(ua))
+}
+
 func TestUInt160String(t *testing.T) {
 	hexStr := "b28427088a3729b2536d10122960394e8be6721f"
 	hexRevStr := "1f72e68b4e39602912106d53b229378a082784b2"

pkg/vm/vm.go

@@ -907,6 +907,8 @@ func (v *VM) execute(ctx *Context, op Instruction) {
 		if err != nil {
 			panic(fmt.Sprintf("wrong parameters: %s", err.Error()))
 		}
+		// It's ok to have more keys than there are signatures (it would
+		// just mean that some keys didn't sign), but not the other way around.
 		if len(pkeys) < len(sigs) {
 			panic("more signatures than there are keys")
 		}
@@ -914,18 +916,24 @@ func (v *VM) execute(ctx *Context, op Instruction) {
 			panic("VM is not set up properly for signature checks")
 		}
 		sigok := true
+		// j counts keys and i counts signatures.
 		j := 0
-		for i := 0; sigok && i < len(pkeys) && j < len(sigs); {
+		for i := 0; sigok && j < len(pkeys) && i < len(sigs); {
 			pkey := &keys.PublicKey{}
 			err := pkey.DecodeBytes(pkeys[j])
 			if err != nil {
 				panic(err.Error())
 			}
+			// We only move to the next signature if the check was
+			// successful, but if it's not maybe the next key will
+			// fit, so we always move to the next key.
 			if pkey.Verify(sigs[i], v.checkhash) {
 				i++
 			}
 			j++
-			if len(pkeys)-i > len(sigs)-j {
+			// When there are more signatures left to check than
+			// there are keys the check won't successed for sure.
+			if len(sigs)-i > len(pkeys)-j {
 				sigok = false
 			}
 		}