Conversation
b84940d to
bf06cda
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4087 +/- ##
==========================================
- Coverage 83.51% 83.44% -0.07%
==========================================
Files 359 359
Lines 43910 44130 +220
==========================================
+ Hits 36670 36824 +154
- Misses 5437 5497 +60
- Partials 1803 1809 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bb928b3 to
43b4467
Compare
43b4467 to
995606b
Compare
174df2d to
0315022
Compare
0de3721 to
5196698
Compare
There was a problem hiding this comment.
@Turalchik, conflicts.
The first part of review, will submit more later.
|
Conflicts. |
|
|
||
| func OpcodeDynamic(base int64, op opcode.Opcode, args *vm.PriceArgs) int64 { | ||
| if dynamicCoefficients[op] == nil { | ||
| fmt.Println(op, args) |
| if dynamicCoefficients[op] == nil { | ||
| fmt.Println(op, args) | ||
| } | ||
| return base * max(1, dynamicCoefficients[op](args)/factor) |
There was a problem hiding this comment.
Shouldn't be like that. Calculate the overall price of all opcodes in the script and then divide the resulting sum by the factor.
| } | ||
|
|
||
| func AppendGas(args *vm.PriceArgs) int64 { | ||
| return dotInts(appendW, []int64{int64(args.Refs), int64(args.NClonedItems), 1}) |
There was a problem hiding this comment.
Additional slice allocation on every opcode doesn't bring us anything good. Remove dotInts, use direct multiplication.
| for range n { | ||
| key := v.estack.popNoRef() | ||
| val := v.estack.popNoRef().value | ||
| numComparisons += m.Len() |
There was a problem hiding this comment.
Is it m.Len() or the number of iterations over cycle? m.Len() may be different from the number of iterations in case if map has duplicated entries.
There was a problem hiding this comment.
I do not know how we can find out the number of comparisons if Add cannot return it #4202 (comment).
There was a problem hiding this comment.
It's just a question. OK, I think we're fine with m.Len() here.
| v.estack.PushItem(t.Value().([]stackitem.MapElement)[index].Value.Dup()) | ||
| m := t.Value().([]stackitem.MapElement) | ||
| item = m[i].Value.Dup() |
There was a problem hiding this comment.
No need to declare m, useless variable.
| r := v.refs | ||
| var ( | ||
| item = v.estack.Pop() | ||
| n int | ||
| ) |
There was a problem hiding this comment.
Move r under var declaration.
|
|
||
| func (v *VM) cpValues(src iter.Seq[stackitem.Item], n int, isReferenced bool) []stackitem.Item { | ||
| arr := make([]stackitem.Item, 0, n) | ||
| func (v *VM) cpValues(src iter.Seq[stackitem.Item], n int, isReferenced bool) ([]stackitem.Item, int) { |
There was a problem hiding this comment.
Doc for int return is needed.
| defer func(canGetPrice bool) { | ||
| if canGetPrice { | ||
| v.gasConsumed += v.getPrice(op, parameter, priceArgs) | ||
| } |
There was a problem hiding this comment.
Can't do this directly. We need to preserve the old behaviour for pre-Gorgon and introduce new behaviour starting from Gorgon.
| if scparser.IsSignatureContract(script) { | ||
| size += 67 + io.GetVarSize(script) | ||
| netFee += Opcode(base, opcode.PUSHDATA1, opcode.PUSHDATA1) + base*ECDSAVerifyPrice | ||
| netFee += Opcode(base, opcode.PUSHDATA1, opcode.PUSHDATA1)/factor + base*ECDSAVerifyPrice |
There was a problem hiding this comment.
Two problems here:
fee.Opcode() behaviour change
The behaviour of Opcode() shouldn't be changed because it's an exported API that may be used by some external users.
I see two possible solutions:
I
- Divide the result of
fee.Opcodebyfactorinternally, insidefee.Opcodeitself (it will returnDatoshithen). - Use new
OpcodeV0function as VM's PriceGetter for pre-Gorgon (it will returnDatoshi*factor, but with old coefficients). - Use new
OpcodeV1function as VM's PriceGetter for post-Gorgon (also will return `Datoshi*factor, but with new coefficients).
II
- Add an extra
factorargument toOpcode. Use pre-Gorgon coefficients iffactoris 0 and use post-Gorgon coefficients otherwise.
@roman-khimov, do you have other suggestions? Which one do you prefer?
fee.Calculate() becomes Gorgon-dependent.
The behaviour of fee.Calculate (and other similar and dependent APIs) becomes Gorgon-dependent.
I can suggest introducing fee.CalculateV0 (using pre-Gorgon coefficients and returning Datoshi*factor) and fee.CalculateV1 (using post-Gorgon coefficients, returning Datoshi*factor). Dependent functions will have to switch between two implementations by themselves based on Gorgon. It's also important to keep the original behaviour of fee.Calculate unchanged.
@roman-khimov also need your opinion on that.
| const factor = 1000 | ||
|
|
||
| // Opcode price weights used in dynamic fee formulas. | ||
| // All values are multiplied by vm.OpcodePriceMultiplier. |
There was a problem hiding this comment.
| // All values are multiplied by vm.OpcodePriceMultiplier. | |
| // All values are multiplied by [vm.OpcodePriceMultiplier]. |
There was a problem hiding this comment.
Use it for every link in doc.
| ) | ||
|
|
||
| // OpcodeV1 returns opcode pricing used since Gorgon hardfork, | ||
| // supporting dynamic (input-dependent) costs. |
There was a problem hiding this comment.
s/(input-dependent)/(parameter-dependent)
| } | ||
| ) | ||
|
|
||
| // OpcodeV1 returns opcode pricing used since Gorgon hardfork, |
There was a problem hiding this comment.
returns opcode pricing
... in the unit of Datoshi*[vm.OpcodePriceMultiplier]
| opcode.PUSH4, // amount of args | ||
| opcode.PACK, // pack args | ||
| ) | ||
| ) / vm.OpcodePriceMultiplier |
There was a problem hiding this comment.
It's OK for the old version of Opcode to divide by vm.OpcodePriceMultiplier in the middle of the script calculation. However, it won't be OK for OpcodeV1; the division should happen after the final price is known and the final value should be rounded up to cover non-integer remnant.
| // Array fields are still copied by reference. | ||
| func (i *Struct) Clone() (*Struct, error) { | ||
| // Clone returns a Struct with all Struct fields copied by the value | ||
| // and number of cloned items. Array fields are still copied by reference. |
There was a problem hiding this comment.
s/items/fields
... including the struct itself.
|
|
||
| // PriceArgs contains opcode-specific parameters used to calculate dynamic price. | ||
| type PriceArgs struct { | ||
| // Typ specifies type of stack item involved in opcode. |
There was a problem hiding this comment.
s/stack item/[stackitem.Item]
There was a problem hiding this comment.
s/involved in opcode/which in most of the cases serves as an operand of the given opcode.
| } | ||
|
|
||
| // PriceArgs contains opcode-specific parameters used to calculate dynamic price. | ||
| type PriceArgs struct { |
There was a problem hiding this comment.
s/PriceArgs/OpcodePriceArgs or something like that?
Just because vm.PriceArgs is too wide definition if referred from an external user package.
| // Size is size of compound type, buffer or bytearray length, | ||
| // number slots or stack elements. | ||
| Size int |
There was a problem hiding this comment.
| // Size is size of compound type, buffer or bytearray length, | |
| // number slots or stack elements. | |
| Size int | |
| // Size denotes one of the following: | |
| // - the number of elements in the compound type (i.e. the length of Array or Struct, the number of key-value pairs in Map); | |
| // - the length of Buffer or ByteArray; | |
| // - the number of VM slot cells or stack elements involved into opcode handling. | |
| Size int |
@Turalchik we need to be specific and precise. Imagine external users using this parameter to calculate something.
And may be it's worth to rename Size to Length?
Close #4043. Signed-off-by: Tural Devrishev <tural@nspcc.ru>
Close #4043.