feat: membership #13

richard-ramos · 2024-09-03T17:34:10Z

Description

Adds membership management as described in #11 and https://github.com/waku-org/specs/blob/master/standards/core/rln-contract.md .

Some test units are still pending but the contract is already in a state in which comments on it can be made.
I added some observations too about some points in the contract which require clarification.

Some changes regarding the spec were made based on comments on #11 as well as removing the need of having an actual membership state stored, since this can be determined based on the expiration dates and existence of the membership in the contract.

To track the membership age I implemented this as a double linked list. It's worth mentioning that keeping track of the expired memberships means some gas expense due to having to maintain the linking for the memberships in this list. I'm exploring whether using array that grows infinitely as a list and keeping track of the index in which to start navigating the array, to see if I can save some gas this way. Suggestions of other data structures to keep track of this information are appreciated, although maybe we could consider not keeping track of the oldest memberships and have this indexing being done offchain.

Tomorrow I expect to finish the pending test units based on this implementation.

Checklist

Ensure you completed all of the steps below before submitting your pull request:

Added natspec comments?
Ran pnpm adorno?

src/IPriceCalculator.sol

src/Membership.sol

richard-ramos · 2024-09-05T21:51:57Z

src/Membership.sol

+    /// @param userMessageLimit The user message limit
+    /// @return true if the user message limit is valid, false otherwise
+    function isValidUserMessageLimit(uint32 userMessageLimit) external view returns (bool) {
+        return userMessageLimit >= minRateLimitPerMembership && userMessageLimit <= maxRateLimitPerMembership;


This does not take into account the expired memberships, although since it's an external function I could attempt to optionally calculate whether the rate limit of currently expired memberships would allow the userMessageLimit to be registered. WDYT?

src/Membership.sol

src/IPriceCalculator.sol

src/Membership.sol

richard-ramos · 2024-09-05T21:59:23Z

src/Membership.sol

+        }
+
+        // Attempt to free expired membership slots
+        while (totalRateLimitPerEpoch + _rateLimit > maxTotalRateLimitPerEpoch) {


This will try to remove expired memberships sequentially, but it's possible that the accumulated rate limit of all the memberships being removed is not enough for the chosen rate limit to acquire. Should we also accept an optional list of memberships to expire, so the user of the contract can explore the contract offchain and pass a list expired memberships whose accumulated rate limits is enough to acquire the rate limit they need?

Something interesting here is to prefer inserting new elements on the tree rather than attempt to erase expired memberships first, mainly because there's no guarantee that an expired membership rate limit will match the rate limit to acquire. (and then having to iterate to obtain enough rate limit from expired memberships).

there's no guarantee that an expired membership rate limit will match the rate limit to acquire

Is it technically possible to track / predict how much expired rate limit capacity we will have at a given point in time? (So that we can quickly tell if it's even worth it to start iterating through expired membership in an attempt to overwrite them, or their rate limit won't suffice anyway.)

A proposal we discussed in a call was that:

optional "slot to override" parameter when doing the insertion

if no slot to override specified, then it tries to grab next available slot in tree. If global rate limit gets exceeded, it fials

if slot to override is specified:

if slot to override is not over grace period, fails

if slot to override rate limit < rate limit of new membership, fails
Otherwise, replace slot.

This means that in terms of gas cost, it's more predictable:

there is space, low gas cost

no space, most specific membership to override, higher gas cost

than the previous idea of "override if there is something to override, otherwise do not".

Also, we should have a call on contract to purge memberships over grace period. so we can be the one to cover the purge cost.

if no slot to override specified, then it tries to grab next available slot in tree

I'd like to emphasize here that we have (at least) two types of limited resource:

contract storage space;

total rate limit.

The idea of slot overwriting was initially meant to save contract storage space. Later on, a question emerged (in my head at least): is saving this particular resource worth the increased gas costs? Agressively overwriting old memberships would definitely consume more gas (at the expense of the user who registers a new membership), whereas savings from a smaller tree are a) not guaranteed; b) spread out across everyone; c) not even that large (?) (I'm not sure how to qunatify it).

TLDR: do we need to

grab next available slot in tree

every time, or is it perhaps better to do this (more) lazily?

Is it technically possible to track / predict how much expired rate limit capacity we will have at a given point in time? (So that we can quickly tell if it's even worth it to start iterating through expired membership in an attempt to overwrite them, or their rate limit won't suffice anyway.)

I'd say this can be done offchain and then via function parameters indicate to the contract whether we want to: just insert a new leaf in the tree / have the contract remove automatically expired / remove specific memberships. Then nwaku could call estimateGas on any of these options and choose the one that incurs in less gas cost.

A proposal we discussed in a call ...

Ah! this seems to be aligned with what I just mentioned

TLDR: do we need to grab next available slot in tree every time, or is it perhaps better to do this (more) lazily?

This is hard to answer without testing. Perhaps we can have the three options I mentioned earlier, and have some script to try to determine the behavior of the contract once we have a large enough number of memberships.

@fryorcraken, @s-tikhomirov ,
In bf757e7 the register function was modified and now exists as two overloaded functions:

register(uint256 idCommitment, uint32 userMessageLimit); register(uint256 idCommitment, uint32 userMessageLimit, uint256[] calldata membershipsToErase)

I'm still working on the test units for the last function but this should cover what was discussed before.

The idea of slot overwriting was initially meant to save contract storage space

Interesting, for me it was about booting off the network publishers with expired membership to keep network traffic and membership set closely related.

every time, or is it perhaps better to do this (more) lazily?

As @richard-ramos ' stated:

register(uint256 idCommitment, uint32 userMessageLimit); register(uint256 idCommitment, uint32 userMessageLimit, uint256[] calldata membershipsToErase)

With 2 functions on the contrat, it is now possible for the app to provide more feedback:

App sees there is space on the tree (R_{max} not reached yet, including new membership).

App can use register(uint256 idCommitment, uint32 userMessageLimit); and only an insertion happen

else

App sees there is no space on the tree (R_{max} is reached when including new membership).

App finds an Expired membership with greater or equal rate limit (a good actor would actually use the expired membership with greatest rate limit)

App can warn user that membership insertion would cost a bit more than usual

Insertion can be done with register(uint256 idCommitment, uint32 userMessageLimit, uint256[] calldata membershipsToErase)

This gives more predictability to the user.
App devs can also decide to spend the gas to purge Expired memberships to make it cheaper for users.

s-tikhomirov · 2024-09-06T15:53:48Z

Left a few comments, will look into the code in more detail next week, thank you!

One meta-point I'd like to make for now: I think we should discuss and make a decision on @alrevuelta's proposal about billing periods. If we conclude that this is a good idea, then I think we should first amend the spec to reflect these changes, and then review the code to ensure it matches the updated spec.

jm-clius · 2024-09-11T14:37:46Z

Great work!

@kaiserd @0x-r4bbit could we get a review from the Smart Contract team as well?

0x-r4bbit · 2024-09-12T09:49:27Z

Looking into this now

0x-r4bbit

Left some comments.

Generally, you could make use of custom errors to save some gas

src/LinearPriceCalculator.sol

src/Membership.sol

src/WakuRlnV2.sol

richard-ramos · 2024-09-12T19:28:14Z

Thank you for the review, @0x-r4bbit. I fixed the items and included your suggestion in the last commit

src/WakuRlnV2.sol

s-tikhomirov · 2024-09-24T07:43:49Z

src/WakuRlnV2.sol

+    /// @param reusedIndex indicates whether we're inserting a new element in the merkle tree or updating a existing
+    /// leaf
+    function _register(uint256 idCommitment, uint32 userMessageLimit, uint32 index, bool reusedIndex) internal {
+        if (nextCommitmentIndex >= SET_SIZE) revert FullTree();


Can this check be done in the two register functions alongside their mofidiers? That would allow us to save gas by reverting earlier if the membership set is full, before even calling _register.

src/WakuRlnV2.sol

* refactor: do not keep track of membership registration order * unify terminology for mdetails * more DRY in LinearPriceCalculator * improve terminology consistency (#16) * refactor, minor fixes * do not reset ongoing grace period on extension (cf. spec change) * minor renaming, comments * refactor, group functions by funcitonality * extract membership expiration calculation to internal function * save active duration per membership (must carry over after extension) * optional lazy erasure from membership set * minor fixes in tests * fix: off-by-one: end index can't be equal to next free index * minor refactoring, comments * define period boundaries: start inclusive, end exclusive * separate eraseMembership functions to user-focused (lazy) and admin-focused (tree cleanup) * minor fix to maintain line lengths * unify membership-related events * add test for zero grace period * fix typo in comment Co-authored-by: richΛrd <[email protected]> --------- Co-authored-by: richΛrd <[email protected]> * code review * refactor: unique register function * fix: split errors of `_eraseMembershipLazily` * chore: minor naming clarifications --------- Co-authored-by: Sergei Tikhomirov <[email protected]>

This allows the contract to transfer ownership to address 0x000...000 as defined in waku-org/specs#34: `At some point, the _Owner_ SHOULD renounce their privileges, and the contract MUST become immutable`. The problem with `Ownable2StepUpgradeable` is that it requires accepting the ownership transfer, which is not possible with address 0x0

jm-clius

@richard-ramos @s-tikhomirov are there any blockers preventing this PR from being approved and merged? I know we may want to deploy this to a testnet to see how well it works in practice, but presumably we can address any new issues in subsequent PRs?

richard-ramos · 2024-10-22T14:20:41Z

@jm-clius
I havent merged it because of it not being approved ✔️ :(

jm-clius · 2024-10-22T16:48:53Z

@s-tikhomirov @0x-r4bbit could we get approvals if you're happy with scope of functionality on one hand and implementation on the other?

0x-r4bbit

Sorry, didn't mean to block this here!

s-tikhomirov · 2024-10-23T08:52:36Z

LGTM!

richard-ramos · 2024-10-23T16:22:25Z

Thank you, @0x-r4bbit and @s-tikhomirov!

richard-ramos force-pushed the feat/membership branch 5 times, most recently from ecf420d to 8be1af6 Compare September 5, 2024 20:33

feat: membership

f3d085d

richard-ramos force-pushed the feat/membership branch from 8be1af6 to f3d085d Compare September 5, 2024 21:18