Repository: kaspanet/kaspad
Pull request #1722 (2021-05-13 15:17:35)

Fix overflow when checking coinbase maturity and don't ban peers that send transactions with immature spend
This depends on #1723 

codecov[bot]: # [Codecov](https://codecov.io/gh/kaspanet/kaspad/pull/1722?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet) Report
> Merging [#1722](https://codecov.io/gh/kaspanet/kaspad/pull/1722?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet) (c599c3d) into [v0.10.2-dev](https://codecov.io/gh/kaspanet/kaspad/commit/04dc1947ffce9088f28f074eab0efcd2589df7d1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet) (04dc194) will **increase** coverage by `0.02%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/kaspanet/kaspad/pull/1722/graphs/tree.svg?width=650&height=150&src=pr&token=2FBIEEJTNU&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet)](https://codecov.io/gh/kaspanet/kaspad/pull/1722?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet)

```diff
@@               Coverage Diff               @@
##           v0.10.2-dev    #1722      +/-   ##
===============================================
+ Coverage        59.38%   59.40%   +0.02%     
===============================================
  Files              551      551              
  Lines            21967    21968       +1     
===============================================
+ Hits             13045    13051       +6     
+ Misses            6843     6837       -6     
- Partials          2079     2080       +1     
```


| [Impacted Files](https://codecov.io/gh/kaspanet/kaspad/pull/1722?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet) | Coverage Δ | |
|---|---|---|
| [domain/miningmanager/mempool/error.go](https://codecov.io/gh/kaspanet/kaspad/pull/1722/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet#diff-ZG9tYWluL21pbmluZ21hbmFnZXIvbWVtcG9vbC9lcnJvci5nbw==) | `26.31% <ø> (+5.26%)` | :arrow_up: |
| [...ses/transactionvalidator/transaction\_in\_context.go](https://codecov.io/gh/kaspanet/kaspad/pull/1722/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet#diff-ZG9tYWluL2NvbnNlbnN1cy9wcm9jZXNzZXMvdHJhbnNhY3Rpb252YWxpZGF0b3IvdHJhbnNhY3Rpb25faW5fY29udGV4dC5nbw==) | `71.33% <100.00%> (+2.35%)` | :arrow_up: |
| [domain/miningmanager/mempool/mempool.go](https://codecov.io/gh/kaspanet/kaspad/pull/1722/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet#diff-ZG9tYWluL21pbmluZ21hbmFnZXIvbWVtcG9vbC9tZW1wb29sLmdv) | `55.23% <100.00%> (+0.26%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kaspanet/kaspad/pull/1722?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kaspanet/kaspad/pull/1722?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet). Last update [04dc194...c599c3d](https://codecov.io/gh/kaspanet/kaspad/pull/1722?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kaspanet).


Patch:
diff --git a/domain/consensus/processes/transactionvalidator/transaction_in_context.go b/domain/consensus/processes/transactionvalidator/transaction_in_context.go
index 4f7b8bb0..a9a16ec4 100644
--- a/domain/consensus/processes/transactionvalidator/transaction_in_context.go
+++ b/domain/consensus/processes/transactionvalidator/transaction_in_context.go
@@ -68,8 +68,7 @@ func (v *transactionValidator) checkTransactionCoinbaseMaturity(stagingArea *mod
 			missingOutpoints = append(missingOutpoints, &input.PreviousOutpoint)
 		} else if utxoEntry.IsCoinbase() {
 			originDAAScore := utxoEntry.BlockDAAScore()
-			daaScoreSincePrev := povDAAScore - originDAAScore
-			if daaScoreSincePrev < v.blockCoinbaseMaturity {
+			if originDAAScore+v.blockCoinbaseMaturity > povDAAScore {
 				return errors.Wrapf(ruleerrors.ErrImmatureSpend, "tried to spend coinbase "+
 					"transaction output %s from DAA score %d "+
 					"to DAA score %d before required maturity "+
diff --git a/domain/miningmanager/mempool/error.go b/domain/miningmanager/mempool/error.go
index 001bb4da..bc51bb3a 100644
--- a/domain/miningmanager/mempool/error.go
+++ b/domain/miningmanager/mempool/error.go
@@ -49,6 +49,7 @@ const (
 	RejectInsufficientFee RejectCode = 0x42
 	RejectFinality        RejectCode = 0x43
 	RejectDifficulty      RejectCode = 0x44
+	RejectImmatureSpend   RejectCode = 0x45
 )
 
 // Map of reject codes back strings for pretty printing.
@@ -63,6 +64,7 @@ var rejectCodeStrings = map[RejectCode]string{
 	RejectFinality:        "REJECT_FINALITY",
 	RejectDifficulty:      "REJECT_DIFFICULTY",
 	RejectNotRequested:    "REJECT_NOTREQUESTED",
+	RejectImmatureSpend:   "REJECT_IMMATURESPEND",
 }
 
 // String returns the RejectCode in human-readable form.
diff --git a/domain/miningmanager/mempool/mempool.go b/domain/miningmanager/mempool/mempool.go
index 9ff7c9d6..79576dc8 100644
--- a/domain/miningmanager/mempool/mempool.go
+++ b/domain/miningmanager/mempool/mempool.go
@@ -720,6 +720,9 @@ func (mp *mempool) maybeAcceptTransaction(tx *consensusexternalapi.DomainTransac
 		if errors.As(err, &missingOutpoints) {
 			return missingOutpoints.MissingOutpoints, nil, nil
 		}
+		if errors.Is(err, ruleerrors.ErrImmatureSpend) {
+			return nil, nil, txRuleError(RejectImmatureSpend, "one of the transaction inputs spends an immature UTXO")
+		}
 		if errors.As(err, &ruleerrors.RuleError{}) {
 			return nil, nil, newRuleError(err)
 		}


Test patch:
diff --git a/domain/consensus/processes/transactionvalidator/transactionvalidator_test.go b/domain/consensus/processes/transactionvalidator/transactionvalidator_test.go
index 6327d3ad..81c95e49 100644
--- a/domain/consensus/processes/transactionvalidator/transactionvalidator_test.go
+++ b/domain/consensus/processes/transactionvalidator/transactionvalidator_test.go
@@ -78,7 +78,17 @@ func TestValidateTransactionInContextAndPopulateMassAndFee(t *testing.T) {
 				true,
 				uint64(5)),
 		}
-		txInputWithMaxSequence := externalapi.DomainTransactionInput{
+		immatureInput := externalapi.DomainTransactionInput{
+			PreviousOutpoint: prevOutPoint,
+			SignatureScript:  []byte{},
+			Sequence:         constants.MaxTxInSequenceNum,
+			UTXOEntry: utxo.NewUTXOEntry(
+				100_000_000, // 1 KAS
+				scriptPublicKey,
+				true,
+				uint64(6)),
+		}
+		txInputWithSequenceLockTimeIsSeconds := externalapi.DomainTransactionInput{
 			PreviousOutpoint: prevOutPoint,
 			SignatureScript:  []byte{},
 			Sequence:         constants.SequenceLockTimeIsSeconds,
@@ -110,7 +120,7 @@ func TestValidateTransactionInContextAndPopulateMassAndFee(t *testing.T) {
 
 		validTx := externalapi.DomainTransaction{
 			Version:      constants.MaxTransactionVersion,
-			Inputs:       []*externalapi.DomainTransactionInput{&txInputWithMaxSequence},
+			Inputs:       []*externalapi.DomainTransactionInput{&txInputWithSequenceLockTimeIsSeconds},
 			Outputs:      []*externalapi.DomainTransactionOutput{&txOut},
 			SubnetworkID: subnetworks.SubnetworkIDRegistry,
 			Gas:          0,
@@ -127,7 +137,7 @@ func TestValidateTransactionInContextAndPopulateMassAndFee(t *testing.T) {
 
 		txWithImmatureCoinbase := externalapi.DomainTransaction{
 			Version:      constants.MaxTransactionVersion,
-			Inputs:       []*externalapi.DomainTransactionInput{&txInput},
+			Inputs:       []*externalapi.DomainTransactionInput{&immatureInput},
 			Outputs:      []*externalapi.DomainTransactionOutput{&txOut},
 			SubnetworkID: subnetworks.SubnetworkIDRegistry,
 			Gas:          0,
@@ -158,7 +168,15 @@ func TestValidateTransactionInContextAndPopulateMassAndFee(t *testing.T) {
 
 		povBlockHash := externalapi.NewDomainHashFromByteArray(&[32]byte{0x01})
 		tc.DAABlocksStore().StageDAAScore(stagingArea, povBlockHash, consensusConfig.BlockCoinbaseMaturity+txInput.UTXOEntry.BlockDAAScore())
-		tc.DAABlocksStore().StageDAAScore(stagingArea, povBlockHash, 10)
+
+		// Just use some stub ghostdag data
+		tc.GHOSTDAGDataStore().Stage(stagingArea, povBlockHash, model.NewBlockGHOSTDAGData(
+			0,
+			nil,
+			consensusConfig.GenesisHash,
+			nil,
+			nil,
+			nil))
 
 		tests := []struct {
 			name                     string
@@ -171,7 +189,7 @@ func TestValidateTransactionInContextAndPopulateMassAndFee(t *testing.T) {
 			{
 				name:                     "Valid transaction",
 				tx:                       &validTx,
-				povBlockHash:             model.VirtualBlockHash,
+				povBlockHash:             povBlockHash,
 				selectedParentMedianTime: 1,
 				isValid:                  true,
 				expectedError:            nil,
@@ -189,7 +207,7 @@ func TestValidateTransactionInContextAndPopulateMassAndFee(t *testing.T) {
 			{ // The total inputs amount is bigger than the allowed maximum (constants.MaxSompi)
 				name:                     "checkTransactionInputAmounts",
 				tx:                       &txWithLargeAmount,
-				povBlockHash:             model.VirtualBlockHash,
+				povBlockHash:             povBlockHash,
 				selectedParentMedianTime: 1,
 				isValid:                  false,
 				expectedError:            ruleerrors.ErrBadTxOutValue,
@@ -197,7 +215,7 @@ func TestValidateTransactionInContextAndPopulateMassAndFee(t *testing.T) {
 			{ // The total SompiIn (sum of inputs amount) is smaller than the total SompiOut (sum of outputs value) and hence invalid.
 				name:                     "checkTransactionOutputAmounts",
 				tx:                       &txWithBigValue,
-				povBlockHash:             model.VirtualBlockHash,
+				povBlockHash:             povBlockHash,
 				selectedParentMedianTime: 1,
 				isValid:                  false,
 				expectedError:            ruleerrors.ErrSpendTooHigh,
@@ -205,15 +223,15 @@ func TestValidateTransactionInContextAndPopulateMassAndFee(t *testing.T) {
 			{ // the selectedParentMedianTime is negative and hence invalid.
 				name:                     "checkTransactionSequenceLock",
 				tx:                       &validTx,
-				povBlockHash:             model.VirtualBlockHash,
+				povBlockHash:             povBlockHash,
 				selectedParentMedianTime: -1,
 				isValid:                  false,
 				expectedError:            ruleerrors.ErrUnfinalizedTx,
 			},
-			{ // The SignatureScript (in the txInput) is empty and hence invalid.
+			{ // The SignatureScript (in the immatureInput) is empty and hence invalid.
 				name:                     "checkTransactionScripts",
 				tx:                       &txWithInvalidSignature,
-				povBlockHash:             model.VirtualBlockHash,
+				povBlockHash:             povBlockHash,
 				selectedParentMedianTime: 1,
 				isValid:                  false,
 				expectedError:            ruleerrors.ErrScriptValidation,
@@ -226,7 +244,7 @@ func TestValidateTransactionInContextAndPopulateMassAndFee(t *testing.T) {
 			if test.isValid {
 				if err != nil {
 					t.Fatalf("Unexpected error on TestValidateTransactionInContextAndPopulateMassAndFee"+
-						" on test '%v': %v", test.name, err)
+						" on test '%v': %+v", test.name, err)
 				}
 			} else {
 				if err == nil || !errors.Is(err, test.expectedError) {
diff --git a/domain/miningmanager/miningmanager_test.go b/domain/miningmanager/miningmanager_test.go
index 70359aa0..4cde5d3d 100644
--- a/domain/miningmanager/miningmanager_test.go
+++ b/domain/miningmanager/miningmanager_test.go
@@ -1,6 +1,7 @@
 package miningmanager_test
 
 import (
+	"github.com/kaspanet/kaspad/domain/miningmanager/mempool"
 	"strings"
 	"testing"
 
@@ -70,11 +71,35 @@ func TestValidateAndInsertTransaction(t *testing.T) {
 	})
 }
 
+func TestImmatureSpend(t *testing.T) {
+	testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) {
+		factory := consensus.NewFactory()
+		tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestValidateAndInsertTransaction")
+		if err != nil {
+			t.Fatalf("Error setting up TestConsensus: %+v", err)
+		}
+		defer teardown(false)
+
+		miningFactory := miningmanager.NewFactory()
+		miningManager := miningFactory.NewMiningManager(tc, &consensusConfig.Params)
+		tx := createTransactionWithUTXOEntry(t, 0)
+		err = miningManager.ValidateAndInsertTransaction(tx, false)
+		txRuleError := &mempool.TxRuleError{}
+		if !errors.As(err, txRuleError) || txRuleError.RejectCode != mempool.RejectImmatureSpend {
+			t.Fatalf("Unexpected error %+v", err)
+		}
+		transactionsFromMempool := miningManager.AllTransactions()
+		if contains(tx, transactionsFromMempool) {
+			t.Fatalf("Mempool contains a transaction with immature coinbase")
+		}
+	})
+}
+
 // TestInsertDoubleTransactionsToMempool verifies that an attempt to insert a transaction
 // more than once into the mempool will result in raising an appropriate error.
 func TestInsertDoubleTransactionsToMempool(t *testing.T) {
 	testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) {
-
+		consensusConfig.BlockCoinbaseMaturity = 0
 		factory := consensus.NewFactory()
 		tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestInsertDoubleTransactionsToMempool")
 		if err != nil {
@@ -99,7 +124,7 @@ func TestInsertDoubleTransactionsToMempool(t *testing.T) {
 // TestHandleNewBlockTransactions verifies that all the transactions in the block were successfully removed from the mempool.
 func TestHandleNewBlockTransactions(t *testing.T) {
 	testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) {
-
+		consensusConfig.BlockCoinbaseMaturity = 0
 		factory := consensus.NewFactory()
 		tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestHandleNewBlockTransactions")
 		if err != nil {
@@ -165,7 +190,7 @@ func domainBlocksToBlockIds(blocks []*externalapi.DomainTransaction) []*external
 // will be removed from the mempool.
 func TestDoubleSpends(t *testing.T) {
 	testutils.ForAllNets(t, true, func(t *testing.T, consensusConfig *consensus.Config) {
-
+		consensusConfig.BlockCoinbaseMaturity = 0
 		factory := consensus.NewFactory()
 		tc, teardown, err := factory.NewTestConsensus(consensusConfig, "TestDoubleSpends")
 		if err != nil {
@@ -309,7 +334,7 @@ func createTransactionWithUTXOEntry(t *testing.T, i int) *externalapi.DomainTran
 			100000000, // 1 KAS
 			scriptPublicKey,
 			true,
-			uint64(5)),
+			uint64(0)),
 	}
 	txOut := externalapi.DomainTransactionOutput{
 		Value:           10000,