webb-tools / dkg-substrate

Multy-party threshold ECDSA (GG20) Substrate node
https://tangle.webb.tools/
GNU General Public License v3.0
59 stars 15 forks source link

[BUG] Potential Memory Leak in the Signing Rounds #447

Closed shekohex closed 1 year ago

shekohex commented 1 year ago

Describe the bug Looks like we do forget to clean up the already completed/stalled/terminated signing rounds inside the signing_rounds list, which result in that these references are held for the duration of the runtime, or until we override them. https://github.com/webb-tools/dkg-substrate/blob/0d86b54f57a38881ef0e555ec757b5324e5c8ca7/dkg-gadget/src/worker.rs#L137-L138 For a background about that bug, currently the signing_rounds list is capped at https://github.com/webb-tools/dkg-substrate/blob/0d86b54f57a38881ef0e555ec757b5324e5c8ca7/dkg-gadget/src/worker.rs#L94

And as per the recent changes we did, we do not allowing overwriting exciting rounds if it is still active: https://github.com/webb-tools/dkg-substrate/blob/0d86b54f57a38881ef0e555ec757b5324e5c8ca7/dkg-gadget/src/worker.rs#L368-L386

To Reproduce Steps to reproduce the behavior:

  1. Go to https://github.com/webb-tools/dkg-substrate/commit/0d86b54f57a38881ef0e555ec757b5324e5c8ca7
  2. Run ./scripts/run-standalone.sh after compiling the node.
  3. Wait for a few sessions (the more you wait the more garbage we create):
  4. Shutdown all nodes (ctrl+c)
  5. You will notice a lot of dkg_gadget::async_protocols: AsyncProtocolParameters(N)'s handler is going to be dropped where N is the number of sessions.

Log output

Log Output ```Paste log output here 2022-11-11 09:36:48.986 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.986 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 INFO tokio-runtime-worker dkg_gadget::async_protocols::remote: Shutting down meta handler: drop code 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped 2022-11-11 09:36:48.987 DEBUG tokio-runtime-worker dkg_gadget::async_protocols: AsyncProtocolParameters(13)'s handler is going to be dropped ```

Expected behaviour

No memory leak happens and we clean up unused resources as soon as we do not need them anymore.

Screenshots

Environment (please complete the following information):

Other information and links

dutterbutter commented 1 year ago

Although not entirely related, this issue outlines tooling to profile nodes for memory leaks: https://github.com/webb-tools/tangle/issues/18

drewstone commented 1 year ago

Is this still valid @shekohex?