From 4f15c694a89091e686387cee14d16ee0b46afdb8 Mon Sep 17 00:00:00 2001 From: apenzk Date: Sun, 2 Mar 2025 10:42:10 +0100 Subject: [PATCH] use time for acceptor terms, not blocks --- .../mcr/contracts/src/settlement/MCR.sol | 59 ++++----- .../contracts/src/settlement/MCRStorage.sol | 9 +- .../mcr/contracts/test/settlement/MCR.sol | 122 ++++++++---------- 3 files changed, 87 insertions(+), 103 deletions(-) diff --git a/protocol-units/settlement/mcr/contracts/src/settlement/MCR.sol b/protocol-units/settlement/mcr/contracts/src/settlement/MCR.sol index 372b7c263..5aa2a142c 100644 --- a/protocol-units/settlement/mcr/contracts/src/settlement/MCR.sol +++ b/protocol-units/settlement/mcr/contracts/src/settlement/MCR.sol @@ -19,10 +19,10 @@ contract MCR is Initializable, BaseSettlement, MCRStorage, IMCR { bytes32 public constant TRUSTED_ATTESTER = keccak256("TRUSTED_ATTESTER"); /// @notice Error thrown when acceptor term is greater than 256 blocks - error AcceptorTermTooLong(); + error AcceptorDurationTooLong(); /// @notice Error thrown when acceptor term is too large for epoch duration - error AcceptorTermTooLongForEpoch(); + error AcceptorDurationTooLongForEpoch(); /// @notice Error thrown when minimum commitment age is greater than epoch duration error MinCommitmentAgeTooLong(); @@ -31,25 +31,20 @@ contract MCR is Initializable, BaseSettlement, MCRStorage, IMCR { error MaxAcceptorNonReactivityTimeTooLong(); /// @notice Sets the acceptor term duration, must be less than epoch duration - /// @param _acceptorTerm New acceptor term duration in time units - function setAcceptorTerm(uint256 _acceptorTerm) public onlyRole(COMMITMENT_ADMIN) { - // Ensure acceptor term is not longer than 256 blocks - if (_acceptorTerm > 256) { - revert AcceptorTermTooLong(); - } + /// @param _acceptorDuration New acceptor term duration in time units + function setAcceptorDuration(uint256 _acceptorDuration) public onlyRole(COMMITMENT_ADMIN) { // Ensure acceptor term is sufficiently small compared to epoch duration uint256 epochDuration = stakingContract.getEpochDuration(address(this)); // TODO If we would use block heights instead of timestamps we could handle everything much smoother. - uint256 estimatedL1BlockDelta = 12 seconds; - if (2 * _acceptorTerm >= epochDuration / estimatedL1BlockDelta) { - revert AcceptorTermTooLongForEpoch(); + if (2 * _acceptorDuration >= epochDuration ) { + revert AcceptorDurationTooLongForEpoch(); } - acceptorTerm = _acceptorTerm; + acceptorDuration = _acceptorDuration; } - function getAcceptorTerm() public view returns (uint256) { - return acceptorTerm; + function getAcceptorDuration() public view returns (uint256) { + return acceptorDuration; } /// @notice Sets the minimum time that must pass before a commitment can be postconfirmed @@ -91,7 +86,7 @@ contract MCR is Initializable, BaseSettlement, MCRStorage, IMCR { uint256 _leadingSuperBlockTolerance, uint256 _epochDuration, // in time units address[] memory _custodians, - uint256 _acceptorTerm, // in time units + uint256 _acceptorDuration, // in time units address _moveTokenAddress // the primary custodian for rewards in the staking contract ) public initializer { __BaseSettlement_init_unchained(); @@ -101,7 +96,7 @@ contract MCR is Initializable, BaseSettlement, MCRStorage, IMCR { stakingContract.registerDomain(_epochDuration, _custodians); grantCommitmentAdmin(msg.sender); grantTrustedAttester(msg.sender); - acceptorTerm = _acceptorTerm; + acceptorDuration = _acceptorDuration; moveTokenAddress = _moveTokenAddress; // Set default values to 1/10th of epoch duration @@ -391,26 +386,32 @@ contract MCR is Initializable, BaseSettlement, MCRStorage, IMCR { return true; } - /// @notice Gets the block height at which the current acceptor's term started - function getAcceptorStartL1BlockHeight(uint256 currentL1Block) public view returns (uint256) { - uint256 currentL1BlockCorrected = currentL1Block - 1; // The first block is 1, not 0 - return currentL1BlockCorrected - (currentL1BlockCorrected % acceptorTerm) + 1; + /// @notice Gets the time at which the current epoch started + function getEpochStartTime() public view returns (uint256) { + uint256 currentTime = block.timestamp; + return currentTime - (currentTime % stakingContract.getEpochDuration(address(this))); + } + + /// @notice Gets the time at which the current acceptor's term started + function getAcceptorStartTime() public view returns (uint256) { + uint256 currentTime = block.timestamp; + // We reset the times to match the start of epochs. This ensures every epoch has the same setup. + uint256 currentTimeCorrected = currentTime % stakingContract.getEpochDuration(address(this)); + return currentTimeCorrected - (currentTimeCorrected % acceptorDuration); } /// @notice Determines the acceptor in the accepting epoch using L1 block hash as a source of randomness // At the border between epochs this is not ideal as getAcceptor works on blocks and epochs works with time. // Thus we must consider the edge cases where the acceptor is only active for a short time. function getAcceptor() public view returns (address) { - uint256 currentL1Block = block.number; - uint256 acceptorStartL1Block = getAcceptorStartL1BlockHeight(currentL1Block); - require(acceptorStartL1Block > 0, "Acceptor start block should not be 0"); - require(acceptorStartL1Block <= currentL1Block, "Acceptor start block is in the future"); - require(currentL1Block - acceptorStartL1Block <= 256, "Acceptor start block is too old, as data is not available for more than 256 blocks"); - bytes32 randomness = blockhash(acceptorStartL1Block-1); - require(randomness != 0, "Block too old for randomness"); + // TODO unless we swap with everything, including epochs, we must use block.timestamp. + // However, to get easy access to L1 randomness we need to know the block at which the acceptor started, which is expensive (unless we count in blocks instead of time) + // TODO as long as we do not swap to block.number, the randomness below is precictable. + uint256 randSeed1 = getAcceptorStartTime(); + uint256 randSeed2 = getEpochStartTime(); address[] memory attesters = stakingContract.getStakedAttestersForAcceptingEpoch(address(this)); - uint256 acceptorIndex = uint256(randomness) % attesters.length; - return attesters[acceptorIndex]; + uint256 acceptorIndex = uint256(keccak256(abi.encodePacked(randSeed1, randSeed2))) % attesters.length; // randomize the order of the attesters + return attesters[acceptorIndex]; } /// @dev it is possible if the accepting epoch is behind the presentEpoch that heights dont obtain enough votes in the assigned epoch. diff --git a/protocol-units/settlement/mcr/contracts/src/settlement/MCRStorage.sol b/protocol-units/settlement/mcr/contracts/src/settlement/MCRStorage.sol index dc2d81637..43c73bc5d 100644 --- a/protocol-units/settlement/mcr/contracts/src/settlement/MCRStorage.sol +++ b/protocol-units/settlement/mcr/contracts/src/settlement/MCRStorage.sol @@ -25,13 +25,10 @@ contract MCRStorage { // track the last postconfirmed superBlock height, so that we can require superBlocks are submitted in order and handle staking effectively uint256 public lastPostconfirmedSuperBlockHeight; - /// Acceptor term time in seconds (determined by L1 blocks). The confimer remains the same for acceptorTerm period. - // This means we accept that if the acceptor is not active the postconfirmations will be delayed. - // TODO permit that anyone can confirm but only the Acceptor gets rewarded. - // TODO The Acceptor should also get rewarded even if another attestor confirmed the postconfirmation. - // The Acceptor term can be minimal, but it should not be O(1) as the acceptor should have some time + /// Acceptor term time in seconds. The acceptor remains the same for acceptorDuration period. + // The Acceptor term can be minimal, but it should not be too small as the acceptor should have some time // to prepare and post L1-transactions that will start the validation of attestations. - uint256 public acceptorTerm; + uint256 public acceptorDuration; /// @notice Minimum time that must pass before a commitment can be postconfirmed uint256 public minCommitmentAgeForPostconfirmation; diff --git a/protocol-units/settlement/mcr/contracts/test/settlement/MCR.sol b/protocol-units/settlement/mcr/contracts/test/settlement/MCR.sol index 1dff85ea9..5b898c2e7 100644 --- a/protocol-units/settlement/mcr/contracts/test/settlement/MCR.sol +++ b/protocol-units/settlement/mcr/contracts/test/settlement/MCR.sol @@ -21,7 +21,7 @@ contract MCRTest is Test, IMCR { string public stakingSignature = "initialize(address)"; string public mcrSignature = "initialize(address,uint256,uint256,uint256,address[],uint256,address)"; uint256 epochDuration = 7200 seconds; - uint256 acceptorTerm = epochDuration/12 seconds/4; + uint256 acceptorDuration = epochDuration/12 seconds/4; bytes32 honestCommitmentTemplate = keccak256(abi.encodePacked(uint256(1), uint256(2), uint256(3))); bytes32 honestBlockIdTemplate = keccak256(abi.encodePacked(uint256(1), uint256(2), uint256(3))); bytes32 dishonestCommitmentTemplate = keccak256(abi.encodePacked(uint256(3), uint256(2), uint256(1))); @@ -88,7 +88,7 @@ contract MCRTest is Test, IMCR { 5, // _leadingSuperBlockTolerance, max blocks ahead of last confirmed epochDuration, // _epochDuration, how long an epoch lasts, constant stakes in that time custodians, // _custodians, array with moveProxy address - acceptorTerm, // _acceptorTerm, how long an acceptor serves + acceptorDuration, // _acceptorDuration, how long an acceptor serves // TODO can we replace the following line with the moveToken address? address(moveProxy) // _moveTokenAddress, the primary custodian for rewards in the staking contract ); @@ -321,7 +321,7 @@ contract MCRTest is Test, IMCR { } // ---------------------------------------------------------------- - // -------- Test functions ---------------------------------------- + // -------- General tests ---------------------------------------- // ---------------------------------------------------------------- function testCannotInitializeTwice() public { @@ -771,8 +771,6 @@ contract MCRTest is Test, IMCR { assertEq(mcr.getLastPostconfirmedSuperBlockHeight(), 1, "Last postconfirmed superblock height should be 1, as supermajority was reached (2/2 > threshold)"); } - - function testSetMinCommitmentAge() public { // Set min commitment age to a too long value vm.expectRevert(MCR.MinCommitmentAgeTooLong.selector); @@ -808,82 +806,81 @@ contract MCRTest is Test, IMCR { // ---------------------------------------------------------------- - // -------- Acceptor -------------------------------------- + // -------- Acceptor tests -------------------------------------- // ---------------------------------------------------------------- /// @notice Test that getAcceptorStartTime correctly calculates term start times - function testAcceptorStartL1BlockHeight() public { + function testAcceptorStartTime() public { // Test at block 0 - assertEq(mcr.getAcceptorStartL1BlockHeight(block.number), 1, "Acceptor term should start at L1Block 1"); + assertEq(block.timestamp, 1, "Current time should be 1"); // TODO why is it 1? and not 0? + assertEq(acceptorDuration, mcr.getAcceptorDuration(), "Acceptor term should be correctly set"); + assertEq(mcr.getAcceptorStartTime(), 0, "Acceptor term should start at (1) time 0"); // Test at half an acceptor term - vm.roll(acceptorTerm/2); - assertEq(mcr.getAcceptorStartL1BlockHeight(block.number), 1, "Acceptor term should start at L1Block 1"); + vm.warp(acceptorDuration-1); + assertEq(mcr.getAcceptorStartTime(), 0, "Acceptor term should start at (2) time 0"); // Test at an acceptor term boundary - vm.roll(acceptorTerm); - assertEq(mcr.getAcceptorStartL1BlockHeight(block.number), 1, "Acceptor term should start at L1Block 1"); + vm.warp(acceptorDuration); + console.log("current time", block.timestamp); + console.log("acceptorDuration", acceptorDuration); + console.log("epochTime", epochDuration); + assertEq(mcr.getAcceptorStartTime(), acceptorDuration, "Acceptor term should start at (3) time acceptorDuration"); // Test at an acceptor term boundary - vm.roll(acceptorTerm+1); - assertEq(mcr.getAcceptorStartL1BlockHeight(block.number), acceptorTerm+1, "Acceptor term should start at L1Block acceptorTerm+1"); + vm.warp(acceptorDuration+1); + assertEq(mcr.getAcceptorStartTime(), acceptorDuration, "Acceptor term should start at (4) time acceptorDuration"); // Test at 1.5 acceptor terms - vm.warp(3 * acceptorTerm / 2); - assertEq(mcr.getAcceptorStartL1BlockHeight(block.number), acceptorTerm+1, "Acceptor term should start at L1Block acceptorTerm+1"); + vm.warp(2 * acceptorDuration ); + assertEq(mcr.getAcceptorStartTime(), 2 * acceptorDuration, "Acceptor term should start at (5) time 2 * acceptorDuration"); } - /// @notice Test setting acceptor term with validation - function testSetAcceptorTerm() public { - // Ensure we can retrieve the epoch duration correct - assertEq(epochDuration, staking.getEpochDuration(address(mcr))); - - // Set acceptor term to 256 blocks (should succeed) - mcr.setAcceptorTerm(256); - assertEq(mcr.acceptorTerm(), 256, "Term should be updated to 256"); - - // Try setting acceptor term to over 256 blocks (should fail) - vm.expectRevert(MCR.AcceptorTermTooLong.selector); - mcr.setAcceptorTerm(257); - assertEq(mcr.acceptorTerm(), 256, "Term should remain at 256"); - - // Check validity with respect to epoch duration - uint256 validTerm = epochDuration/12 seconds/4; - mcr.setAcceptorTerm(validTerm); - assertEq(mcr.acceptorTerm(), validTerm, "Term should be updated to epoch related value"); - - // Try setting acceptor term to epoch duration - uint256 invalidTerm = epochDuration/12 seconds; - vm.expectRevert(MCR.AcceptorTermTooLong.selector); - mcr.setAcceptorTerm(invalidTerm); - assertEq(mcr.acceptorTerm(), validTerm, "Term should remain at epoch related value"); + /// @notice Test setting acceptor duration with validation + function testSetAcceptorDuration() public { + // Check the epoch duration is set correctly + assertEq(epochDuration, staking.getEpochDuration(address(mcr))); + // Test valid duration (less than half epoch duration) + uint256 validDuration = epochDuration / 2 - 1; + mcr.setAcceptorDuration(validDuration); + assertEq(mcr.getAcceptorDuration(), validDuration, "Duration should be updated to valid value"); + + // Test duration too long compared to epoch (>= epochDuration/2) + uint256 invalidDuration = epochDuration / 2; + vm.expectRevert(MCR.AcceptorDurationTooLongForEpoch.selector); + mcr.setAcceptorDuration(invalidDuration); + assertEq(mcr.getAcceptorDuration(), validDuration, "Duration should remain at previous valid value"); + + // Test duration equal to epoch duration (should fail) + vm.expectRevert(MCR.AcceptorDurationTooLongForEpoch.selector); + mcr.setAcceptorDuration(epochDuration); + assertEq(mcr.getAcceptorDuration(), validDuration, "Duration should remain at previous valid value"); } /// @notice Test that getAcceptor correctly selects an acceptor based on block hash function testGetAcceptor() public { // Setup with three attesters with equal stakes - (address alice, , address carol) = setupGenesisWithThreeAttesters(1, 1, 1); + (, address bob, address carol) = setupGenesisWithThreeAttesters(1, 1, 1); + uint256 myAcceptorDuration = 13; + mcr.setAcceptorDuration(myAcceptorDuration); + assertEq(myAcceptorDuration,mcr.getAcceptorDuration(),"Acceptor duration not set correctly"); - uint256 myAcceptorTerm = 4; - mcr.setAcceptorTerm(myAcceptorTerm); address initialAcceptor = mcr.getAcceptor(); - assertTrue( initialAcceptor == carol, "Acceptor should be Carol"); + assertEq(initialAcceptor, bob, "Acceptor should be bob"); - vm.roll(2); // we started at block 1 - assertEq(mcr.getAcceptor(), initialAcceptor, "Acceptor should not change within term"); - - vm.roll(myAcceptorTerm); // L1blocks started at 1, not 0 + vm.warp(myAcceptorDuration-1); assertEq(mcr.getAcceptor(), initialAcceptor, "Acceptor should not change within term"); - // Move to next acceptor Term - vm.roll(myAcceptorTerm+1); // L1blocks started at 1, not 0 + // Move two acceptor terms (moving one resulted still in bob as acceptor with current randomness) + vm.warp(2*myAcceptorDuration); address newAcceptor = mcr.getAcceptor(); - assertTrue( newAcceptor == alice, "New acceptor should be Alice"); + assertEq(mcr.getAcceptorStartTime(),2*myAcceptorDuration,"Acceptor start time should be myAcceptorDuration"); + assertEq(newAcceptor, carol, "New acceptor should be Carol"); } // ---------------------------------------------------------------- - // -------- Attester rewards -------------------------------------- + // -------- Attester reward tests -------------------------------------- // ---------------------------------------------------------------- function testAttesterRewardPoints() public { @@ -945,17 +942,6 @@ contract MCRTest is Test, IMCR { assertEq(moveToken.balanceOf(carol), carolInitialBalance + mcr.getStakeForAcceptingEpoch(address(moveToken), carol), "Carol reward not correct."); } - /// @notice Test that the acceptor privilege window works correctly - function testAcceptorPrivilegeWindow() public { - address alice = setupGenesisWithOneAttester(1); - - // set the max acceptor non-reactivity time to 1/4 epochDuration - mcr.setAcceptorPrivilegeWindow(epochDuration/4); - assertEq(mcr.getMaxAcceptorNonReactivityTime(), epochDuration/4, "Max acceptor non-reactivity time should be 1/4 epochDuration"); - - - } - /// @notice Test that postconfirmation rewards are distributed correctly when the acceptor is live function testPostconfirmationRewardsLiveAcceptor() public { uint256 stake = 7; @@ -1050,7 +1036,7 @@ contract MCRTest is Test, IMCR { (address alice, address bob, ) = setupGenesisWithThreeAttesters(aliceStake, bobStake, 0); uint256 aliceInitialBalance = moveToken.balanceOf(alice); uint256 bobInitialBalance = moveToken.balanceOf(bob); - uint256 thisAcceptorTerm = mcr.getAcceptorTerm(); + uint256 thisAcceptorDuration = mcr.getAcceptorDuration(); // set the time windows assertEq(mcr.getMinCommitmentAgeForPostconfirmation(), 0, "Min commitment age should be 0"); @@ -1058,8 +1044,8 @@ contract MCRTest is Test, IMCR { mcr.setAcceptorPrivilegeWindow(thisAcceptorPriviledgeWindow); assertEq(mcr.getMaxAcceptorNonReactivityTime(), thisAcceptorPriviledgeWindow, "Max acceptor non-reactivity time should be 1/100 epochDuration"); console.log("getMaxAcceptorNonReactivityTime", mcr.getMaxAcceptorNonReactivityTime()); - console.log("thisAcceptorTerm", thisAcceptorTerm); - assertGt(thisAcceptorTerm, thisAcceptorPriviledgeWindow, "Acceptor term should be greater than thisAcceptorPriviledgeWindow"); + console.log("thisAcceptorDuration", thisAcceptorDuration); + assertGt(thisAcceptorDuration, thisAcceptorPriviledgeWindow, "Acceptor term should be greater than thisAcceptorPriviledgeWindow"); vm.prank(alice); mcr.submitSuperBlockCommitment(makeHonestCommitment(1)); @@ -1088,11 +1074,11 @@ contract MCRTest is Test, IMCR { } // ---------------------------------------------------------------- - // -------- Acceptor rewards -------------------------------------- + // -------- Acceptor reward tests -------------------------------------- // ---------------------------------------------------------------- - // An acceptor that is in place for acceptorTerm time should be replaced by a new acceptor after their term ended. + // An acceptor that is in place for acceptorDuration time should be replaced by a new acceptor after their term ended. // TODO reward logic is not yet implemented function testAcceptorRewards() public { (address alice, address bob, ) = setupGenesisWithThreeAttesters(1, 1, 0);