Beefing up the Coordinator: Restrictions to initiator, ritual duration, fee model, automatic TX reimbursements, etc #86

cygnusv · 2023-06-09T06:22:24Z

This is an omnibus PR that addresses several pending functionalities for the Coordinator. From a subjective order of relevance:

Introduces fee models: initially we're just introducing here a basic IFeeModel interface that allows to calculate initiation costs, but that could be later extended to manage other payment inputs. For this PR, I'm instantiating the interface with a simple FlatRateFeeModel, where ritual initiation cost is just dkg_size * duration * rate and the currency is assumed to be an ERC20 token. We also introduce duration as a parameter to initiate rituals.
Introduces an access-controlled role for initiating rituals, as well as a one-off switch to unleash ritual initiation. The proposed model works as follows:
- Initially, Coordinator contract works in permissioned way (only addresses that have the INITIATOR_ROLE can create rituals)
- This role can be granted by the Coordinator admin (uses OpenZeppelin access control model, see tests)
- Admin can switch the Coordinator from permissioned to permissionless ritual initiation; at this point, the initiator role is not relevant anymore. Note that this action can't be undone.
- Rationale for this model is that it both works for many types of deployments without changing the code:
  - It's the exact model we plan for the beta release (initially, ritual initiation is controlled by Threshold DAO but can be opened in the future)
  - For devnets and testnets, we have maximum flexibility: if we want it permissioned, we just grant initiator roles; if we want permissionless, we just call the switch.
Automatic reimbursement of costs for posting transcripts and aggregations. It uses Keep's ReimbursementPool model, currently in production with tBTCv2
Ensures that provided decryption request static key by nodes is 42 byte (courtesy of @derekpierre 🎸 )

Optimize Ritual struct layout

Also replace contract owner for parameter administration role

Initiator is just the entity that calls initiateRitual(), needs to be authorized, and provides initial payment. Authority is who controls ritual lifecycle going forward.

Fee Models dictate what's the cost for rituals

Also, changes AccessControl base contract for the more specific AccessControlDefaultAdminRules

Also, change isInitiationRegulated to isInitiationPublic

contracts/contracts/coordination/Coordinator.sol

KPrasch · 2023-06-21T14:53:02Z

contracts/contracts/coordination/Coordinator.sol

        timeout = _timeout;
        maxDkgSize = _maxDkgSize;
+        feeModel = IFeeModel(_feeModel);


Shall we permit the feeModel address to be updated by the proper role?

Yeah, it's possible, but I'd rather handle that in a later PR, when we decide how do we want to manage fee models (see comment in L71). For our immediate needs (devnets), I don't foresee we will want to change the fee model

See https://github.com/keep-network/keep-core/blame/main/solidity/random-beacon/contracts/ReimbursementPool.sol

Can't revoke INITIATOR_ROLE addresses since our AccessControl doesn't inherit from AccessControlEnumerable. It doesn't matter much anyway

We may want to parametrize tests based on this

Add also previous StakeInfo deployment on Mumbai

Either ritual is successful and fees can be used by the protocol, or ritual is failed and fees are refunded to initiator (consider partial refund)

…es long (as of nucypher-core v0.10.0).

theref

This looks great

ape-config.yaml

arjunhassard

Huge step towards v7.0.0 👏

It may be that:
(i) educating stakers on reimbursements
(ii) adding it to staker UX
(iii) maintaining & updating this section of the contracts
may be more trouble than it's worth during genesis given that they will be 'paid back' by duration-based fees within a few days).

Also, allowing permissionless ritual initiation is predicated on those rituals being far cheaper, so it's hard to know ex ante whether the reimbursements will be enough to justify the gas of withdrawal, and also exactly what the reimbursement could be bundled with and on what layer!

arjunhassard · 2023-06-26T13:22:36Z

contracts/contracts/coordination/FlateRateFeeModel.sol

+        uint256 size = providers.length;
+        require(duration > 0, "Invalid ritual duration");
+        require(size > 0, "Invalid ritual size");
+        return feeRatePerSecond * size * duration;


the proposal contains a default duration fixed at 182.5 days for initialization
and then top ups are fixed at 30 days (payable every 30 days) to keep the availability horizon 182.5 days into the future

Is this something you would expect to be hardcoded at the smart contract level?

I'm opening an issue to track top-ups #93

No not hardcoded, as ofc we may need to change these. Just wanted to note the likely genesis defaults, pending feedback. However there is a small question of trust implications post-v7.0.0 – ideally the Threshold DAO would control sponsorship parameters like these

arjunhassard · 2023-06-26T13:25:33Z

contracts/contracts/coordination/FlateRateFeeModel.sol

+        stakes = IAccessControlApplication(_stakes);
+    }
+
+    function getRitualInitiationCost(


Since the 'paywall' here appears to be generating a public key for the first time, I'm wondering how a sponsor tops-up the availability horizon without a new ritual, and also if once they are conferred theINITIATOR_ROLE, they can do this unilaterally and whenever they want

Good questions. Let's track these here #93

derekpierre

Presumably we'll need respective nucypher changes (CoordinatorAgent etc.) before mergining into main.

derekpierre · 2023-06-26T20:06:31Z

contracts/contracts/coordination/Coordinator.sol


        // TODO: Include cohort fingerprint in StartRitual event?
-        emit StartRitual(id, msg.sender, providers);
+        emit StartRitual(id, ritual.authority, providers);


StartRitual's 2nd parameter is indexed as "initiator":

Should this be ritual.initiator?

Should both initiator and authority be in the event?

Good catch. I meant authority here. IMO, once we introduce the authority role, the initiator is nothing more than a sponsor so there's in principle not much interest on it. In fact, once the fee is fully processed (so there's no refund), this role is not needed anymore. I would lean towards just including the authority in the event.

contracts/contracts/coordination/Coordinator.sol

vzotova · 2023-06-26T20:59:27Z

contracts/contracts/coordination/Coordinator.sol

-        ritual.initiator = msg.sender;  // TODO: Consider sponsor model
-        ritual.dkgSize = uint32(length);
+        ritual.initiator = msg.sender;
+        ritual.authority = authority;


Could be authority zero address?

contracts/contracts/coordination/Coordinator.sol

KPrasch

Need a matching PR into nucypher/nucypher but looks good 👍🏻

ape-config.yaml

derekpierre

🎸

cygnusv added 8 commits June 9, 2023 08:04

Introduce duration parameter for ritual creation

41f1634

Optimize Ritual struct layout

Introduce Initiator role to gatekeep ritual initiation

e56c3ef

Also replace contract owner for parameter administration role

Add authority field to Ritual. Can be different than initiator

f24fd29

Initiator is just the entity that calls initiateRitual(), needs to be authorized, and provides initial payment. Authority is who controls ritual lifecycle going forward.

Introduce interface for Fee Models

deb7fcf

Fee Models dictate what's the cost for rituals

Basic FlateRateFeeModel

b8492bf

EndRitual event only needs to track ritualID and the outcome

7ccae5b

Use DEFAULT_ADMIN_ROLE as parameters admin

66e1f64

Also, changes AccessControl base contract for the more specific AccessControlDefaultAdminRules

Function to make initiation public (no initiator allowlist)

64792c1

Also, change isInitiationRegulated to isInitiationPublic

cygnusv force-pushed the initiator branch from 5c65614 to f31cef5 Compare June 14, 2023 13:26

This was referenced Jun 14, 2023

Implement Base Fee nucypher/nucypher#3146

Closed

Permissionless ritual initialization leads to spending attack on nodes nucypher/nucypher#3102

Closed

cygnusv force-pushed the initiator branch from f31cef5 to 923da74 Compare June 16, 2023 08:46

KPrasch reviewed Jun 21, 2023

View reviewed changes

contracts/contracts/coordination/Coordinator.sol Show resolved Hide resolved

KPrasch mentioned this pull request Jun 21, 2023

TACo Encryption Allow Logic Interfaces #89

Merged

KPrasch reviewed Jun 21, 2023

View reviewed changes

cygnusv added 3 commits June 22, 2023 08:10

Introduce feeModel parameter and use it to charge the proper ritual cost

d13d287

Automatic reimbursing of ritual costs via ReimbursementPool

ddb0847

See https://github.com/keep-network/keep-core/blame/main/solidity/random-beacon/contracts/ReimbursementPool.sol

Assorted fixes

f11e8c4

Can't revoke INITIATOR_ROLE addresses since our AccessControl doesn't inherit from AccessControlEnumerable. It doesn't matter much anyway

cygnusv force-pushed the initiator branch from 9c2a9ab to ab7478e Compare June 22, 2023 06:13

cygnusv and others added 9 commits June 23, 2023 13:01

Bump OZ dependency to v4.9.1

486b893

Make transcript size a test constant

cf9e74a

We may want to parametrize tests based on this

Deployment script for FlatRateFeeModel

55fa421

Add DAI deployment address for Polygon Mainnet and Mumbai

a896ed0

Add also previous StakeInfo deployment on Mumbai

Add stakes reference to Fee Model contracts

85cc9c2

Single finalization logic when posting aggregations

d634394

Treat fees as pending until ritual is final

02b93ed

Either ritual is successful and fees can be used by the protocol, or ritual is failed and fees are refunded to initiator (consider partial refund)

Ensure that provided decryption request static key by nodes is 42 byt…

f819fac

…es long (as of nucypher-core v0.10.0).

Adapt Coordinator tests to latest additions

bc11d12

cygnusv force-pushed the initiator branch from 96e3914 to bc11d12 Compare June 23, 2023 11:01

cygnusv changed the title ~~Initiator, ritual duration, fee model, etc~~ Beefing up the Coordinator: Restrictions to initiator, ritual duration, fee model, automatic TX reimbursements, etc Jun 23, 2023

cygnusv marked this pull request as ready for review June 23, 2023 11:33

cygnusv requested review from KPrasch, theref, derekpierre, vzotova, manumonti, arjunhassard, jMyles and piotr-roslaniec June 23, 2023 11:33

theref approved these changes Jun 23, 2023

View reviewed changes

ape-config.yaml Show resolved Hide resolved

piotr-roslaniec mentioned this pull request Jun 26, 2023

Support increased number of nodes in coordinator tests #90

Merged

piotr-roslaniec approved these changes Jun 26, 2023

View reviewed changes

arjunhassard approved these changes Jun 26, 2023

View reviewed changes

derekpierre reviewed Jun 26, 2023

View reviewed changes

vzotova reviewed Jun 26, 2023

View reviewed changes

contracts/contracts/coordination/Coordinator.sol Outdated Show resolved Hide resolved

vzotova reviewed Jun 26, 2023

View reviewed changes

contracts/contracts/coordination/Coordinator.sol Show resolved Hide resolved

vzotova approved these changes Jun 26, 2023

View reviewed changes

Use SafeERC20 to interact with ERC20 tokens in Coordinator

362891e

cygnusv mentioned this pull request Jun 29, 2023

Top-up ritual to extend duration #93

Open

KPrasch approved these changes Jun 29, 2023

View reviewed changes

ape-config.yaml Show resolved Hide resolved

Fixes StartRitual event. Simplifies ReimbursementPool setter.

b2eb840

cygnusv force-pushed the initiator branch from 15b844a to b2eb840 Compare June 29, 2023 08:55

derekpierre approved these changes Jun 29, 2023

View reviewed changes

cygnusv changed the base branch from main to development June 29, 2023 15:13

cygnusv merged commit d6f07bd into nucypher:development Jun 29, 2023
2 checks passed

piotr-roslaniec mentioned this pull request Jun 30, 2023

Fix conftest.py attempting to use incorrect openzeppelin #94

Merged

derekpierre mentioned this pull request Sep 20, 2023

v7.0.0 Major feature freeze nucypher/nucypher#3200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beefing up the Coordinator: Restrictions to initiator, ritual duration, fee model, automatic TX reimbursements, etc #86

Beefing up the Coordinator: Restrictions to initiator, ritual duration, fee model, automatic TX reimbursements, etc #86

cygnusv commented Jun 9, 2023 •

edited

Loading

KPrasch Jun 21, 2023

cygnusv Jun 23, 2023

theref left a comment

arjunhassard left a comment

arjunhassard Jun 26, 2023

cygnusv Jun 29, 2023

cygnusv Jun 29, 2023

arjunhassard Jun 30, 2023

arjunhassard Jun 26, 2023

cygnusv Jun 29, 2023

derekpierre left a comment

derekpierre Jun 26, 2023

cygnusv Jun 29, 2023

vzotova Jun 26, 2023

KPrasch left a comment

derekpierre left a comment

Beefing up the Coordinator: Restrictions to initiator, ritual duration, fee model, automatic TX reimbursements, etc #86

Beefing up the Coordinator: Restrictions to initiator, ritual duration, fee model, automatic TX reimbursements, etc #86

Conversation

cygnusv commented Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theref left a comment

Choose a reason for hiding this comment

arjunhassard 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

derekpierre 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

KPrasch left a comment

Choose a reason for hiding this comment

derekpierre left a comment

Choose a reason for hiding this comment

cygnusv commented Jun 9, 2023 •

edited

Loading