From 6b55950901c5c3ef19226508551f991c7a4c7bcb Mon Sep 17 00:00:00 2001 From: Svarog Date: Sun, 13 Jan 2019 11:01:43 +0200 Subject: [PATCH] [DEV-345] Enforce requirement for transactions from 1(DAGCoin) or 2(sub network registry) sub network to have gas = 0 (#157) * [DEV-345] Validate that gas and payload are 0 when required by sub-network * [DEV-345] Remove check for txOut.Value < 0, since txOut.Value is a uint64 * [DEV-345] Added tests for CheckTransactionSanity * [DEV-345] Remove checks for Gas and Payload validity in wire.MsgTx.Decode * [DEV-345] Verify that payload in Gas sub-network is always 8 bytes (uint64). * [DEV-345] Renamed tstCheck{Script/Rule}Error to check{Script/Rule}Error * [DEV-345] Improved formatting --- blockdag/common_test.go | 34 +++++++++++++++++++++++++++ blockdag/error.go | 5 ++++ blockdag/error_test.go | 1 + blockdag/test_utils.go | 38 +++++++++++++++++++++++++++++- blockdag/validate.go | 29 +++++++++++++++++++---- blockdag/validate_test.go | 48 ++++++++++++++++++++++++++++++++++++++ txscript/engine_test.go | 6 ++--- txscript/script_test.go | 2 +- txscript/scriptnum_test.go | 2 +- txscript/stack_test.go | 6 ++--- txscript/standard_test.go | 10 ++++---- wire/msgtx.go | 12 ---------- wire/msgtx_test.go | 44 ---------------------------------- 13 files changed, 162 insertions(+), 75 deletions(-) diff --git a/blockdag/common_test.go b/blockdag/common_test.go index 3941d9b1e..0b06cc318 100644 --- a/blockdag/common_test.go +++ b/blockdag/common_test.go @@ -7,9 +7,11 @@ package blockdag import ( "compress/bzip2" "encoding/binary" + "fmt" "io" "os" "path/filepath" + "reflect" "strings" "time" @@ -220,3 +222,35 @@ func buildNodeGenerator(phantomK uint32, withChildren bool) func(parents blockSe } return buildNode } + +// checkRuleError ensures the type of the two passed errors are of the +// same type (either both nil or both of type RuleError) and their error codes +// match when not nil. +func checkRuleError(gotErr, wantErr error) error { + // Ensure the error code is of the expected type and the error + // code matches the value specified in the test instance. + if reflect.TypeOf(gotErr) != reflect.TypeOf(wantErr) { + return fmt.Errorf("wrong error - got %T (%[1]v), want %T", + gotErr, wantErr) + } + if gotErr == nil { + return nil + } + + // Ensure the want error type is a script error. + werr, ok := wantErr.(RuleError) + if !ok { + return fmt.Errorf("unexpected test error type %T", wantErr) + } + + // Ensure the error codes match. It's safe to use a raw type assert + // here since the code above already proved they are the same type and + // the want error is a script error. + gotErrorCode := gotErr.(RuleError).ErrorCode + if gotErrorCode != werr.ErrorCode { + return fmt.Errorf("mismatched error code - got %v (%v), want %v", + gotErrorCode, gotErr, werr.ErrorCode) + } + + return nil +} diff --git a/blockdag/error.go b/blockdag/error.go index 0c325a257..24723575b 100644 --- a/blockdag/error.go +++ b/blockdag/error.go @@ -220,6 +220,10 @@ const ( // by subnetwork ErrInvalidGas + // ErrInvalidPayload transaction includes a payload in a sub-network that doesn't allow + // a Payload + ErrInvalidPayload + // ErrSubNetwork indicates that a block doesn't adhere to the sub-network // registry rules ErrSubNetworkRegistry @@ -270,6 +274,7 @@ var errorCodeStrings = map[ErrorCode]string{ ErrFinality: "ErrFinality", ErrTransactionsNotSorted: "ErrTransactionsNotSorted", ErrInvalidGas: "ErrInvalidGas", + ErrInvalidPayload: "ErrInvalidPayload", } // String returns the ErrorCode as a human-readable name. diff --git a/blockdag/error_test.go b/blockdag/error_test.go index 218a86a96..e997e4a3d 100644 --- a/blockdag/error_test.go +++ b/blockdag/error_test.go @@ -58,6 +58,7 @@ func TestErrorCodeStringer(t *testing.T) { {ErrFinality, "ErrFinality"}, {ErrTransactionsNotSorted, "ErrTransactionsNotSorted"}, {ErrInvalidGas, "ErrInvalidGas"}, + {ErrInvalidPayload, "ErrInvalidPayload"}, {0xffff, "Unknown ErrorCode (65535)"}, } diff --git a/blockdag/test_utils.go b/blockdag/test_utils.go index a1b4a783a..f4957176c 100644 --- a/blockdag/test_utils.go +++ b/blockdag/test_utils.go @@ -3,11 +3,12 @@ package blockdag import ( "encoding/binary" "fmt" - "github.com/daglabs/btcd/util/subnetworkid" "os" "path/filepath" "time" + "github.com/daglabs/btcd/util/subnetworkid" + "github.com/daglabs/btcd/dagconfig" "github.com/daglabs/btcd/dagconfig/daghash" "github.com/daglabs/btcd/database" @@ -104,6 +105,41 @@ func DAGSetup(dbName string, config Config) (*BlockDAG, func(), error) { // OpTrueScript is script returning TRUE var OpTrueScript = []byte{txscript.OpTrue} +type txSubnetworkData struct { + subnetworkID subnetworkid.SubNetworkID + Gas uint64 + Payload []byte +} + +func createTxForTest(numInputs uint32, numOutputs uint32, outputValue uint64, subnetworkData *txSubnetworkData) *wire.MsgTx { + tx := wire.NewMsgTx(wire.TxVersion) + + for i := uint32(0); i < numInputs; i++ { + tx.AddTxIn(&wire.TxIn{ + PreviousOutPoint: *wire.NewOutPoint(&daghash.Hash{}, i), + SignatureScript: []byte{}, + Sequence: wire.MaxTxInSequenceNum, + }) + } + for i := uint32(0); i < numOutputs; i++ { + tx.AddTxOut(&wire.TxOut{ + PkScript: OpTrueScript, + Value: outputValue, + }) + } + + if subnetworkData != nil { + tx.SubNetworkID = subnetworkData.subnetworkID + tx.Gas = subnetworkData.Gas + tx.Payload = subnetworkData.Payload + } else { + tx.SubNetworkID = wire.SubNetworkDAGCoin + tx.Gas = 0 + tx.Payload = []byte{} + } + return tx +} + // createCoinbaseTxForTest returns a coinbase transaction with the requested number of // outputs paying an appropriate subsidy based on the passed block height to the // address associated with the harness. It automatically uses a standard diff --git a/blockdag/validate.go b/blockdag/validate.go index a0fe2b292..836512561 100644 --- a/blockdag/validate.go +++ b/blockdag/validate.go @@ -204,11 +204,6 @@ func CheckTransactionSanity(tx *util.Tx) error { var totalSatoshi uint64 for _, txOut := range msgTx.TxOut { satoshi := txOut.Value - if satoshi < 0 { - str := fmt.Sprintf("transaction output has negative "+ - "value of %v", satoshi) - return ruleError(ErrBadTxOutValue, str) - } if satoshi > util.MaxSatoshi { str := fmt.Sprintf("transaction output value of %v is "+ "higher than max allowed value of %v", satoshi, @@ -267,6 +262,30 @@ func CheckTransactionSanity(tx *util.Tx) error { } } + // Transactions in native and SubNetworkRegistry SubNetworks must have Gas = 0 + if (msgTx.SubNetworkID == wire.SubNetworkDAGCoin || + msgTx.SubNetworkID == wire.SubNetworkRegistry) && + msgTx.Gas > 0 { + + return ruleError(ErrInvalidGas, "transaction in the native or "+ + "registry sub-networks has gas > 0 ") + } + + if msgTx.SubNetworkID == wire.SubNetworkDAGCoin && + len(msgTx.Payload) > 0 { + + return ruleError(ErrInvalidPayload, + "transaction in the native sub-network includes a payload") + } + + if msgTx.SubNetworkID == wire.SubNetworkRegistry && + len(msgTx.Payload) != 8 { + + return ruleError(ErrInvalidPayload, + "transaction in the sub-network registry include a payload "+ + "with length != 8 bytes") + } + return nil } diff --git a/blockdag/validate_test.go b/blockdag/validate_test.go index e1edecdf2..4156519a8 100644 --- a/blockdag/validate_test.go +++ b/blockdag/validate_test.go @@ -1145,3 +1145,51 @@ var BlockWithWrongTxOrder = wire.MsgBlock{ }, }, } + +func TestCheckTransactionSanity(t *testing.T) { + tests := []struct { + name string + numInputs uint32 + numOutputs uint32 + outputValue uint64 + subnetworkData *txSubnetworkData + extraModificationsFunc func(*wire.MsgTx) + expectedErr error + }{ + {"good one", 1, 1, 1, nil, nil, nil}, + {"no inputs", 0, 1, 1, nil, nil, ruleError(ErrNoTxInputs, "")}, + {"no outputs", 1, 0, 1, nil, nil, ruleError(ErrNoTxOutputs, "")}, + {"too big", 100000, 1, 1, nil, nil, ruleError(ErrTxTooBig, "")}, + {"too much satoshi in one output", 1, 1, util.MaxSatoshi + 1, nil, nil, ruleError(ErrBadTxOutValue, "")}, + {"too much satoshi in total outputs", 1, 2, util.MaxSatoshi - 1, nil, nil, ruleError(ErrBadTxOutValue, "")}, + {"duplicate inputs", 2, 1, 1, nil, + func(tx *wire.MsgTx) { tx.TxIn[1].PreviousOutPoint.Index = 0 }, + ruleError(ErrDuplicateTxInputs, "")}, + {"non-zero gas in DAGCoin", 1, 1, 0, + &txSubnetworkData{wire.SubNetworkDAGCoin, 1, []byte{}}, + nil, ruleError(ErrInvalidGas, "")}, + {"non-zero gas in subnetwork registry", 1, 1, 0, + &txSubnetworkData{wire.SubNetworkRegistry, 1, []byte{}}, + nil, ruleError(ErrInvalidGas, "")}, + {"non-zero payload in DAGCoin", 1, 1, 0, + &txSubnetworkData{wire.SubNetworkDAGCoin, 0, []byte{1}}, + nil, ruleError(ErrInvalidPayload, "")}, + {"payload in subnetwork registry isn't 8 bytes", 1, 1, 0, + &txSubnetworkData{wire.SubNetworkRegistry, 0, []byte{1, 2, 3, 4, 5, 6, 7}}, + nil, ruleError(ErrInvalidPayload, "")}, + } + + for _, test := range tests { + tx := createTxForTest(test.numInputs, test.numOutputs, test.outputValue, test.subnetworkData) + + if test.extraModificationsFunc != nil { + test.extraModificationsFunc(tx) + } + + err := CheckTransactionSanity(util.NewTx(tx)) + if e := checkRuleError(err, test.expectedErr); e != nil { + t.Errorf("TestCheckTransactionSanity: '%s': %v", test.name, e) + continue + } + } +} diff --git a/txscript/engine_test.go b/txscript/engine_test.go index b423edd20..5e7d6ecd1 100644 --- a/txscript/engine_test.go +++ b/txscript/engine_test.go @@ -156,7 +156,7 @@ func TestCheckErrorCondition(t *testing.T) { } err = vm.CheckErrorCondition(test.finalScript) - if e := tstCheckScriptError(err, test.expectedErr); e != nil { + if e := checkScriptError(err, test.expectedErr); e != nil { t.Errorf("TestCheckErrorCondition: %d: %s", i, e) } }() @@ -443,7 +443,7 @@ func TestDisasmPC(t *testing.T) { for i, test := range tests { actual, err := vm.DisasmPC() - if e := tstCheckScriptError(err, test.expectedErr); e != nil { + if e := checkScriptError(err, test.expectedErr); e != nil { t.Errorf("TestDisasmPC: %d: %s", i, e) } @@ -505,7 +505,7 @@ func TestDisasmScript(t *testing.T) { for _, test := range tests { actual, err := vm.DisasmScript(test.index) - if e := tstCheckScriptError(err, test.expectedErr); e != nil { + if e := checkScriptError(err, test.expectedErr); e != nil { t.Errorf("TestDisasmScript: %d: %s", test.index, e) } diff --git a/txscript/script_test.go b/txscript/script_test.go index 2d6e58bfc..3f3c83983 100644 --- a/txscript/script_test.go +++ b/txscript/script_test.go @@ -3654,7 +3654,7 @@ func TestUnparsingInvalidOpcodes(t *testing.T) { for _, test := range tests { _, err := test.pop.bytes() - if e := tstCheckScriptError(err, test.expectedErr); e != nil { + if e := checkScriptError(err, test.expectedErr); e != nil { t.Errorf("Parsed opcode test '%s': %v", test.name, e) continue } diff --git a/txscript/scriptnum_test.go b/txscript/scriptnum_test.go index f2b9a9dcc..ec1708ed5 100644 --- a/txscript/scriptnum_test.go +++ b/txscript/scriptnum_test.go @@ -180,7 +180,7 @@ func TestMakeScriptNum(t *testing.T) { // code matches the value specified in the test instance. gotNum, err := makeScriptNum(test.serialized, test.numLen) - if e := tstCheckScriptError(err, test.err); e != nil { + if e := checkScriptError(err, test.err); e != nil { t.Errorf("makeScriptNum(%#x): %v", test.serialized, e) continue } diff --git a/txscript/stack_test.go b/txscript/stack_test.go index 432eda050..791bd976e 100644 --- a/txscript/stack_test.go +++ b/txscript/stack_test.go @@ -12,10 +12,10 @@ import ( "testing" ) -// tstCheckScriptError ensures the type of the two passed errors are of the +// checkScriptError ensures the type of the two passed errors are of the // same type (either both nil or both of type Error) and their error codes // match when not nil. -func tstCheckScriptError(gotErr, wantErr error) error { +func checkScriptError(gotErr, wantErr error) error { // Ensure the error code is of the expected type and the error // code matches the value specified in the test instance. if reflect.TypeOf(gotErr) != reflect.TypeOf(wantErr) { @@ -827,7 +827,7 @@ func TestStack(t *testing.T) { // Ensure the error code is of the expected type and the error // code matches the value specified in the test instance. - if e := tstCheckScriptError(err, test.err); e != nil { + if e := checkScriptError(err, test.err); e != nil { t.Errorf("%s: %v", test.name, e) continue } diff --git a/txscript/standard_test.go b/txscript/standard_test.go index 626d5c2b2..a4aea57fe 100644 --- a/txscript/standard_test.go +++ b/txscript/standard_test.go @@ -464,7 +464,7 @@ func TestCalcScriptInfo(t *testing.T) { pkScript := mustParseShortForm(test.pkScript) si, err := CalcScriptInfo(sigScript, pkScript, test.isP2SH) - if e := tstCheckScriptError(err, test.scriptInfoErr); e != nil { + if e := checkScriptError(err, test.scriptInfoErr); e != nil { t.Errorf("scriptinfo test %q: %v", test.name, e) continue } @@ -610,7 +610,7 @@ func TestPayToAddrScript(t *testing.T) { t.Logf("Running %d tests", len(tests)) for i, test := range tests { pkScript, err := PayToAddrScript(test.in) - if e := tstCheckScriptError(err, test.err); e != nil { + if e := checkScriptError(err, test.err); e != nil { t.Errorf("PayToAddrScript #%d unexpected error - "+ "got %v, want %v", i, err, test.err) continue @@ -716,7 +716,7 @@ func TestMultiSigScript(t *testing.T) { t.Logf("Running %d tests", len(tests)) for i, test := range tests { script, err := MultiSigScript(test.keys, test.nrequired) - if e := tstCheckScriptError(err, test.err); e != nil { + if e := checkScriptError(err, test.err); e != nil { t.Errorf("MultiSigScript #%d: %v", i, e) continue } @@ -768,7 +768,7 @@ func TestCalcMultiSigStats(t *testing.T) { for i, test := range tests { script := mustParseShortForm(test.script) _, _, err := CalcMultiSigStats(script) - if e := tstCheckScriptError(err, test.err); e != nil { + if e := checkScriptError(err, test.err); e != nil { t.Errorf("CalcMultiSigStats #%d (%s): %v", i, test.name, e) continue @@ -1077,7 +1077,7 @@ func TestNullDataScript(t *testing.T) { for i, test := range tests { script, err := NullDataScript(test.data) - if e := tstCheckScriptError(err, test.err); e != nil { + if e := checkScriptError(err, test.err); e != nil { t.Errorf("NullDataScript: #%d (%s): %v", i, test.name, e) continue diff --git a/wire/msgtx.go b/wire/msgtx.go index b5015146f..f0311967b 100644 --- a/wire/msgtx.go +++ b/wire/msgtx.go @@ -476,24 +476,12 @@ func (msg *MsgTx) BtcDecode(r io.Reader, pver uint32) error { return err } - isRegistrySubNetwork := msg.SubNetworkID == SubNetworkRegistry - - if isRegistrySubNetwork && msg.Gas != 0 { - str := fmt.Sprintf("Transactions from subnetwork %v should have 0 gas", msg.SubNetworkID) - return messageError("MsgTx.BtcDecode", str) - } - payloadLength, err := ReadVarInt(r, pver) if err != nil { returnScriptBuffers() return err } - if isRegistrySubNetwork && payloadLength != 8 { - str := fmt.Sprintf("For registry sub network the payload should always be uint64 (8 bytes length)") - return messageError("MsgTx.BtcDecode", str) - } - msg.Payload = make([]byte, payloadLength) _, err = io.ReadFull(r, msg.Payload) } diff --git a/wire/msgtx_test.go b/wire/msgtx_test.go index f51faf726..6e6b1907a 100644 --- a/wire/msgtx_test.go +++ b/wire/msgtx_test.go @@ -624,50 +624,6 @@ func TestTxSerializeErrors(t *testing.T) { if err == nil || err.Error() != expectedErr.Error() { t.Errorf("TestTxSerializeErrors: expected error %v but got %v", expectedErr, err) } - - registryWithGasTxEncoded := []byte{ - 0x01, 0x00, 0x00, 0x00, // Version - 0x00, // Varint for number of input transactions - 0x00, // Varint for number of output transactions - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Lock time - 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, // Sub Network ID - 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Gas - 0x08, // Payload length varint - 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Payload / Gas limit - } - - r = bytes.NewReader(registryWithGasTxEncoded) - err = tx.Deserialize(r) - - str = fmt.Sprintf("Transactions from subnetwork %v should have 0 gas", SubNetworkRegistry) - expectedErr = messageError("MsgTx.BtcDecode", str) - if err == nil || err.Error() != expectedErr.Error() { - t.Errorf("TestTxSerializeErrors: expected error %v but got %v", expectedErr, err) - } - - registryWithWrongPayloadTxEncoded := []byte{ - 0x01, 0x00, 0x00, 0x00, // Version - 0x00, // Varint for number of input transactions - 0x00, // Varint for number of output transactions - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Lock time - 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, // Sub Network ID - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Gas - 0x03, // Payload length varint - 0x01, 0x02, 0x03, // Payload / Gas limit - } - - r = bytes.NewReader(registryWithWrongPayloadTxEncoded) - err = tx.Deserialize(r) - - str = fmt.Sprintf("For registry sub network the payload should always be uint64 (8 bytes length)") - expectedErr = messageError("MsgTx.BtcDecode", str) - if err == nil || err.Error() != expectedErr.Error() { - t.Errorf("TestTxSerializeErrors: expected error %v but got %v", expectedErr, err) - } } // TestTxOverflowErrors performs tests to ensure deserializing transactions