Implemented rpc server method GetRawTransaction #135

dauTT · 2019-02-10T20:53:33Z

Problem

Implemented #129

...

Solution

Added utility function GetVarSize. (reference)
Implemented serializable interface
Added fee calculation
Finally Implemented getrawtransaction rcp call.

To make sure that things are correct, I have added tests and checked the result against neo-python and neo-cli.

...

…ializable interface. 2) Added few arithmetic operation (Add, Sub, div): this will be used to calculated networkfeeand feePerByte. Changed return value of the Value() method to int instead of int64. Modified fixed8_test accordingly.

- Structs accepting the Size method implement the serializable interface. - Structs accepting the MarshalJSON method implements the customized json marshaller interface.

im-kulikov · 2019-02-11T12:56:53Z

config/config.go

+		EnrollmentTransaction int `yaml:"EnrollmentTransaction"`
+		IssueTransaction      int `yaml:"IssueTransaction"`
+		PublishTransaction    int `yaml:"PublishTransaction"`
+		RegisterTransaction   int `yaml:"RegisterTransaction"`


it must be int64

or.. mb float64

in the C# implementation is int64

public Fixed8(long data) { this.value = data; }

Why do you suggest float64?

Shall I change everything so that the NewFixed8 method accept int64?

// NewFixed8 return a new Fixed8 type multiplied by decimals. func NewFixed8(val int64) Fixed8 { return Fixed8(decimals * val) }

@dauTT yep, long / int64 for new.. but...

public Fixed8(long data) { this.value = data; }

and

public static Fixed8 FromDecimal(decimal value) { value *= D; if (value < long.MinValue || value > long.MaxValue) throw new OverflowException(); return new Fixed8 { value = (long)value }; }

private const long D = 100_000_000;

okay, I think I understood what you mean.

im-kulikov · 2019-02-11T12:57:43Z

config/config.go

+	case transaction.RegisterType:
+		return util.NewFixed8(s.RegisterTransaction)
+	default:
+		return util.NewFixed8(0)


we must fix NewFixed8 method:

// NewFixed8 return a new Fixed8 type multiplied by decimals. func NewFixed8(val int64) Fixed8 { return Fixed8(decimals * val) }

or mb to:

// NewFixed8 return a new Fixed8 type multiplied by decimals. func NewFixed8(val float64) Fixed8 { return Fixed8(int64(decimals * val)) }

im-kulikov · 2019-02-11T12:58:37Z

pkg/core/blockchain.go

@@ -25,6 +25,8 @@ var (
 	genAmount         = []int{8, 7, 6, 5, 4, 3, 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}
 	decrementInterval = 2000000
 	persistInterval   = 1 * time.Second
+
+	BlockchainDefault *Blockchain


it can't be exported

What do you mean it can't be exported? I Do need to register a blockchain as a default one in order to use it in the References method in the fee.go file.

func References(t *transaction.Transaction) map[util.Uint256]*transaction.Output { references := make(map[util.Uint256]*transaction.Output) for prevHash, inputs := range t.GroupInputsByPrevHash() { if BlockchainDefault == nil { panic("no default blockchain available! please register one.") } else if tx, _, err := BlockchainDefault.GetTransaction(prevHash); err != nil { tx = nil } else if tx != nil { for _, in := range inputs { references[in.PrevHash] = tx.Outputs[in.PrevIndex] } } else { references = nil } } return references }

At least within the core package, I do not have any problem to use it BlockchainDefault.

data race in runtime...

it's a bad practice, to set globalVariable..

https://dave.cheney.net/2017/06/11/go-without-package-scoped-variables

thanks for the link!

im-kulikov · 2019-02-12T19:24:14Z

Package unsafe contains operations that step around the type safety of Go programs.
Packages that import unsafe may be non-portable and are not protected by the Go 1 compatibility guidelines.

https://golang.org/pkg/unsafe/

var AttrUsage uint8

it's 1 byte

can be simplified to...

// Size returns the size in number bytes of the Attribute
func (attr *Attribute) Size() int {
	switch attr.Usage {
	case ContractHash, ECDH02, ECDH03, Vote,
		Hash1, Hash2, Hash3, Hash4, Hash5, Hash6, Hash7, Hash8, Hash9, Hash10, Hash11, Hash12, Hash13, Hash14, Hash15:
		return 33 // uint8 + 32
	case Script:
		return 21 // uint8 + 20
	case Description:
		return 2 + len(attr.Data) // uint8 + uint8 + len of data
	default:
		return 1 + len(attr.Data) // uint8 + len of data
	}
}

// GetVarIntSize returns the size in number of bytes of a variable integer
// (reference: GetVarSize(int value),  https://github.com/neo-project/neo/blob/master/neo/IO/Helper.cs)
func GetVarIntSize(value int) int {
	var size uintptr

	if value < 0xFD {
		size = 1 // uint8
	} else if value <= 0xFFFF {
		size = 3 // uint8 + uint16
	} else {
		size = 5 // uint8 + uint32
	}
	return int(size)
}

// GetVarSize return the size om bytes of a variable. This implementation is not exactly like the C#
// (reference: GetVarSize<T>(this T[] value), https://github.com/neo-project/neo/blob/master/neo/IO/Helper.cs#L53) as in the C# variable
// like Uint160, Uint256 are not supported. @TODO: make sure to have full unit tests coverage.
func GetVarSize(value interface{}) int {
	v := reflect.ValueOf(value)
	switch v.Kind() {
	case reflect.String:
		return GetVarStringSize(v.String())
	case reflect.Int:
		return GetVarIntSize(int(v.Int()))
	case reflect.Slice, reflect.Array:
		valueLength := v.Len()
		valueSize := 0

		switch t := value.(type) {
		case io.Serializable:
			for i := 0; i < valueLength; i++ {
				valueSize += t.Size()
			}
		case []uint8, []int8,
			Uint160, Uint256,
			[20]uint8, [32]uint8,
			[20]int8, [32]int8:
			valueSize = valueLength
		case []uint16, []int16,
			[10]uint16, [10]int16:
			valueSize = valueLength * 2
		case []uint32, []int32,
			[30]uint32, [30]int32:
			valueSize = valueLength * 4
		case []uint64, []int64,
			[30]uint64, [30]int64:
			valueSize = valueLength * 8
		}
		return GetVarIntSize(valueLength) + valueSize
	default:
		panic(fmt.Sprintf("unable to calculate GetVarSize, %s", reflect.TypeOf(value)))
	}
}

// Size returns the size of the transaction in term of bytes
func (t *Transaction) Size() int {
	attrSize := util.GetVarSize(t.Attributes)
	inputSize := util.GetVarSize(t.Inputs)
	outputSize := util.GetVarSize(t.Outputs)
	witnesSize := util.GetVarSize(t.Scripts)
	// uint8 + uint8 + + attrSize + inputSize + outputSize + witnesSize
	return 2 + attrSize + inputSize + outputSize + witnesSize
}

// Size returns the size in bytes of the Input
func (in *Input) Size() int { return in.PrevHash.Size() + 2 } // uint16

// Size returns the lenght of the bytes representation of Uint256
func (u Uint256) Size() int { return uint256Size }

// Size returns the lenght of the bytes representation of Uint160
func (u Uint160) Size() int { return uint160Size }

im-kulikov · 2019-02-12T20:11:07Z

or mb:

// GetVarSize return the size om bytes of a variable. This implementation is not exactly like the C#
// (reference: GetVarSize<T>(this T[] value), https://github.com/neo-project/neo/blob/master/neo/IO/Helper.cs#L53) as in the C# variable
// like Uint160, Uint256 are not supported. @TODO: make sure to have full unit tests coverage.
func GetVarSize(value interface{}) int {
	v := reflect.ValueOf(value)
	switch v.Kind() {
	case reflect.String:
		return GetVarStringSize(v.String())
	case reflect.Int,
		reflect.Int8,
		reflect.Int16,
		reflect.Int32,
		reflect.Int64:
		return GetVarIntSize(int(v.Int()))
	case reflect.Uint,
		reflect.Uint8,
		reflect.Uint16,
		reflect.Uint32,
		reflect.Uint64:
		return GetVarIntSize(int(v.Uint()))
	case reflect.Slice, reflect.Array:
		valueLength := v.Len()
		valueSize := 0

		switch t := value.(type) {
		case io.Serializable:
			for i := 0; i < valueLength; i++ {
				valueSize += t.Size()
			}
		case []uint8, []int8,
			Uint160, Uint256,
			[20]uint8, [32]uint8,
			[20]int8, [32]int8:
			valueSize = valueLength
		case []uint16, []int16,
			[10]uint16, [10]int16:
			valueSize = valueLength * 2
		case []uint32, []int32,
			[30]uint32, [30]int32:
			valueSize = valueLength * 4
		case []uint64, []int64,
			[30]uint64, [30]int64:
			valueSize = valueLength * 8
		}
		return GetVarIntSize(valueLength) + valueSize
	default:
		panic(fmt.Sprintf("unable to calculate GetVarSize, %s", reflect.TypeOf(value)))
	}
}

im-kulikov

remove unsafe from code..

dauTT · 2019-02-12T21:23:12Z

I agree that your suggestion above to change the GeVarSize is good. However the test TestSize in the fee_test.go file is failing. The reason is that case

case io.Serializable:
			for i := 0; i < valueLength; i++ {
				valueSize += t.Size()
			}

is not working properly because type is not a Struct but a Inteface. I still need to do a comprimise in this way:

func GetVarSize(value interface{}) int {
	v := reflect.ValueOf(value)
	switch v.Kind() {
	case reflect.String:
		return GetVarStringSize(v.String())
	case reflect.Int,
		reflect.Int8,
		reflect.Int16,
		reflect.Int32,
		reflect.Int64:
		return GetVarIntSize(int(v.Int()))
	case reflect.Uint,
		reflect.Uint8,
		reflect.Uint16,
		reflect.Uint32,
		reflect.Uint64:
		return GetVarIntSize(int(v.Uint()))
	case reflect.Slice, reflect.Array:
		valueLength := v.Len()
		valueSize := 0

		// case v is a slice / Array of io.Serializable
		typeArray := reflect.TypeOf(value).Elem()
		SerializableType := reflect.TypeOf((*io.Serializable)(nil)).Elem()
		if typeArray.Implements(SerializableType) {
			for i := 0; i < valueLength; i++ {
				elem := v.Index(i).Interface().(io.Serializable)
				valueSize += elem.Size()
			}
		}
		switch value.(type) {
		case []uint8, []int8,
			Uint160, Uint256,
			[20]uint8, [32]uint8,
			[20]int8, [32]int8:
			fmt.Println("t []uint8")
			valueSize = valueLength
		case []uint16, []int16,
			[10]uint16, [10]int16:
			fmt.Println("t []uint16")
			valueSize = valueLength * 2
		case []uint32, []int32,
			[30]uint32, [30]int32:
			fmt.Println("t []uint32")
			valueSize = valueLength * 4
		case []uint64, []int64,
			[30]uint64, [30]int64:
			fmt.Println("t", "[]uint64")
			valueSize = valueLength * 8
		}

		return GetVarIntSize(valueLength) + valueSize
	default:
		panic(fmt.Sprintf("unable to calculate GetVarSize, %s", reflect.TypeOf(value)))
	}
}

Are you okay with this?

im-kulikov · 2019-02-12T21:26:08Z

Let me see what the reason is at the morning.

im-kulikov · 2019-02-12T21:27:56Z

You can try []io.Serizable, if you're doing it now

1) NewFixed8 will accept as input int64 2) race condition affecting configDeafault, blockchainDefault

dauTT · 2019-02-12T21:53:13Z

bad luck, still not working..

case []io.Serializable:
			for i := 0; i < valueLength; i++ {
				valueSize += t[i].Size()
			}

im-kulikov · 2019-02-13T08:57:21Z

@dauTT

// GetVarSize return the size om bytes of a variable. This implementation is not exactly like the C#
// (reference: GetVarSize<T>(this T[] value), https://github.com/neo-project/neo/blob/master/neo/IO/Helper.cs#L53) as in the C# variable
// like Uint160, Uint256 are not supported. @TODO: make sure to have full unit tests coverage.
func GetVarSize(value interface{}) int {
	v := reflect.ValueOf(value)
	switch v.Kind() {
	case reflect.String:
		return GetVarStringSize(v.String())
	case reflect.Int,
		reflect.Int8,
		reflect.Int16,
		reflect.Int32,
		reflect.Int64:
		return GetVarIntSize(int(v.Int()))
	case reflect.Uint,
		reflect.Uint8,
		reflect.Uint16,
		reflect.Uint32,
		reflect.Uint64:
		return GetVarIntSize(int(v.Uint()))
	case reflect.Slice, reflect.Array:
		valueLength := v.Len()
		valueSize := 0

		switch reflect.ValueOf(value).Index(0).Interface().(type) {
		case io.Serializable:
			for i := 0; i < valueLength; i++ {
				elem := v.Index(i).Interface().(io.Serializable)
				valueSize += elem.Size()
			}
		case uint8, int8:
			valueSize = valueLength
		case uint16, int16:
			valueSize = valueLength * 2
		case uint32, int32:
			valueSize = valueLength * 4
		case uint64, int64:
			valueSize = valueLength * 8
		}

		return GetVarIntSize(valueLength) + valueSize
	default:
		panic(fmt.Sprintf("unable to calculate GetVarSize, %s", reflect.TypeOf(value)))
	}
}

im-kulikov · 2019-02-13T09:15:48Z

config/config.go

@@ -27,6 +30,10 @@ var (
 	// BuildTime the time and date the current version of the node built,
 	// set at build time.
 	BuildTime string
+
+	// ConfigDefault is the Config which is defined in the Load method.
+	configDefault *Config


don't use global variables

im-kulikov · 2019-02-13T09:16:17Z

pkg/core/blockchain.go

@@ -25,6 +26,10 @@ var (
 	genAmount         = []int{8, 7, 6, 5, 4, 3, 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1}
 	decrementInterval = 2000000
 	persistInterval   = 1 * time.Second
+
+	// BlockchainDefault is the Blockchain which is defined in the NewBlockchain method.
+	blockchainDefault *Blockchain


don't use global variables

2) Extended Blockchainer interface to include the methods: References, FeePerByte, SystemFee, NetworkFee 3) Deleted fee_test.go, fee.go. Moved corresponding methods to blockchain_test.go and blockchain.go respectively 4) Amended tx_raw_output.go

im-kulikov · 2019-02-14T10:38:56Z

pkg/rpc/server.go

+	case "getrawtransaction":
+		var err error
+
+		param0, exists := reqParams.ValueAtAndType(0, "string")


can you replace
param0, exists := reqParams.ValueAtAndType(0, "string")
to
param0, exists := reqParams.ValueWithType(0, "string")

im-kulikov

I propose to gradually abandon ValueAtAndType and Value in favor of a more correct implementation of Value / ValueWithType

im-kulikov · 2019-02-14T12:08:21Z

@dauTT if you do not mind, I will look at the weekend and fully review this PR

dauTT · 2019-02-18T06:41:34Z

okay, I will change and use require

im-kulikov · 2019-02-18T06:55:36Z

@dauTT thanks a lot. You did a great job.

im-kulikov

Thanks, you’re amazing 🚀🎉

dauTT · 2019-02-18T09:22:16Z

I shall say, YOU are an amazing reviewer. 😄

im-kulikov · 2019-02-19T11:50:31Z

@dauTT can you fix conflicts?

im-kulikov · 2019-02-19T14:01:37Z

hm..

dauTT · 2019-02-19T14:17:13Z

tests are failing. Will fix it.

dauTT · 2019-02-19T14:29:37Z

The code is clean now.

im-kulikov

SGTM, any thoughts @decentralisedkev / @fabwa

dauTT · 2019-02-20T17:12:24Z

After the merge the tests are failing. Please do not merge yet.

dauTT · 2019-02-20T17:24:16Z

The code is clean now. All tests pass

im-kulikov

@decentralisedkev @fabwa can we merge?

im-kulikov · 2019-02-20T17:40:46Z

🙏

hal0x2328 · 2019-02-26T14:44:32Z

Award dauTT 400 points
Award im-kulikov 150 points for review

lllwvlvwlll · 2019-03-10T16:44:08Z

https://neoscan.io/transaction/144dc06fbc54dd84165af08ddd9c01e8a720ca1bb05471dfd1af1503160f245f

dauTT added 6 commits February 10, 2019 19:33

Added utility function GetVarSize

41e27d7

Implemented Size or MarshalJSON method.

a837151

- Structs accepting the Size method implement the serializable interface. - Structs accepting the MarshalJSON method implements the customized json marshaller interface.

Added fee calculation

c280890

Implemented rcp server method GetRawTransaction

30d0a4a

Updated Tests

6540cf8

im-kulikov reviewed Feb 11, 2019

View reviewed changes

im-kulikov suggested changes Feb 12, 2019

View reviewed changes

dauTT added 2 commits February 12, 2019 22:43

Fixed:

ba3393e

1) NewFixed8 will accept as input int64 2) race condition affecting configDeafault, blockchainDefault

Simplified Size calculation

36b69e4

im-kulikov reviewed Feb 13, 2019

View reviewed changes

dauTT and others added 3 commits February 13, 2019 18:40

Simplified GetVarSize Method

42590dc

Merge branch 'master' into dauTT/patch-5

38528c5

im-kulikov reviewed Feb 14, 2019

View reviewed changes

dauTT added 3 commits February 14, 2019 12:49

Replaced ValueAtAndType with ValueWithType

9b91af1

Cosmetic changes + Added test case getrawtransaction_7

04a25fe

Clean up Print statement

5bff6c4

Replaced assert.NoError with require.NoError

da3f83f

im-kulikov approved these changes Feb 18, 2019

View reviewed changes

Merge branch 'master' into dauTT/patch-5

a9d07e8

Merge branch 'master' into dauTT/patch-5

a8328ff

Fixed tests by adding context.Background() as argument

18b6b35

im-kulikov approved these changes Feb 20, 2019

View reviewed changes

Merge branch 'master' into dauTT/patch-5

b576289

Fixed tests

61b8e96

im-kulikov approved these changes Feb 20, 2019

View reviewed changes

fabwa merged commit 19201dc into nspcc-dev:master Feb 20, 2019

dauTT deleted the dauTT/patch-5 branch February 21, 2019 07:23

dauTT mentioned this pull request Feb 22, 2019

implement rcp call: getrawtransaction (server side) #129

Closed

hal0x2328 added the I1 High impact label Feb 26, 2019

roman-khimov mentioned this pull request Dec 28, 2019

Implement getRawTransaction #584

Closed

Implemented rpc server method GetRawTransaction #135

Implemented rpc server method GetRawTransaction #135

Conversation

dauTT commented Feb 10, 2019

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-kulikov commented Feb 12, 2019 • edited Loading

im-kulikov commented Feb 12, 2019

im-kulikov left a comment

Choose a reason for hiding this comment

dauTT commented Feb 12, 2019 • edited Loading

im-kulikov commented Feb 12, 2019

im-kulikov commented Feb 12, 2019

dauTT commented Feb 12, 2019

im-kulikov commented Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

im-kulikov left a comment

Choose a reason for hiding this comment

im-kulikov commented Feb 14, 2019

dauTT commented Feb 18, 2019

im-kulikov commented Feb 18, 2019 • edited Loading

im-kulikov left a comment

Choose a reason for hiding this comment

dauTT commented Feb 18, 2019

im-kulikov commented Feb 19, 2019

im-kulikov commented Feb 19, 2019

dauTT commented Feb 19, 2019

dauTT commented Feb 19, 2019

im-kulikov left a comment

Choose a reason for hiding this comment

dauTT commented Feb 20, 2019

dauTT commented Feb 20, 2019 • edited Loading

im-kulikov left a comment

Choose a reason for hiding this comment

im-kulikov commented Feb 20, 2019

hal0x2328 commented Feb 26, 2019

lllwvlvwlll commented Mar 10, 2019

im-kulikov commented Feb 12, 2019 •

edited

Loading

dauTT commented Feb 12, 2019 •

edited

Loading

im-kulikov commented Feb 13, 2019 •

edited

Loading

im-kulikov commented Feb 18, 2019 •

edited

Loading

dauTT commented Feb 20, 2019 •

edited

Loading