-
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: master
Are you sure you want to change the base?
Changes from all commits
bcc5fd0
e63f10b
9d16c53
f746f8d
8d7f9e8
3fc8077
620d938
b31d95e
0c72e57
ee21427
4a6f36a
7e31d0c
993e3fe
37bf0cb
7dba130
0457ccd
d7a291e
6e780c0
6fff362
02d6ab9
4814339
9a657a5
e5e3b08
f43f296
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; | ||
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. Use 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. Notifications is a syscall, outside from VM 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. Its still an 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. We have |
||
|
||
private uint random_times = 0; | ||
|
||
/// <summary> | ||
|
@@ -394,9 +399,16 @@ protected internal void RuntimeNotifyV1(byte[] eventName, Array state) | |
/// <param name="state">The arguments of the event.</param> | ||
protected internal void SendNotification(UInt160 hash, string eventName, Array state) | ||
{ | ||
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) | ||
shargon marked this conversation as resolved.
Show resolved
Hide resolved
shargon marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
throw new InvalidOperationException($"Maximum number of notifications `{MaxNotificationCount}` is reached."); | ||
} | ||
NotifyEventArgs notification = new(ScriptContainer, hash, eventName, (Array)state.DeepCopy(asImmutable: true)); | ||
Notify?.Invoke(this, notification); | ||
notifications ??= new List<NotifyEventArgs>(); | ||
notifications.Add(notification); | ||
CurrentContext.GetState<ExecutionContextState>().NotificationCount++; | ||
} | ||
|
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.
@AnnaShaleva Should be removed from config
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.
It comes from the branch change