chore(rln): update ganache #1347

s1fr0 · 2022-11-06T23:47:26Z

The npm package ganache-cli employed by onchain RLN tests is deprecated and is now replaced by ganache.

This issue was originally raised in #1344 (comment).

Scope

This PR:

replaces ganache-cli with ganache.
This update introduced some breaking changes which required some parameters previously passed to ganache-cli to be revised.
moves ganache installation logic from Makefile to the waku_rln_relay_onchain test suite.
This allows to run ganache only during execution of onchain tests and not during the whole duration of tests2. Furthermore, onchain RLN tests will be executed by CI on both linux and macOS platform (before only macOS was ran these tests in CI).
adds gracefully termination of ganache by sending a SIGINT signal.
uninstalls ganache when tests are terminated.

Out of scope

Any other extra feature.

… test + uninstall; gracefully terminate ganache

status-im-auto · 2022-11-07T00:03:09Z

Jenkins Builds

Click to see older builds (11)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`7e02161`	#1	2022-11-07 00:03:08	~15 min	`macos`	📦`bin`
✔️	`7e02161`	#1	2022-11-07 00:03:37	~15 min	`linux`	📦`bin`

✔️	`fa19a9e`	#2	2022-11-07 00:13:28	~15 min	`macos`	📦`bin`
✔️	`fa19a9e`	#2	2022-11-07 00:13:53	~15 min	`linux`	📦`bin`

✔️	`2db873d`	#3	2022-11-07 08:02:10	~14 min	`macos`	📦`bin`
✔️	`2db873d`	#3	2022-11-07 08:02:15	~14 min	`linux`	📦`bin`

✔️	`a6d620a`	#4	2022-11-07 08:34:28	~15 min	`linux`	📦`bin`
✔️	`a6d620a`	#4	2022-11-07 08:35:32	~16 min	`macos`	📦`bin`

✔️	`fd7142e`	#5	2022-11-07 18:42:17	~19 min	`macos`	📦`bin`
❌	`fd7142e`	#5	2022-11-07 18:42:40	~19 min	`linux`	📄`log`
✔️	`fd7142e`	#6	2022-11-07 22:47:54	~15 min	`linux`	📦`bin`

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`7bc657b`	#6	2022-11-08 10:45:04	~17 min	`macos`	📦`bin`
✔️	`7bc657b`	#7	2022-11-08 10:45:29	~18 min	`linux`	📦`bin`

✔️	`49f876f`	#7	2022-11-08 12:54:42	~14 min	`macos`	📦`bin`
✔️	`49f876f`	#8	2022-11-08 12:58:07	~18 min	`linux`	📦`bin`

rymnc

LGTM, please add package.json to the .gitignore, thanks!

s1fr0 · 2022-11-07T07:47:39Z

LGTM, please add package.json to the .gitignore, thanks!

Thanks! Done in 2db873d

LNSD · 2022-11-07T08:17:22Z

Merged #1344 into master. The following comment by @s1fr0 about running on-chain tests also in Linux remained unresponded, I am bringing it here since it is still relevant.

I think we can expand this test to both linux and macOS, I see no reason to test it only on the latter. @staheri14

LNSD

LGTM: Looks Great To Me 😁

Thank you for taking the time to solve this issue. Very much appreciated 🙏🏼

LNSD · 2022-11-07T08:04:55Z

tests/v2/test_waku_rln_relay_onchain.nim

+  let installGanache = startProcess("npm", args = ["install",  "ganache"], options = {poUsePath})
+  debug "Ganache installation completed. Printing install log", returnCode=installGanache.waitForExit(), log=installGanache.outputstream.readAll()


Chronicles at compile time remove all the statements below the -d:chronicles_log_level=<level> log level. So, in this case, if we set the log level to INFO, for example, it won't call waitForExit() nor readAll().

Suggested change

let installGanache = startProcess("npm", args = ["install", "ganache"], options = {poUsePath})

debug "Ganache installation completed. Printing install log", returnCode=installGanache.waitForExit(), log=installGanache.outputstream.readAll()

let installGanache = startProcess("npm", args = ["install", "ganache"], options = {poUsePath})

let installGanacheRc = installGanache.waitForExit()

let installGanacheOutStream = installGanache.outputstream.readAll()

debug "Ganache installation completed. Printing install log", returnCode=installGanacheRc, log=installGanacheOutStream

Done in 20dc2da

LNSD · 2022-11-07T08:08:20Z

tests/v2/test_waku_rln_relay_onchain.nim

+  # We gracefully terminate Ganache daemon
+  # We send a SIGINT signal to the runGanache PID to trigger RPC server termination and clean-up
+  debug "Sending SIGINT to Ganache", returnCode=kill(ganachePID.int32, SIGINT)
+  debug "Ganache daemon terminated. Printing run log", returnCode=runGanache.waitForExit(), log=runGanache.outputstream.readAll()


Same here.

Suggested change

# We gracefully terminate Ganache daemon

# We send a SIGINT signal to the runGanache PID to trigger RPC server termination and clean-up

debug "Sending SIGINT to Ganache", returnCode=kill(ganachePID.int32, SIGINT)

debug "Ganache daemon terminated. Printing run log", returnCode=runGanache.waitForExit(), log=runGanache.outputstream.readAll()

# We gracefully terminate Ganache daemon

# We send a SIGINT signal to the runGanache PID to trigger RPC server termination and clean-up

let ganacheKillRc = kill(ganachePID.int32, SIGINT)

debug "Sent SIGINT to Ganache", returnCode=ganacheKillRc

let ganacheDaemonRc = runGanache.waitForExit()

let ganacheDaemonOutStream = runGanache.outputstream.readAll()

debug "Ganache daemon terminated. Printing run log", returnCode=ganacheDaemonRc, log=ganacheDaemonOutStream

Done in 20dc2da

LNSD · 2022-11-07T08:10:03Z

tests/v2/test_waku_rln_relay_onchain.nim

+
+  # We uninstall Ganache
+  let uninstallGanache = startProcess("npm", args = ["uninstall",  "ganache"], options = {poUsePath})
+  debug "Ganache uninstall completed. Printing uninstall log", returnCode=uninstallGanache.waitForExit(), log=uninstallGanache.outputstream.readAll()


And here.

Suggested change

debug "Ganache uninstall completed. Printing uninstall log", returnCode=uninstallGanache.waitForExit(), log=uninstallGanache.outputstream.readAll()

let uninstallGanacheRc = uninstallGanache.waitForExit()

let uninstallGanacheOutStream = uninstallGanache.outputstream.readAll()

debug "Ganache uninstall completed. Printing uninstall log", returnCode=uninstallGanacheRc, log=uninstallGanacheOutStream`

Done in 20dc2da

LNSD · 2022-11-07T08:21:54Z

tests/v2/test_waku_rln_relay_onchain.nim

+  ########################
+  ## Ganache installation
+  ########################


Can you wrap the ganache install and start logic in a proc (e.g., proc setupGanache()) and call it here?

Done in 20dc2da

LNSD · 2022-11-07T08:22:49Z

tests/v2/test_waku_rln_relay_onchain.nim

+  ################################
+  ## Terminating/removing Ganache
+  ################################


Same here. Can you wrap the ganache kill and uninstall logic in a proc (e.g., proc cleanupGanache()) and call it here?

Done in 20dc2da

LNSD · 2022-11-07T08:27:04Z

LGTM, please add package.json to the .gitignore, thanks!

@rymnc Sorry for my ignorance, but why do you need to add package.json to the .gitignore? Is ganache generating a package.json when it is installed? 🤔

s1fr0 · 2022-11-07T08:27:52Z

@LNSD During my testing for this PR with process spawning etc. in nim, I realized that some of osproc APIs might act differently in macOS and linux (and windows) or lack some features.

Hence, I strongly believe that tests (and in particular these ones) should happen on both platforms, even though this means longer tests.

Of course, we might allow exceptions with well documented reasons. But IMO this is not the case and by running them on both platform we just give users more confidence that everything is working fine.

rymnc · 2022-11-07T08:28:17Z

@rymnc Sorry for my ignorance, but why do you need to add package.json to the .gitignore? Is ganache generating a package.json when it is installed? 🤔

Yeah, the npm install adds one by default :)

s1fr0 · 2022-11-07T08:29:12Z

LGTM, please add package.json to the .gitignore, thanks!

@rymnc Sorry for my ignorance, but why do you need to add package.json to the .gitignore? Is ganache generating a package.json when it is installed? 🤔

Yes, and when I uninstall ganache, it leaves an empty {} JSON file. I can try to move it to the build folder. At the moment (and before) it was installed in main nwaku folder.

LNSD · 2022-11-07T08:37:17Z

I think we can expand this test to both linux and macOS, I see no reason to test it only on the latter. @staheri14

My only concern about running the on-chain RLN tests in Linux is that it takes more than 1h to execute each task in the GH Actions CI. The macOS jobs were already failing due to timeout.

We need to investigate why it is taking too much time (on-chain tests are doubling the current tests duration)

alrevuelta

looks great! I always had the impression the Makefile wasn't the right place for ganache.

…e uninstalled

staheri14

Good one! left a comment, also I agree with @LNSD comment about creating separate procs for ganache setup and termination.

staheri14 · 2022-11-07T17:48:39Z

tests/v2/test_waku_rln_relay_onchain.nim

+
+  # We run Ganache daemon
+  # Note that we run directly "node node_modules/ganache/dist/node/cli.js" rather than using "npx ganache", so that the daemon does not spawn in a new child process. 
+  # In this way, we can directly send a SIGINT signal to the corresponding PID to gracefully terminate Ganache without dealing with multiple processes.


I'd add the description of the argument as well as a link to the ganache documentation https://github.com/trufflesuite/ganache#documentation:
-p stands for port, -l sets the gas limit, and -e sets the default account balance, specified in ether.

Indeed! This is a very good point, I completely missed it:

@s1fr0 instead of using the short version of the flags, can you use the long version (e.g., --port instead of -p)?

Absolutely! Options documented and switched to long version in 20dc2da

staheri14 · 2022-11-07T17:58:00Z

onchain RLN tests will be executed by CI on both linux and macOS platform (before only macOS was ran these tests in CI).

@s1fr0
I think the on-chain tests will be run in CIs regardless of the OS since the OS check on the CI is removed as part of this PR.

s1fr0 · 2022-11-07T18:02:13Z

onchain RLN tests will be executed by CI on both linux and macOS platform (before only macOS was ran these tests in CI).

@s1fr0 I think the on-chain tests will be run in CIs regardless of the OS since the OS check on the CI is removed as part of this PR.

Yes, this PR removes previous check on the OS for the CI, hence onchain tests will be executed on both platforms.

rymnc · 2022-11-07T18:22:22Z

I think we can expand this test to both linux and macOS, I see no reason to test it only on the latter. @staheri14

My only concern about running the on-chain RLN tests in Linux is that it takes more than 1h to execute each task in the GH Actions CI. The macOS jobs were already failing due to timeout.

We need to investigate why it is taking too much time (on-chain tests are doubling the current tests duration)

Actually, the tests fail, and I guess the ganache process is not killed appropriately (even though there is a pkill in the Makefile for it). That's why they live for an hour. This PR should resolve that!

s1fr0 · 2022-11-07T19:47:32Z

@rymnc The tests went fine today (now jenkins seems to fail for an unrelated one) but I obseverd an absurd amount of time while building macOS (not even in test): did you notice some specific test failing that relates to this? Of course it is likely affecting something, but I'm not completely sure it's the (only) reason.

rymnc · 2022-11-08T09:23:27Z

@rymnc The tests went fine today (now jenkins seems to fail for an unrelated one) but I obseverd an absurd amount of time while building macOS (not even in test): did you notice some specific test failing that relates to this? Of course it is likely affecting something, but I'm not completely sure it's the (only) reason.

Unsure, macOS always took longer than linux to build and test

…nto update-ganache

s1fr0 · 2022-11-08T10:33:36Z

FYI in 37fd9c8 I moved installation of nodes modules to the build folder as proposed in #1347 (comment). I believe it makes sense to have there cache of installed/built packages since are the result of a building process, git will automatically ignore files there (with no need to manually change .gitignore) and make clean will delete them.

s1fr0 · 2022-11-08T13:25:52Z

Merging. See #1356 for a possible follow-up improvement to this PR.

Lorenzo Delgado and others added 2 commits November 4, 2022 15:20

chore: build rln dependecies only when building v2

f21d753

chore(rln): deprecate ganache-cli; move installation from makefile to…

7e02161

… test + uninstall; gracefully terminate ganache

s1fr0 added the track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications label Nov 6, 2022

s1fr0 requested review from LNSD, staheri14 and rymnc November 6, 2022 23:47

s1fr0 self-assigned this Nov 6, 2022

s1fr0 mentioned this pull request Nov 6, 2022

chore: build rln dependecies only when building v2 #1344

Merged

4 tasks

Merge branch 'master' into update-ganache

fa19a9e

rymnc approved these changes Nov 7, 2022

View reviewed changes

chore: add package.json to gitignore

2db873d

Merge branch 'master' into update-ganache

a6d620a

LNSD approved these changes Nov 7, 2022

View reviewed changes

alrevuelta reviewed Nov 7, 2022

View reviewed changes

fix(rln): move ganache package to build folder and remove package onc…

37fd9c8

…e uninstalled

staheri14 approved these changes Nov 7, 2022

View reviewed changes

Merge branch 'master' into update-ganache

fd7142e

s1fr0 added 2 commits November 8, 2022 11:22

refactor(rln): (un)install/run/stop ganache with separate procs

20dc2da

Merge branch 'update-ganache' of https://github.com/status-im/nwaku i…

7bc657b

…nto update-ganache

Merge branch 'master' into update-ganache

49f876f

s1fr0 merged commit 17d71fa into master Nov 8, 2022

s1fr0 deleted the update-ganache branch November 8, 2022 13:28

LNSD mentioned this pull request Nov 9, 2022

Rln-Relay features/refactoring targeted for release 0.13.0 #1268

Closed

29 tasks

rymnc added this to the Release 0.13.0 milestone Nov 14, 2022

alrevuelta mentioned this pull request Nov 14, 2022

docs: release v0.13.0 #1378

Merged

rymnc mentioned this pull request Mar 15, 2023

Terminating ganache-cli process on windows #833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(rln): update ganache #1347

chore(rln): update ganache #1347

s1fr0 commented Nov 6, 2022 •

edited

status-im-auto commented Nov 7, 2022 •

edited

rymnc left a comment

s1fr0 commented Nov 7, 2022

LNSD commented Nov 7, 2022 •

edited

LNSD left a comment

LNSD Nov 7, 2022

s1fr0 Nov 8, 2022

LNSD Nov 7, 2022

s1fr0 Nov 8, 2022

LNSD Nov 7, 2022

s1fr0 Nov 8, 2022

LNSD Nov 7, 2022

alrevuelta Nov 7, 2022

s1fr0 Nov 8, 2022

LNSD Nov 7, 2022

s1fr0 Nov 8, 2022

LNSD commented Nov 7, 2022

s1fr0 commented Nov 7, 2022

rymnc commented Nov 7, 2022

s1fr0 commented Nov 7, 2022

LNSD commented Nov 7, 2022

alrevuelta left a comment

staheri14 left a comment

staheri14 Nov 7, 2022

LNSD Nov 7, 2022 •

edited

s1fr0 Nov 8, 2022

staheri14 commented Nov 7, 2022 •

edited

s1fr0 commented Nov 7, 2022

rymnc commented Nov 7, 2022

s1fr0 commented Nov 7, 2022

rymnc commented Nov 8, 2022

s1fr0 commented Nov 8, 2022 •

edited

s1fr0 commented Nov 8, 2022

		let installGanache = startProcess("npm", args = ["install", "ganache"], options = {poUsePath})
		debug "Ganache installation completed. Printing install log", returnCode=installGanache.waitForExit(), log=installGanache.outputstream.readAll()

-  debug "Ganache uninstall completed. Printing uninstall log", returnCode=uninstallGanache.waitForExit(), log=uninstallGanache.outputstream.readAll()
+  let uninstallGanacheRc = uninstallGanache.waitForExit()
+  let uninstallGanacheOutStream = uninstallGanache.outputstream.readAll()
+  debug "Ganache uninstall completed. Printing uninstall log", returnCode=uninstallGanacheRc, log=uninstallGanacheOutStream`

chore(rln): update ganache #1347

chore(rln): update ganache #1347

Conversation

s1fr0 commented Nov 6, 2022 • edited

Scope

Out of scope

status-im-auto commented Nov 7, 2022 • edited

Jenkins Builds

rymnc left a comment

Choose a reason for hiding this comment

s1fr0 commented Nov 7, 2022

LNSD commented Nov 7, 2022 • edited

LNSD left a 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

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

LNSD commented Nov 7, 2022

s1fr0 commented Nov 7, 2022

rymnc commented Nov 7, 2022

s1fr0 commented Nov 7, 2022

LNSD commented Nov 7, 2022

alrevuelta left a comment

Choose a reason for hiding this comment

staheri14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LNSD Nov 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staheri14 commented Nov 7, 2022 • edited

s1fr0 commented Nov 7, 2022

rymnc commented Nov 7, 2022

s1fr0 commented Nov 7, 2022

rymnc commented Nov 8, 2022

s1fr0 commented Nov 8, 2022 • edited

s1fr0 commented Nov 8, 2022

s1fr0 commented Nov 6, 2022 •

edited

status-im-auto commented Nov 7, 2022 •

edited

LNSD commented Nov 7, 2022 •

edited

LNSD Nov 7, 2022 •

edited

staheri14 commented Nov 7, 2022 •

edited

s1fr0 commented Nov 8, 2022 •

edited