add logs to execution #737

Jim8y · 2022-06-25T02:35:54Z

No description provided.

Jim8y · 2022-06-25T03:03:14Z

will update utest later

shargon

Could you do it without the neo changes? Attach an event before and remove later, like in neo UT

Jim8y · 2022-06-28T14:43:11Z

Could you do it without the neo changes? Attach an event before and remove later, like in neo UT

@shargon working on that

erikzhang · 2022-07-02T00:17:34Z

src/RpcServer/RpcServer.SmartContract.cs

@@ -83,6 +83,14 @@ private JObject GetInvokeResult(byte[] script, Signer[] signers = null, Witness[
                    obj["state"] = ToJson(n.State, session);
                    return obj;
                }));
+                json["logs"] = new JArray(session.Engine.Notifications.Select(n =>


This is incorrect.

This logic was when i try to add the log in the core, checking and removing useless code.

erikzhang · 2022-07-02T00:23:04Z

src/ApplicationLogs/LogReader.cs

+                }
+                list.Add(e);
+            }
+            ApplicationEngine.Log += Ev;


Should we add a setting in config.json to control whether logs are saved or not? I think maybe not all nodes want to do this.

The term log and notify is abusively miss used, I don't think the user could tell which is log and which is notify.

Jim8y · 2022-07-03T18:34:06Z

@nicolegys May you please help me test this?

src/ApplicationLogs/LogReader.cs

erikzhang · 2022-07-05T00:24:20Z

src/RpcServer/RpcServerPlugin.cs

+                }
+                _logs.Enqueue(e);
+            }
+            ApplicationEngine.Log += Ev;


This method will be called multiple times.

devhawk · 2022-07-08T18:48:00Z

Why are we doing this?

devhawk · 2022-07-08T18:49:44Z

src/ApplicationLogs/LogReader.cs

@@ -25,7 +25,7 @@ namespace Neo.Plugins
 {
    public class LogReader : Plugin
    {
-        private DB db;
+        private DB _db;


Changes like this make it much harder to review PRs. Reviewers need to check each change to see if it is substantive or name change.

src/RpcServer/RpcServer.csproj

devhawk · 2022-07-08T18:55:57Z

src/RpcServer/RpcServerPlugin.cs

+        // It is potentially possible to have dos attack by sending a lot of transactions and logs.
+        // To prevent this, we limit the number of logs to be logged per contract.
+        // If the number of logs is greater than MAX_LOG_EVENTS, we remove the oldest log.
+        private static void Ev(object _, LogEventArgs e)


All of this log tracking logic should be in RpcServer not RpcServerPlugin.

The separation of RpcServerPlugin and RpcServer is to enable NeoExpress to host RpcServer directly. So any changes to behavior of this plugin must be in RpcServer class so that NeoExpress can use it

devhawk · 2022-07-08T18:57:29Z

src/RpcServer/RpcServer.cs

@@ -49,6 +48,7 @@ public RpcServer(NeoSystem system, RpcServerSettings settings)
            Initialize_SmartContract();
        }

+


Like the variable name change in LogReader.cs, these kind of changes make it harder to review PRs. We should do changes like this in a dedicated PR like neo-project/neo#2785

devhawk

Log tracking logic needs to be moved into RpcServer

Jim8y · 2022-07-11T03:28:14Z

@devhawk done as you suggested

devhawk · 2022-07-11T16:43:26Z

src/RpcServer/RpcServerPlugin.cs

@@ -56,6 +52,13 @@ protected override void OnSystemLoaded(NeoSystem system)
            servers.TryAdd(s.Network, server);
        }

+        public override void Dispose()


It looks like we could revert all the changes to this file in this PR

devhawk · 2022-07-11T16:43:49Z

src/RpcServer/RpcServer.cs

+        /// <summary>
+        /// Maximum number of events to be logged per contract
+        /// </summary>
+        private const int MaxLogEvents = 50;


Should this be configurable?

+1 @Liaojinghui

Agree with @devhawk

devhawk · 2022-07-11T16:45:11Z

src/RpcServer/RpcServer.cs

+
+        private void OnCommitted(NeoSystem system, Block block)
+        {
+            LogEvents.Clear();


Should we be clearing all the events or just the ones for the transactions in block?

I think we should clear all log events, otherwise some logs will be there forever, which will be a memory leak.

If you clear them in OnCommitting, other plugins that depend on this data may not work. I think it should be cleaned up in OnCommitted by a certain plugin.

devhawk · 2022-07-11T16:45:57Z

src/RpcServer/RpcServer.cs

+        // It is potentially possible to have dos attack by sending a lot of transactions and logs.
+        // To prevent this, we limit the number of logs to be logged per contract.
+        // If the number of logs is greater than MAX_LOG_EVENTS, we remove the oldest log.
+        private static void Ev(object _, LogEventArgs e)


Can we change Ev to something more descriptive like OnLog

fixed

erikzhang · 2022-07-13T00:30:43Z

src/ApplicationLogs/LogReader.cs

                Put(appExec.Transaction.Hash.ToArray(), Neo.Utility.StrictUTF8.GetBytes(txJson.ToString()));
            }
+            RpcServer.LogEvents.Clear();


Clear() has been called in RpcServer.

This is to make sure logs being removed from the memory as soon as possible. Add nother one in the server to ensure logs being removed even if there is no log plugin.

… into log-in-execution * 'log-in-execution' of github.com:Liaojinghui/neo-modules: Fix nep11 tracker (#741)

nicolegys · 2022-09-05T10:20:55Z

Test pass

Jim8y · 2022-09-13T01:20:44Z

@shargon May you please review this pr?

superboyiii · 2022-10-09T06:58:40Z

@shargon @erikzhang Could you review it?

Jim8y added 5 commits June 24, 2022 22:35

add logs to execution

a634f59

fix issue

042db65

optimise code style

86a68cc

optimise code

a89b625

fix log message issue

f7eefd3

shargon reviewed Jun 25, 2022

View reviewed changes

Jim8y and others added 3 commits July 1, 2022 09:01

add log to transaction log

5b731eb

remove unused import

724fc54

Update LogReader.cs

4546018

erikzhang reviewed Jul 2, 2022

View reviewed changes

Jim8y added 5 commits July 2, 2022 13:51

Merge branch 'master' into log-in-execution

ac65c60

make initial more clear

ea6d01a

revert change to rpc client

e23f226

add logs to the rpcserver plugin

c45f703

use queue to manage logs

8888011

erikzhang reviewed Jul 3, 2022

View reviewed changes

src/ApplicationLogs/LogReader.cs Outdated Show resolved Hide resolved

update JArray

46aa190

erikzhang reviewed Jul 5, 2022

View reviewed changes

remove log event on dispose

7b9723e

devhawk reviewed Jul 8, 2022

View reviewed changes

src/RpcServer/RpcServer.csproj Outdated Show resolved Hide resolved

devhawk reviewed Jul 8, 2022

View reviewed changes

devhawk previously requested changes Jul 8, 2022

View reviewed changes

revert _db change and move log to rpcserver

397458c

fix error

391abe0

devhawk reviewed Jul 11, 2022

View reviewed changes

update the function name

c4b093d

erikzhang reviewed Jul 13, 2022

View reviewed changes

Jim8y added 7 commits July 12, 2022 21:31

Merge branch 'master' into log-in-execution

f7afd69

revert name change

12ded50

Merge branch 'log-in-execution' of github.com:Liaojinghui/neo-modules…

0bd5c11

… into log-in-execution * 'log-in-execution' of github.com:Liaojinghui/neo-modules: Fix nep11 tracker (#741)

revert csproj change

0aa880b

revert order change

c0f35ec

remove unused use

8a83e5c

Merge branch 'master' into log-in-execution

6d15ff0

superboyiii mentioned this pull request Sep 5, 2022

Neo v3.5.0 Checklist neo-project/neo#2811

Closed

Merge branch 'master' into log-in-execution

c0e72ac

Merge branch 'master' into log-in-execution

9c724dd

shargon and others added 4 commits October 10, 2022 01:21

Update RpcServerPlugin.cs

68f35f6

make log size configurable

5783063

Merge branch 'master' into log-in-execution

0e3ccb6

Merge branch 'master' into log-in-execution

12a36c6

Jim8y mentioned this pull request Nov 30, 2022

Transactions for invalid transfers neo-project/neo#2831

Open

roman-khimov mentioned this pull request Dec 8, 2022

question: how to get panic message? nspcc-dev/neo-go#2715

Closed

Merge branch 'master' into log-in-execution

b899935

Jim8y closed this by deleting the head repository Mar 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add logs to execution #737

add logs to execution #737

Jim8y commented Jun 25, 2022

Jim8y commented Jun 25, 2022

shargon left a comment •

edited

Loading

Jim8y commented Jun 28, 2022

erikzhang Jul 2, 2022

Jim8y Jul 3, 2022

erikzhang Jul 2, 2022

Jim8y Jul 2, 2022

Jim8y commented Jul 3, 2022

erikzhang Jul 5, 2022

devhawk commented Jul 8, 2022

devhawk Jul 8, 2022

devhawk Jul 8, 2022

devhawk Jul 8, 2022

devhawk left a comment

Jim8y commented Jul 11, 2022

devhawk Jul 11, 2022

devhawk Jul 11, 2022

nicolegys Sep 5, 2022

shargon Oct 10, 2022

devhawk Jul 11, 2022

Jim8y Jul 13, 2022

erikzhang Jul 17, 2022

devhawk Jul 11, 2022

erikzhang Jul 13, 2022

Jim8y Jul 13, 2022 •

edited

Loading

nicolegys commented Sep 5, 2022 •

edited

Loading

Jim8y commented Sep 13, 2022

superboyiii commented Oct 9, 2022

		@@ -49,6 +48,7 @@ public RpcServer(NeoSystem system, RpcServerSettings settings)
		Initialize_SmartContract();
		}

add logs to execution #737

add logs to execution #737

Conversation

Jim8y commented Jun 25, 2022

Jim8y commented Jun 25, 2022

shargon left a comment • edited Loading

Choose a reason for hiding this comment

Jim8y commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y commented Jul 3, 2022

Choose a reason for hiding this comment

devhawk commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devhawk left a comment

Choose a reason for hiding this comment

Jim8y commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

nicolegys commented Sep 5, 2022 • edited Loading

Jim8y commented Sep 13, 2022

superboyiii commented Oct 9, 2022

shargon left a comment •

edited

Loading

Jim8y Jul 13, 2022 •

edited

Loading

nicolegys commented Sep 5, 2022 •

edited

Loading