-
Notifications
You must be signed in to change notification settings - Fork 1k
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.
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
SmartContract: restrict the number of allowed notifications #3548
base: HF_Echidna
Are you sure you want to change the base?
Changes from 2 commits
b31d95e
0c72e57
ee21427
4a6f36a
6fff362
4814339
9a657a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,11 @@ partial class ApplicationEngine | |
/// </summary> | ||
public const int MaxNotificationSize = 1024; | ||
|
||
/// <summary> | ||
/// The maximum number of notifications per application execution. | ||
/// </summary> | ||
public const int MaxNotificationCount = 512; | ||
|
||
private uint random_times = 0; | ||
|
||
/// <summary> | ||
|
@@ -395,8 +400,15 @@ protected internal void RuntimeNotifyV1(byte[] eventName, Array state) | |
protected internal void SendNotification(UInt160 hash, string eventName, Array state) | ||
{ | ||
NotifyEventArgs notification = new(ScriptContainer, hash, eventName, (Array)state.DeepCopy(asImmutable: true)); | ||
Notify?.Invoke(this, notification); | ||
notifications ??= new List<NotifyEventArgs>(); | ||
// Restrict the number of notifications for Application executions. Do not check | ||
shargon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// persisting triggers to avoid native persist failure. Do not check verification | ||
// trigger since verification context is loaded with ReadOnly flag. | ||
if (IsHardforkEnabled(Hardfork.HF_Echidna) && Trigger == TriggerType.Application && notifications.Count == MaxNotificationCount) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks good to me, we just need to ensure .Count is correct calculated at this step. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is trigger really needed? Limits shouldn't have a trigger. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it's needed, because we can't allow OnPersist and PostPersist failures (imagine that in future MaxNotificationCount may be reduced to some lower value). And for Verification trigger this check is useless by design, no notifications are possible with this trigger. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
shargon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
throw new InvalidOperationException($"Maximum number of notifications `{MaxNotificationCount}` is reached."); | ||
} | ||
Notify?.Invoke(this, notification); | ||
notifications.Add(notification); | ||
CurrentContext.GetState<ExecutionContextState>().NotificationCount++; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -78,10 +78,10 @@ | |
list.AddRange(nodes); | ||
list.Sort(); | ||
engine.SnapshotCache.Add(key, new StorageItem(list)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. revert There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Formatting job is failing without this change. |
||
|
||
if (engine.IsHardforkEnabled(Hardfork.HF_Echidna)) | ||
{ | ||
var oldNodes = new VM.Types.Array(engine.ReferenceCounter, GetDesignatedByRole(engine.Snapshot, role, index - 1).Select(u => (ByteString)u.EncodePoint(true))); | ||
Check warning on line 84 in src/Neo/SmartContract/Native/RoleManagement.cs GitHub Actions / Test (ubuntu-latest)
Check warning on line 84 in src/Neo/SmartContract/Native/RoleManagement.cs GitHub Actions / Test-Everything
Check warning on line 84 in src/Neo/SmartContract/Native/RoleManagement.cs GitHub Actions / Test-Everything
Check warning on line 84 in src/Neo/SmartContract/Native/RoleManagement.cs GitHub Actions / Test-Everything
Check warning on line 84 in src/Neo/SmartContract/Native/RoleManagement.cs GitHub Actions / Test-Everything
Check warning on line 84 in src/Neo/SmartContract/Native/RoleManagement.cs GitHub Actions / Test (macos-latest)
Check warning on line 84 in src/Neo/SmartContract/Native/RoleManagement.cs GitHub Actions / Test (macos-latest)
Check warning on line 84 in src/Neo/SmartContract/Native/RoleManagement.cs GitHub Actions / Test (macos-latest)
|
||
var newNodes = new VM.Types.Array(engine.ReferenceCounter, nodes.Select(u => (ByteString)u.EncodePoint(true))); | ||
|
||
engine.SendNotification(Hash, "Designation", new VM.Types.Array(engine.ReferenceCounter, [(int)role, engine.PersistingBlock.Index, oldNodes, newNodes])); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
ExecutionEngineLimits
Classengine.Limits.MaxNotificationCount
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notifications is a syscall, outside from VM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its still an
Engine
Limitation, just cause the VM is splitted into two libraries shouldn't make it be to separate things.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have
MaxNotificationSize
andMaxEventName
declared in the same class asMaxNotificationCount
, these three constants must be kept together.