Skip to content

Commit

Permalink
core: ensure System.Runtime.GetNotifications can't break MaxStackSize
Browse files Browse the repository at this point in the history
This test ensures that NeoGo node doesn't have the DeepCopy problem
described in neo-project/neo#3300 and fixed in
neo-project/neo#3301. This problem leads to the
fact that Notifications items are not being properly refcounted by C#
node which leads to possibility to build an enormously large object on
stack. Go node doesn't have this problem.

The reason (at least, as I understand it) is in the fact that C# node
performs objects refcounting inside the DeepCopy even if the object
itself is not yet on stack. I.e. System.Runtime.Notify handler
immediately adds references to the notification argumetns inside
DeepCopy:
https://github.com/neo-project/neo/blob/b1d27f0189861167b8005a7e40e6d8500fb48cc4/src/Neo.VM/Types/Array.cs#L108
https://github.com/neo-project/neo/blob/b1d27f0189861167b8005a7e40e6d8500fb48cc4/src/Neo.VM/Types/Array.cs#L75

Whereas Go node just performs the honest DeepCopy without references counting:
https://github.com/nspcc-dev/neo-go/blob/b66cea5cccbcc8446312b012e29aaf2d1837430a/pkg/vm/stackitem/item.go#L1223

Going further, C# node clears refs for notification arguments (for array
and underlying array items). System.Runtime.GetNotifications pushes the
notificaiton args array back on stack and increments counter only for
the external array, not for its arguments. Which results in negative
refcounter once notificaiton is removed from the stack. The fix itself
(https://github.com/Jim8y/neo/blob/f471c0542ddd8f527478fbdcda76a3ab9194b958/src/Neo/SmartContract/NotifyEventArgs.cs#L84)
doesn't need to be ported to NeoGo because Go node adds object to the
refcounter only at the moment when it's being pushed to stack by
System.Runtime.GetNotifications handler. This object is treated as new
object since it was deepcopied earlier by System.Runtime.Notify handler:
https://github.com/nspcc-dev/neo-go/blob/b66cea5cccbcc8446312b012e29aaf2d1837430a/pkg/vm/stack.go#L178.

Thus, no functoinal changes from the NeoGo side. And we won't
intentionally break our node to follow C# pre-Domovoi invalid behaviour.

Close #3484, close #3482.

Signed-off-by: Anna Shaleva <[email protected]>
  • Loading branch information
AnnaShaleva committed Jun 11, 2024
1 parent 55fa123 commit effba1f
Showing 1 changed file with 75 additions and 0 deletions.
75 changes: 75 additions & 0 deletions pkg/core/blockchain_neotest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2620,3 +2620,78 @@ func TestBlockchain_StoreAsTransaction_ExecutableConflict(t *testing.T) {
require.NoError(t, err)
require.Equal(t, 2, len(aer))
}

// TestEngineLimits ensures that MaxStackSize limit is preserved during System.Runtime.GetNotifications
// syscall handling. This test is an adjusted port of https://github.com/lazynode/Tanya/pull/33 and makes
// sure that NeoGo node is not affected by https://github.com/neo-project/neo/issues/3300 and does not need
// the https://github.com/neo-project/neo/pull/3301.
func TestEngineLimits(t *testing.T) {
bc, acc := chain.NewSingle(t)
e := neotest.NewExecutor(t, bc, acc, acc)

src := `package test
import (
"github.com/nspcc-dev/neo-go/pkg/interop/runtime"
)
// args is an array of LargeEvent parameters containing 500 empty strings.
var args = []any{"", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "", "" };
func ProduceNumerousNotifications(count int) [][]any {
for i := 0; i < count; i++ {
runtime.Notify("LargeEvent", args...)
}
return runtime.GetNotifications(runtime.GetExecutingScriptHash())
}
func ProduceLargeObject(count int) int {
for i := 0; i < count; i++ {
runtime.Notify("LargeEvent", args...)
}
var (
smallObject = make([]int, 100)
res []int
)
for i := 0; i < count; i++ {
runtime.GetNotifications(runtime.GetExecutingScriptHash())
res = append(res, smallObject...)
}
return len(res)
}`
const eArgsCount = 500
eParams := make([]compiler.HybridParameter, eArgsCount)
for i := range eParams {
eParams[i].Name = fmt.Sprintf("str%d", i)
eParams[i].Type = smartcontract.ByteArrayType
}
c := neotest.CompileSource(t, acc.ScriptHash(), strings.NewReader(src), &compiler.Options{
Name: "test_contract",
ContractEvents: []compiler.HybridEvent{
{
Name: "LargeEvent",
Parameters: eParams,
},
},
})
e.DeployContract(t, c, nil)

// ProduceNumerousNotifications: 1 iteration, no limits are hit.
var args = make([]stackitem.Item, eArgsCount)
for i := range args {
args[i] = stackitem.Make("")
}
cInv := e.NewInvoker(c.Hash, acc)
cInv.Invoke(t, stackitem.Make([]stackitem.Item{
stackitem.Make([]stackitem.Item{
stackitem.Make(c.Hash),
stackitem.Make("LargeEvent"),
stackitem.Make(args),
}),
}), "produceNumerousNotifications", 1)

// ProduceNumerousNotifications: hit the limit.
cInv.InvokeFail(t, "stack is too big", "produceNumerousNotifications", 500)

// ProduceLargeObject: 1 iteration, no limits are hit.
cInv.Invoke(t, 100*1, "produceLargeObject", 1)

// ProduceLargeObject: hit the limit.
cInv.InvokeFail(t, "stack is too big", "produceLargeObject", 500)
}

0 comments on commit effba1f

Please sign in to comment.