Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Neo Core Bug]fix 3300 #3301

Merged
merged 15 commits into from
Jun 11, 2024
Merged

[Neo Core Bug]fix 3300 #3301

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1f8e36a
fix 3300
Jim8y Jun 7, 2024
c8d9322
update format
Jim8y Jun 7, 2024
81abdd3
add state subitems to ref counter, with suggestion from DuSmart
Jim8y Jun 7, 2024
fde92e6
Merge branch 'master' into fix-3300
shargon Jun 7, 2024
ee4c159
apply hardfork
Jim8y Jun 7, 2024
d8e12be
Merge branch 'fix-3300' of github.com:Jim8y/neo into fix-3300
Jim8y Jun 7, 2024
c723ae1
format
Jim8y Jun 7, 2024
dca720b
my mistake
Jim8y Jun 7, 2024
814d334
fix hardfork
Jim8y Jun 7, 2024
0bf9d11
remove negative check
Jim8y Jun 7, 2024
afc5349
Merge branch 'master' into fix-3300
shargon Jun 7, 2024
6afd380
add unit test
Jim8y Jun 8, 2024
2e742cc
Merge branch 'fix-3300' of github.com:Jim8y/neo into fix-3300
Jim8y Jun 8, 2024
6529bfa
apply anna's suggestion
Jim8y Jun 10, 2024
f471c05
Merge branch 'master' into fix-3300
NGDAdmin Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/Neo.VM/ReferenceCounter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@

using Neo.VM.StronglyConnectedComponents;
using Neo.VM.Types;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.CompilerServices;
using Buffer = Neo.VM.Types.Buffer;

namespace Neo.VM
{
Expand Down Expand Up @@ -115,7 +117,7 @@ internal int CheckZeroReferred()
tracked_items.Remove(item);
if (item is CompoundType compound)
{
references_count -= compound.SubItemsCount;
DecreaseCounter(compound.SubItemsCount);
foreach (StackItem subitem in compound.SubItems)
{
if (component.Contains(subitem)) continue;
Expand All @@ -136,7 +138,7 @@ internal int CheckZeroReferred()

internal void RemoveReference(StackItem item, CompoundType parent)
{
references_count--;
DecreaseCounter();
if (!NeedTrack(item)) return;
cached_components = null;
item.ObjectReferences![parent].References--;
Expand All @@ -146,10 +148,17 @@ internal void RemoveReference(StackItem item, CompoundType parent)

internal void RemoveStackReference(StackItem item)
{
references_count--;
DecreaseCounter();
if (!NeedTrack(item)) return;
if (--item.StackReferences == 0)
zero_referred.Add(item);
}

private void DecreaseCounter(int count = 1)
{
references_count -= count;
if (references_count < 0) throw new InvalidOperationException("Reference counter is negative.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But how is it ever possible? Have you faced with the issue when refcount is negative?

Copy link
Contributor Author

@Jim8y Jim8y Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, its @dusmart who found the issue. With Array object deepcopied and removed from the stack, all of its references are removed as well, when we add that copied array to the stack, its internal subitems will not be added to the reference counter, but when we remove that copied item from the stack again, the counter will consider the array subitems.

BArray = AArray.DeepCopy

Remove BArray from stack // this will remove BArray and its subitem from the counter.

Aadd BArray back to stack // This will only add BArray itself back to the counter

Remove BArray from stack // This will remove BArray and its subitems from the counter, and create minus counter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AArray.DeepCopy doesn't increase the counter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AArray.DeepCopy doesn't increase the counter?

It should increase the counter:

neo/src/Neo.VM/Types/Array.cs

Line 75 in 6e8c730

referenceCounter.AddReference(item, this);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (references_count < 0) throw new InvalidOperationException("Reference counter is negative.");

I think we should not change this code. It's a bug in the handlers implementation, not in the reference counter logic. So we should fix the source problem, not its consequence.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. reference counter is supposed to be not negative, it should be the counter's property, should not rely on the callers.

@Jim8y let's discuss it here, we have a designated thread for it. If we add this change, then it may affect some old transactions. And I'd like to see @roman-khimov's opinion on this topic.

Copy link
Contributor Author

@Jim8y Jim8y Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? isn't you saying its supposed to be positive? then what problem can this change cause? It happened to be negative in the past?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggest to move the reference counter into another pr and keep the fix simple


}
}
}
12 changes: 9 additions & 3 deletions src/Neo/SmartContract/ApplicationEngine.Runtime.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,14 +407,20 @@ protected internal void SendNotification(UInt160 hash, string eventName, Array s
/// </summary>
/// <param name="hash">The hash of the specified contract. It can be set to <see langword="null"/> to get all notifications.</param>
/// <returns>The notifications sent during the execution.</returns>
protected internal NotifyEventArgs[] GetNotifications(UInt160 hash)
protected internal Array GetNotifications(UInt160 hash)
{
IEnumerable<NotifyEventArgs> notifications = Notifications;
if (hash != null) // must filter by scriptHash
notifications = notifications.Where(p => p.ScriptHash == hash);
NotifyEventArgs[] array = notifications.ToArray();
var array = notifications.ToArray();
if (array.Length > Limits.MaxStackSize) throw new InvalidOperationException();
return array;
Array notifyArray = new(ReferenceCounter);
foreach (var notify in array)
{
notifyArray.Add(notify.ToStackItem(ReferenceCounter));
}

return notifyArray;
Jim8y marked this conversation as resolved.
Show resolved Hide resolved
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Neo/SmartContract/NotifyEventArgs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public StackItem ToStackItem(ReferenceCounter referenceCounter)
{
ScriptHash.ToArray(),
EventName,
State
State.OnStack ? State : State.DeepCopy(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that this change is the one that actually fixes the problem. But the problem is that it requires one more hardfork if we care about safety since it changes the behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just apply to the hardfork you created. will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardfork has being applied.

};
}
}
Expand Down