From c3591d88976b828ec76108e925e61b78fbb48ab0 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 30 Sep 2019 16:52:35 +0300 Subject: [PATCH 1/8] vm: fix CHECKMULTISIG to comply with NEO VM implementation Testing with testnet quickly revealed that our code has an issue when the key count doesn't equal signature count, fix it and add some comments. --- pkg/vm/vm.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index a3ad75215..49f20259b 100644 --- a/pkg/vm/vm.go +++ b/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 } } From 4ae18e8ffcf789fc86f1c0575c0b8e678150918e Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 30 Sep 2019 16:55:02 +0300 Subject: [PATCH 2/8] block: check for Transaction length before messing with txes Fixes panic two lines below. Blocks without transactions are invalid by definition, so there is a need to adjust tests accordingly. --- pkg/core/block.go | 4 ++++ pkg/core/blockchain_test.go | 9 +++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/core/block.go b/pkg/core/block.go index 97fdeabce..fddb06eba 100644 --- a/pkg/core/block.go +++ b/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 diff --git a/pkg/core/blockchain_test.go b/pkg/core/blockchain_test.go index 1943d1610..8dab93802 100644 --- a/pkg/core/blockchain_test.go +++ b/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) From 832c56f97dfce830287e7ffa0b3d8363fe5a66ab Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 30 Sep 2019 16:57:24 +0300 Subject: [PATCH 3/8] core: fix DutyFlag check in GetScriptHashesForVerifying() It's a flag (used by Share and Invoice asset types), so it should be checked like a flag, the same way C# node does. --- pkg/core/blockchain.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index 03e75bedd..c5edaf298 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -899,7 +899,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 { From 1095abb04ca9d478c79e50eec21b898aa4965376 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 30 Sep 2019 17:27:41 +0300 Subject: [PATCH 4/8] core: fix default asset expiration to match C# code Fixes bogus verification failures for non-native assets. --- pkg/core/blockchain.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index c5edaf298..f4ac54a61 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -22,6 +22,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 ( @@ -358,13 +365,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: From 854fb187a30e2a91adee5e2a22cef4476dbdd308 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 30 Sep 2019 17:35:11 +0300 Subject: [PATCH 5/8] core: verify transactions in AddBlock() --- pkg/core/blockchain.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index f4ac54a61..b9720a261 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -207,8 +207,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 { From dd0dd2d03111369d4be67ecea694288e8c152baa Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 30 Sep 2019 17:37:04 +0300 Subject: [PATCH 6/8] core: add claim-specific GetScriptHashesForVerifying() Claim transactions have different logic in C# node, so we need to implement it too. It's not the most elegant way to fix it, but let's make it work first and then refactor if and where needed. Fixes verification of Claim transactions. --- pkg/core/blockchain.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index b9720a261..88bf4e5d8 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -883,10 +883,41 @@ 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") From 8537700b7b006ee317ce3a25c4ce4831d18eee48 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 30 Sep 2019 17:39:42 +0300 Subject: [PATCH 7/8] core: sort hashes and witnesses in VerifyWitnesses() Fixes failure to verify some multi-witnesses transactions in Testnet chain. C# code contains similar logic. --- pkg/core/blockchain.go | 7 ++++++- pkg/util/uint160.go | 12 ++++++++++++ pkg/util/uint160_test.go | 15 +++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index 88bf4e5d8..fb7db4eaf 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "math" + "sort" "sync/atomic" "time" @@ -965,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 { @@ -978,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 diff --git a/pkg/util/uint160.go b/pkg/util/uint160.go index 8c9202935..f2cd6535f 100644 --- a/pkg/util/uint160.go +++ b/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 diff --git a/pkg/util/uint160_test.go b/pkg/util/uint160_test.go index 0d64a5623..06db92ea3 100644 --- a/pkg/util/uint160_test.go +++ b/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" From b7c1efe7e3b404f5ad2a3528156fb5a13c834c75 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Mon, 30 Sep 2019 17:42:37 +0300 Subject: [PATCH 8/8] core: fix References() result key type If the block references two ouputs in some other transaction the code failed to verify it because of key collision. C# code implements it properly by using full CoinReference type as a key, so let's do it in a similar fashion. --- pkg/core/blockchain.go | 10 +++++----- pkg/core/blockchainer.go | 2 +- pkg/network/helper_test.go | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/core/blockchain.go b/pkg/core/blockchain.go index fb7db4eaf..3f955fd06 100644 --- a/pkg/core/blockchain.go +++ b/pkg/core/blockchain.go @@ -663,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 @@ -925,7 +925,7 @@ func (bc *Blockchain) GetScriptHashesForVerifying(t *transaction.Transaction) ([ } 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 } diff --git a/pkg/core/blockchainer.go b/pkg/core/blockchainer.go index ba0b6d732..53f1d6400 100644 --- a/pkg/core/blockchainer.go +++ b/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 diff --git a/pkg/network/helper_test.go b/pkg/network/helper_test.go index 5580f3873..09fafb87d 100644 --- a/pkg/network/helper_test.go +++ b/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") }