Refresh the minimum bondable value #714

pdyraga · 2020-07-24T12:55:04Z

~~Depends on keep-network/keep-ecdsa#508~~
~~Depends on keep-network/sortition-pools#84~~

Deep inside ECDSA keep factory implementation, BondedSortitionPool was updating the minimum bondable value for each new group selection. The idea was to automatically remove from the pool operators that are no longer able to satisfy application needs about the minimum available bondable value and prevent operators with too low value griefing signer selection.

We now have two minimum bond values: one defining the minimum unbonded value the operator needs to have so that it can join and stay in the pool, and another defining the required bond value for the current selection. If the given operator has enough unbonded value to stay in the pool but it does not have enough unbonded value for the current group selection, it is skipped.

In ECDSA keep factory, sortition pool is created with a minimum bond of 20 ETH to avoid small operators joining and griefing future selections before the minimum bond is set to the right value by the application. Anyone can create a sortition pool for an application with the default minimum bond value but the application can change this value later, at any point. 20 ETH per operator should be more than enough to cover 1 BTC deposit in 3-operators group.

The default is fine for most cases but the application is expected to refresh the minimum bondable value, depending on the current situation and needs.

For tBTC, the minimum bond value proposed here is be the minimum lot size expressed in wei given the current BTC price with 150% collateralization. For such a value, operator should be able to participate in at least 3 keeps.

Here we add a function allowing to refresh the minimum bondable value given the current BTC price. The function can be called by anyone at any moment. It is recommended to call this function on tBTC initialization and after the minimum lot size update.

Deep inside ECDSA keep factory implementation, `BondedSortitionPool` was updating the minimum bondable value for each new group selection. The idea was to automatically remove from the pool operators that are no longer able to satisfy application needs about the minimum available bondable value and prevent operators with too low value griefing signer selection. We now have two minimum bond values: one defining the minimum unbonded value the operator needs to have so that it can join and stay in the pool, and another defining the required bond value for the current selection. If the given operator has enough unbonded value to stay in the pool but it does not have enough unbonded value for the current group selection, it is skipped. In ECDSA keep factory, sortition pool is created with a minimum bond of 20 ETH to avoid small operators joining and griefing future selections before the minimum bond is set to the right value by the application. Anyone can create a sortition pool for an application with the default minimum bond value but the application can change this value later, at any point. 20 ETH per operator should be more than enough to cover 1 BTC deposit in 3-operators group. The default is fine for most cases <now> but the application is expected to refresh the minimum bondable value, depending on the current situation and needs. For tBTC, the minimum bond value proposed here is be the minimum lot size expressed in wei given the current BTC price with 150% collateralization. For such a value, operator should be able to participate in at least 3 keeps, or - in extreme circumstances - hold one keep just by themselves. Here we add a function allowing to refresh the minimum bondable value given the current BTC price. The function can be called by anyone at any moment. It is recommended to call this function on tBTC initialization and after the minimum lot size update.

Shadowfiend · 2020-07-24T20:32:10Z

Is this something we would also expect TBTCSystem to call when updating the available lot sizes?

Minimum unbonded value should be updated every time minimum lot size changes. To automate this process and do not require another transaction to be executed, we automatically update the minimum unbonded value when finalizing lot size update.

The first lot size does not have to be the smaller one. When updating the minimum unbonded value, we need to find the smallest lot size from all that are currently available. Also, improved the test coverage.

pdyraga · 2020-07-27T11:38:55Z

Is this something we would also expect TBTCSystem to call when updating the available lot sizes?

Yes, I think it's a good idea. I have not originally added it because I thought not every lot sizes update needs to modify the minimum lot. On the other hand, if we do the refresh and the minimum lot size is updated, we'll save gas on not submitting another transaction. Also, this could be a good opportunity to update the minimum unbonded value given the most recent ETBBTC price. It's done - please see the last two commits.

solidity/contracts/system/KeepFactorySelection.sol

Shadowfiend · 2020-07-28T13:29:41Z

solidity/contracts/system/TBTCSystem.sol

+
+    /// @notice Returns the minimum lot size in satoshi.
+    function getMinimumLotSizeSatoshi() public view returns (uint256) {
+        uint256 minimum = lotSizesSatoshis[0];


It feels like we'd do better to ensure the lot size array is in order when we set it, rather than trying to sort it later?

Yeah, I wasn't sure if I can do that. The mandatory lot size looks to me like something we might want to pass as the very first value just for convenience but given that it's not a function we call every day, expecting lot sizes to come sorted makes sense. If you agree with this change, I am happy to update the code 🙌

Done in 5ec1459.

Shadowfiend · 2020-07-28T13:31:27Z

solidity/contracts/system/TBTCSystem.sol

+        uint256 bondRequirementWei = _fetchBitcoinPrice().mul(
+            bondRequirementSatoshis
+        );
+        keepFactorySelection.setMinimumBondableValue(bondRequirementWei);


Since we're setting this on the sortition pool, shouldn't it be a per-operator value? That is, shouldn't it be 50% rather than 150%?

The reasoning for using 150% was to set up the bar on a low level but not on the lowest level: we make sure every operator we add to the pool can create a keep only with its members if needed. Given that we talk about the minimum lot size, it sounds to me like a sane expectation especially that we need to account for ETHBTC price fluctuations. Technically, we can lower it to 50% but I am not sure if we want to. -_-

The other danger I'm seeing here is that we've really blurred the lines of responsibility on who decides what about bonds:

The threshold/member count for keeps is static, but it's managed by DepositFactory. But it's ultimately passed through Deposit to TBTCSystem and then finally to the ECDSA keep.

The math on total bond amount is done in DepositFunding.sol, but relies entirely on information that TBTCSystem has at its disposal.

The math that turns the bond amount and n/m count into a per-node bond is done in BondedECDSAKeep.

I would suggest that we:

Move n/m values into TBTCSystem's constructor and drop them from DepositFactory.

Stop passing these through DepositFactory and Deposit.

Have requestNewKeep pass those along to BondedECDSAKeepFactory.

Calculate the initial bond requirement that is passed to BondedECDSAKeepFactory entirely in TBTCSystem, rather than doing it in Deposit and passing that info along.

Reuse the n/m info and initial bond requirement calculation function in the implementation of refreshMinimumBondableValue.

EDIT: one more bullet. In a perfect world, we would pass the initial required bond + the n/m info to BondedECDSAKeepFactory and let it decide what the implications are on minimum bondable value.

The whole point here is that the minimum bondable value is as much as possible an implementation detail of BondedECDSAKeepFactory, the Keep setup is as much as possible an implementation detail of TBTCSystem, and the deposit and deposit factory are entirely concerned with the deposit's overall collateralization info.

EDIT: one more bullet. In a perfect world, we would pass the initial required bond + the n/m info to BondedECDSAKeepFactory and let it decide what the implications are on minimum bondable value.

The whole point here is that the minimum bondable value is as much as possible an implementation detail of BondedECDSAKeepFactory, the Keep setup is as much as possible an implementation detail of TBTCSystem, and the deposit and deposit factory are entirely concerned with the deposit's overall collateralization info.

I am no sure if I understand correctly what you say but I think this is ~ what's happening now. TBTCSystem is calculating the bond required for the whole keep and BondedECDSAKeepFactory splits this bond between members.

tBTC needs to be aware of the minimum unbonded value because the refresh needs to happen outside of the function opening a keep. In the previous approach we were updating the minimum unbonded value based on the bonding parameter passed to BondedECDSAKeepFactory when opening a keep but it doesn't work well.

I think there is something I am missing here... 🤔

Taking this discussion offline briefly, will summarize the outcome.

We decided to allow operators in a sortition pool who can support only one keep (one member in one keep to be precise). For this reason, we decided for setMinimumBondValue(minimumBondValue, keepThreshold, keepSize), in the BondedECDSAKeepFactory that will basically take that and divide minimumBondValue by keepSize to determine the correct value to set on the sortition pool. This lets the factory be the center of determining how it spreads bonds across operators (which it already is when opening a new keep), while passing along the full info needed for the final setting in the sortition pool.

keep-ecdsa change is here: keep-network/keep-ecdsa#510

keep-network/keep-ecdsa#510 has been merged and I updated the code in this PR in 56ae488

By expecting the lot size array to be sorted we can simplify the code by not requiring to iterate over the array to determine minimum and maximum lot size.

The new version of keep-ecdsa allows refreshing the minimum unbonded value expected from operator per application.

This way, we do not need to pass those values around from DepositFactory to DepositFunding and Deposit, and have them eventually passed to TBTCSystem. Group size and threshold are global, system parameters so it does make sense to keep them on a global level. Also, this change will help us clean up bond calculation code (more to come in the next commits).

Instead of having the same code in two places - TBTCSystem and DepositFunding, we now calculate the required bond only in TBTCSystem. Instead of passing the bond value to requestNewKeep function, DepositFunding passes the lot size and TBTCSystem does all the calculations.

requestNewKeep reverts the transaction if lot size it not supported and although those tests are passing without a valid lot size - they revert TX before lot size is validated - it's better to use a valid lot size for clarity.

pdyraga · 2020-07-29T13:44:42Z

I marked it as a ready for review, it's ready for another chance @Shadowfiend.

solidity/contracts/system/KeepFactorySelection.sol

solidity/contracts/system/TBTCSystem.sol

Shadowfiend · 2020-07-29T13:51:50Z

solidity/contracts/system/TBTCSystem.sol

-        uint256 _m,
-        uint256 _n,
-        uint256 _bond,
+        uint64 _lotSizeSatoshis,


Well now I feel weird about this taking the lot size 🤔

I think it's a marked improvement though.

Hmm, it feels quite natural to me but I don't have the full context. Deposit is initialized with the lot size and TBTCSystem does want to work with supported lot sizes. The moment we open a keep is where lot sizes ends and wei bonds come into play.

It feels like by the time we reach the concept of a keep, we should be requesting the keep with a particular bond for the keep, rather than passing the lot size. But I like that the price feed and bond calculation is here now. So I'm still a little ways away from figuring out what's bothering me about it :) Regardless, I don't think it's worth doing anything more here for now.

solidity/contracts/system/TBTCSystem.sol

Having it public leaks the pricing info.

Shadowfiend · 2020-07-29T14:12:22Z

solidity/contracts/system/TBTCSystem.sol

@@ -638,6 +625,19 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog {
        return false;
    }

+    /// @notice Calculates bond requirement in wei for the given lot size in
+    /// satoshis given the current BTCETH price.


Super tiny nit that I'm not going to block merge on, but I would say “based on the current ETHBTC price”, mostly because we're using “given” earlier to refer specifically to something that is passed in.

Wait wait, I can edit it 😆

We now say “... based on the current ETHBTC price”, because we're using “given” earlier to refer specifically to something that is passed in.

Shadowfiend · 2020-07-29T14:19:17Z

solidity/contracts/system/TBTCSystem.sol

@@ -626,7 +626,7 @@ contract TBTCSystem is Ownable, ITBTCSystem, DepositLog {
    }

    /// @notice Calculates bond requirement in wei for the given lot size in
-    /// satoshis given the current BTCETH price.
+    /// satoshis based on the current ETHBTC price.


We now leave the decision to BondedECDSAKeepFactory how to distribute the bond expectation between members.

pdyraga · 2020-07-30T16:47:35Z

Ready for another chance @Shadowfiend 🙌

Shadowfiend

Changes look good here.

pdyraga requested review from Shadowfiend and NicholasDotSol July 24, 2020 12:55

pdyraga added 2 commits July 27, 2020 12:12

Use the minimum lot size for the minimum unbonded value calculation

7918a7a

The first lot size does not have to be the smaller one. When updating the minimum unbonded value, we need to find the smallest lot size from all that are currently available. Also, improved the test coverage.

Shadowfiend self-assigned this Jul 27, 2020

Shadowfiend reviewed Jul 28, 2020

View reviewed changes

solidity/contracts/system/KeepFactorySelection.sol Outdated Show resolved Hide resolved

Shadowfiend reviewed Jul 28, 2020

View reviewed changes

pdyraga added 6 commits July 29, 2020 11:40

Expect the lot size array to be sorted

5ec1459

By expecting the lot size array to be sorted we can simplify the code by not requiring to iterate over the array to determine minimum and maximum lot size.

Updated keep-ecdsa dependency to the most recent version

a10d285

The new version of keep-ecdsa allows refreshing the minimum unbonded value expected from operator per application.

Use correct lot sizes in tests

2e4038f

requestNewKeep reverts the transaction if lot size it not supported and although those tests are passing without a valid lot size - they revert TX before lot size is validated - it's better to use a valid lot size for clarity.

Merge branch 'master' into the-minimum-bond

cf69258

pdyraga marked this pull request as ready for review July 29, 2020 13:44

Shadowfiend reviewed Jul 29, 2020

View reviewed changes

solidity/contracts/system/KeepFactorySelection.sol Show resolved Hide resolved

Shadowfiend reviewed Jul 29, 2020

View reviewed changes

solidity/contracts/system/TBTCSystem.sol Outdated Show resolved Hide resolved

Shadowfiend reviewed Jul 29, 2020

View reviewed changes

solidity/contracts/system/TBTCSystem.sol Outdated Show resolved Hide resolved

pdyraga added 3 commits July 29, 2020 15:55

getMinLotSize() renamed to getMinimumLotSize()

1e6f84a

getMaxLotSize() renamed to getMaximumLotSize()

1272e40

calculateBondRequirementWei made internal

3e56a08

Having it public leaks the pricing info.

Shadowfiend reviewed Jul 29, 2020

View reviewed changes

Improved the documentation of TBTCSystem.calculateBondRequirementWei

5eed445

We now say “... based on the current ETHBTC price”, because we're using “given” earlier to refer specifically to something that is passed in.

Shadowfiend reviewed Jul 29, 2020

View reviewed changes

Updated keep-ecdsa dependency to the most recent version

d611483

pdyraga added 2 commits July 30, 2020 17:22

Pass group size and threshold to setMinimumBondableValue

56ae488

We now leave the decision to BondedECDSAKeepFactory how to distribute the bond expectation between members.

Merge branch 'master' into the-minimum-bond

7f343d9

Shadowfiend approved these changes Jul 30, 2020

View reviewed changes

Shadowfiend merged commit 7a2cb12 into master Jul 30, 2020

Shadowfiend deleted the the-minimum-bond branch July 30, 2020 19:07

Shadowfiend added this to the v1.1.0 milestone Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh the minimum bondable value #714

Refresh the minimum bondable value #714

pdyraga commented Jul 24, 2020 •

edited by Shadowfiend

Loading

Shadowfiend commented Jul 24, 2020

pdyraga commented Jul 27, 2020

Shadowfiend Jul 28, 2020

pdyraga Jul 28, 2020 •

edited

Loading

pdyraga Jul 29, 2020

Shadowfiend Jul 28, 2020

pdyraga Jul 28, 2020

Shadowfiend Jul 28, 2020 •

edited

Loading

pdyraga Jul 29, 2020

Shadowfiend Jul 29, 2020

pdyraga Jul 29, 2020

pdyraga Jul 30, 2020

pdyraga commented Jul 29, 2020

Shadowfiend Jul 29, 2020

pdyraga Jul 29, 2020

Shadowfiend Jul 29, 2020

Shadowfiend Jul 29, 2020

pdyraga Jul 29, 2020

Shadowfiend Jul 29, 2020

pdyraga commented Jul 30, 2020

Shadowfiend left a comment

Refresh the minimum bondable value #714

Refresh the minimum bondable value #714

Conversation

pdyraga commented Jul 24, 2020 • edited by Shadowfiend Loading

Shadowfiend commented Jul 24, 2020

pdyraga commented Jul 27, 2020

Choose a reason for hiding this comment

pdyraga Jul 28, 2020 • edited Loading

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

Shadowfiend Jul 28, 2020 • edited Loading

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

pdyraga commented Jul 29, 2020

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

pdyraga commented Jul 30, 2020

Shadowfiend left a comment

Choose a reason for hiding this comment

pdyraga commented Jul 24, 2020 •

edited by Shadowfiend

Loading

pdyraga Jul 28, 2020 •

edited

Loading

Shadowfiend Jul 28, 2020 •

edited

Loading