From dc6327cbf007235621520d39baed926a0c30e3d1 Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 16 Apr 2024 11:09:06 +0800 Subject: [PATCH 01/10] feat: conditional osp --- src/challenge/ChallengeManager.sol | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/challenge/ChallengeManager.sol b/src/challenge/ChallengeManager.sol index c5427e7ea..14acba8f4 100644 --- a/src/challenge/ChallengeManager.sol +++ b/src/challenge/ChallengeManager.sol @@ -35,6 +35,7 @@ contract ChallengeManager is DelegateCallAware, IChallengeManager { ISequencerInbox public sequencerInbox; IBridge public bridge; IOneStepProofEntry public osp; + mapping(bytes32 => IOneStepProofEntry) public ospCond; function challengeInfo(uint64 challengeIndex) external @@ -116,6 +117,20 @@ contract ChallengeManager is DelegateCallAware, IChallengeManager { osp = osp_; } + function setConditionalOsp(bytes32 wasmModuleRoot, IOneStepProofEntry osp_) external onlyDelegated onlyProxyOwner { + emit ConditonalOSPSet(wasmModuleRoot, osp_); + ospCond[wasmModuleRoot] = osp_; + } + + function getOSP(bytes32 wasmModuleRoot) internal view returns (IOneStepProofEntry) { + IOneStepProofEntry t = ospCond[wasmModuleRoot]; + if (address(t) == address(0)){ + return osp; + } else { + return t; + } + } + function createChallenge( bytes32 wasmModuleRoot_, MachineStatus[2] calldata startAndEndMachineStatuses_, @@ -259,7 +274,7 @@ contract ChallengeManager is DelegateCallAware, IChallengeManager { require(challengeLength == 1, "TOO_LONG"); } - bytes32 afterHash = osp.proveOneStep( + bytes32 afterHash = getOSP(challenge.wasmModuleRoot).proveOneStep( ExecutionContext({maxInboxMessagesRead: challenge.maxInboxMessages, bridge: bridge}), challengeStart, selection.oldSegments[selection.challengePosition], From b6d60e67ce3b0da1d4401d9046f6194841c47459 Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 16 Apr 2024 11:10:34 +0800 Subject: [PATCH 02/10] feat: make getter public --- src/challenge/ChallengeManager.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/challenge/ChallengeManager.sol b/src/challenge/ChallengeManager.sol index 14acba8f4..5bd01f65c 100644 --- a/src/challenge/ChallengeManager.sol +++ b/src/challenge/ChallengeManager.sol @@ -122,7 +122,7 @@ contract ChallengeManager is DelegateCallAware, IChallengeManager { ospCond[wasmModuleRoot] = osp_; } - function getOSP(bytes32 wasmModuleRoot) internal view returns (IOneStepProofEntry) { + function getOSP(bytes32 wasmModuleRoot) public view returns (IOneStepProofEntry) { IOneStepProofEntry t = ospCond[wasmModuleRoot]; if (address(t) == address(0)){ return osp; From f05cd14d7b6f76ed8e16d0bdeb429f9759f8ed31 Mon Sep 17 00:00:00 2001 From: gzeon Date: Wed, 17 Apr 2024 02:10:03 +0800 Subject: [PATCH 03/10] fix: interface --- src/challenge/IChallengeManager.sol | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/challenge/IChallengeManager.sol b/src/challenge/IChallengeManager.sol index b6f63d672..8770b2cbf 100644 --- a/src/challenge/IChallengeManager.sol +++ b/src/challenge/IChallengeManager.sol @@ -39,6 +39,7 @@ interface IChallengeManager { event OneStepProofCompleted(uint64 indexed challengeIndex); event ChallengeEnded(uint64 indexed challengeIndex, ChallengeTerminationType kind); + event ConditonalOSPSet(bytes32 indexed wasmModuleRoot, IOneStepProofEntry osp_); function initialize( IChallengeResultReceiver resultReceiver_, @@ -47,6 +48,10 @@ interface IChallengeManager { IOneStepProofEntry osp_ ) external; + function setConditionalOsp(bytes32 wasmModuleRoot, IOneStepProofEntry osp_) external; + + function getOSP(bytes32 wasmModuleRoot) external view returns (IOneStepProofEntry); + function createChallenge( bytes32 wasmModuleRoot_, MachineStatus[2] calldata startAndEndMachineStatuses_, From e5ba1771b0b0bd5635e06f69da253fcf31a96d21 Mon Sep 17 00:00:00 2001 From: gzeon Date: Wed, 17 Apr 2024 02:27:52 +0800 Subject: [PATCH 04/10] fix: set condOsp with postUpgradeInit --- src/challenge/ChallengeManager.sol | 21 ++++++++++----------- src/challenge/IChallengeManager.sol | 13 +++++++++++-- test/foundry/ChallengeManager.t.sol | 17 +++++++++++++---- test/signatures/ChallengeManager | 4 +++- test/storage/ChallengeManager | 1 + 5 files changed, 38 insertions(+), 18 deletions(-) diff --git a/src/challenge/ChallengeManager.sol b/src/challenge/ChallengeManager.sol index 5bd01f65c..a77856a48 100644 --- a/src/challenge/ChallengeManager.sol +++ b/src/challenge/ChallengeManager.sol @@ -111,20 +111,19 @@ contract ChallengeManager is DelegateCallAware, IChallengeManager { osp = osp_; } - function postUpgradeInit(IOneStepProofEntry osp_) external onlyDelegated onlyProxyOwner { - // when updating to 4844 we need to create new osp contracts and set them here - // on the challenge manager + function postUpgradeInit( + IOneStepProofEntry osp_, + bytes32 condRoot, + IOneStepProofEntry condOsp + ) external onlyDelegated onlyProxyOwner { + emit ConditonalOSPSet(condRoot, condOsp); + ospCond[condRoot] = condOsp; osp = osp_; } - function setConditionalOsp(bytes32 wasmModuleRoot, IOneStepProofEntry osp_) external onlyDelegated onlyProxyOwner { - emit ConditonalOSPSet(wasmModuleRoot, osp_); - ospCond[wasmModuleRoot] = osp_; - } - - function getOSP(bytes32 wasmModuleRoot) public view returns (IOneStepProofEntry) { + function getOsp(bytes32 wasmModuleRoot) public view returns (IOneStepProofEntry) { IOneStepProofEntry t = ospCond[wasmModuleRoot]; - if (address(t) == address(0)){ + if (address(t) == address(0)) { return osp; } else { return t; @@ -274,7 +273,7 @@ contract ChallengeManager is DelegateCallAware, IChallengeManager { require(challengeLength == 1, "TOO_LONG"); } - bytes32 afterHash = getOSP(challenge.wasmModuleRoot).proveOneStep( + bytes32 afterHash = getOsp(challenge.wasmModuleRoot).proveOneStep( ExecutionContext({maxInboxMessagesRead: challenge.maxInboxMessages, bridge: bridge}), challengeStart, selection.oldSegments[selection.challengePosition], diff --git a/src/challenge/IChallengeManager.sol b/src/challenge/IChallengeManager.sol index 8770b2cbf..60f44c5d8 100644 --- a/src/challenge/IChallengeManager.sol +++ b/src/challenge/IChallengeManager.sol @@ -48,9 +48,18 @@ interface IChallengeManager { IOneStepProofEntry osp_ ) external; - function setConditionalOsp(bytes32 wasmModuleRoot, IOneStepProofEntry osp_) external; + function postUpgradeInit( + IOneStepProofEntry osp_, + bytes32 condRoot, + IOneStepProofEntry condOsp + ) external; + + /// @notice Get the default osp, which is used for all wasm module roots that don't have a conditional OSP set + /// Use getOsp(wasmModuleRoot) to get the OSP for a specific wasm module root + function osp() external view returns (IOneStepProofEntry); - function getOSP(bytes32 wasmModuleRoot) external view returns (IOneStepProofEntry); + /// @notice Get the OSP for a given wasm module root + function getOsp(bytes32 wasmModuleRoot) external view returns (IOneStepProofEntry); function createChallenge( bytes32 wasmModuleRoot_, diff --git a/test/foundry/ChallengeManager.t.sol b/test/foundry/ChallengeManager.t.sol index 0a46b8af6..9339812bd 100644 --- a/test/foundry/ChallengeManager.t.sol +++ b/test/foundry/ChallengeManager.t.sol @@ -11,9 +11,12 @@ contract ChallengeManagerTest is Test { IBridge bridge = IBridge(address(139)); IOneStepProofEntry osp = IOneStepProofEntry(address(140)); IOneStepProofEntry newOsp = IOneStepProofEntry(address(141)); + IOneStepProofEntry condOsp = IOneStepProofEntry(address(142)); address proxyAdmin = address(141); ChallengeManager chalmanImpl = new ChallengeManager(); + bytes32 randomRoot = keccak256(abi.encodePacked("randomRoot")); + function deploy() public returns (ChallengeManager) { ChallengeManager chalman = ChallengeManager( address(new TransparentUpgradeableProxy(address(chalmanImpl), proxyAdmin, "")) @@ -40,10 +43,16 @@ contract ChallengeManagerTest is Test { vm.prank(proxyAdmin); TransparentUpgradeableProxy(payable(address(chalman))).upgradeToAndCall( address(chalmanImpl), - abi.encodeWithSelector(ChallengeManager.postUpgradeInit.selector, newOsp) + abi.encodeWithSelector( + ChallengeManager.postUpgradeInit.selector, + newOsp, + randomRoot, + condOsp + ) ); - assertEq(address(chalman.osp()), address(newOsp), "New osp not set"); + assertEq(address(chalman.getOsp(bytes32(0))), address(newOsp), "New osp not set"); + assertEq(address(chalman.getOsp(randomRoot)), address(condOsp), "Cond osp not set"); } function testPostUpgradeInitFailsNotAdmin() public { @@ -51,12 +60,12 @@ contract ChallengeManagerTest is Test { vm.expectRevert(abi.encodeWithSelector(NotOwner.selector, address(151), proxyAdmin)); vm.prank(address(151)); - chalman.postUpgradeInit(osp); + chalman.postUpgradeInit(newOsp, randomRoot, condOsp); } function testPostUpgradeInitFailsNotDelCall() public { vm.expectRevert(bytes("Function must be called through delegatecall")); vm.prank(proxyAdmin); - chalmanImpl.postUpgradeInit(osp); + chalmanImpl.postUpgradeInit(newOsp, randomRoot, condOsp); } } diff --git a/test/signatures/ChallengeManager b/test/signatures/ChallengeManager index 3807c4ff4..1d5823206 100644 --- a/test/signatures/ChallengeManager +++ b/test/signatures/ChallengeManager @@ -7,11 +7,13 @@ "clearChallenge(uint64)": "56e9df97", "createChallenge(bytes32,uint8[2],(bytes32[2],uint64[2])[2],uint64,address,address,uint256,uint256)": "14eab5e7", "currentResponder(uint64)": "23a9ef23", + "getOsp(bytes32)": "3690b011", "initialize(address,address,address,address)": "f8c8765e", "isTimedOut(uint64)": "9ede42b9", "oneStepProveExecution(uint64,(uint256,uint256,bytes32[],uint256),bytes)": "d248d124", "osp()": "f26a62c6", - "postUpgradeInit(address)": "c474d2c5", + "ospCond(bytes32)": "dc74bf8b", + "postUpgradeInit(address,bytes32,address)": "5038934d", "resultReceiver()": "3504f1d7", "sequencerInbox()": "ee35f327", "timeout(uint64)": "1b45c86a", diff --git a/test/storage/ChallengeManager b/test/storage/ChallengeManager index 85c7f0358..15c3f1166 100644 --- a/test/storage/ChallengeManager +++ b/test/storage/ChallengeManager @@ -6,3 +6,4 @@ | sequencerInbox | contract ISequencerInbox | 3 | 0 | 20 | src/challenge/ChallengeManager.sol:ChallengeManager | | bridge | contract IBridge | 4 | 0 | 20 | src/challenge/ChallengeManager.sol:ChallengeManager | | osp | contract IOneStepProofEntry | 5 | 0 | 20 | src/challenge/ChallengeManager.sol:ChallengeManager | +| ospCond | mapping(bytes32 => contract IOneStepProofEntry) | 6 | 0 | 32 | src/challenge/ChallengeManager.sol:ChallengeManager | From 337f763783a91ba07f19beb9831f96240438c0b6 Mon Sep 17 00:00:00 2001 From: gzeon Date: Wed, 17 Apr 2024 02:28:44 +0800 Subject: [PATCH 05/10] chore: remove event --- src/challenge/ChallengeManager.sol | 1 - src/challenge/IChallengeManager.sol | 1 - 2 files changed, 2 deletions(-) diff --git a/src/challenge/ChallengeManager.sol b/src/challenge/ChallengeManager.sol index a77856a48..133ab1e2e 100644 --- a/src/challenge/ChallengeManager.sol +++ b/src/challenge/ChallengeManager.sol @@ -116,7 +116,6 @@ contract ChallengeManager is DelegateCallAware, IChallengeManager { bytes32 condRoot, IOneStepProofEntry condOsp ) external onlyDelegated onlyProxyOwner { - emit ConditonalOSPSet(condRoot, condOsp); ospCond[condRoot] = condOsp; osp = osp_; } diff --git a/src/challenge/IChallengeManager.sol b/src/challenge/IChallengeManager.sol index 60f44c5d8..236829b15 100644 --- a/src/challenge/IChallengeManager.sol +++ b/src/challenge/IChallengeManager.sol @@ -39,7 +39,6 @@ interface IChallengeManager { event OneStepProofCompleted(uint64 indexed challengeIndex); event ChallengeEnded(uint64 indexed challengeIndex, ChallengeTerminationType kind); - event ConditonalOSPSet(bytes32 indexed wasmModuleRoot, IOneStepProofEntry osp_); function initialize( IChallengeResultReceiver resultReceiver_, From 96774561636ceac0d3f4e5f915492993830c46db Mon Sep 17 00:00:00 2001 From: gzeon Date: Wed, 17 Apr 2024 02:36:26 +0800 Subject: [PATCH 06/10] docs: explain --- src/challenge/ChallengeManager.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/challenge/ChallengeManager.sol b/src/challenge/ChallengeManager.sol index 133ab1e2e..9ff53dc81 100644 --- a/src/challenge/ChallengeManager.sol +++ b/src/challenge/ChallengeManager.sol @@ -111,6 +111,10 @@ contract ChallengeManager is DelegateCallAware, IChallengeManager { osp = osp_; } + /// @dev A osp breaking change is introduced as part of Stylus upgrade, where the new osp would not support + /// pre-Stylus legacy wasmModuleRoot. To ensure that the new osp is not used for legacy wasmModuleRoot, + /// we introduce a conditional OSP where condRoot should be set to the pre-Stylus root and condOsp should + /// be set to the pre-Stylus osp. The correct value should be handled by the upgrade action contract. function postUpgradeInit( IOneStepProofEntry osp_, bytes32 condRoot, From 82e9425212beae99bb6e54d3ab771252f1d7b9d1 Mon Sep 17 00:00:00 2001 From: gzeon Date: Thu, 25 Apr 2024 00:49:56 +0800 Subject: [PATCH 07/10] fix: use machine hash from osp --- src/challenge/ChallengeManager.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/challenge/ChallengeManager.sol b/src/challenge/ChallengeManager.sol index b55d671f0..fc57b5eb7 100644 --- a/src/challenge/ChallengeManager.sol +++ b/src/challenge/ChallengeManager.sol @@ -250,8 +250,9 @@ contract ChallengeManager is DelegateCallAware, IChallengeManager { } bytes32[] memory segments = new bytes32[](2); - segments[0] = osp.getStartMachineHash(globalStateHashes[0], challenge.wasmModuleRoot); - segments[1] = osp.getEndMachineHash(machineStatuses[1], globalStateHashes[1]); + IOneStepProofEntry _osp = getOsp(challenge.wasmModuleRoot); + segments[0] = _osp.getStartMachineHash(globalStateHashes[0], challenge.wasmModuleRoot); + segments[1] = _osp.getEndMachineHash(machineStatuses[1], globalStateHashes[1]); challenge.mode = ChallengeLib.ChallengeMode.EXECUTION; From 0ab2b78d89f31db9966d181e6d524b3a3843af43 Mon Sep 17 00:00:00 2001 From: Goran Vladika Date: Thu, 25 Apr 2024 17:40:52 +0200 Subject: [PATCH 08/10] Add unit test to test conditional OSP --- test/foundry/ChallengeManager.t.sol | 105 +++++++++++++++++++++++++--- 1 file changed, 95 insertions(+), 10 deletions(-) diff --git a/test/foundry/ChallengeManager.t.sol b/test/foundry/ChallengeManager.t.sol index 9339812bd..c4f127f9c 100644 --- a/test/foundry/ChallengeManager.t.sol +++ b/test/foundry/ChallengeManager.t.sol @@ -4,6 +4,8 @@ pragma solidity ^0.8.4; import "forge-std/Test.sol"; import "./util/TestUtil.sol"; import "../../src/challenge/ChallengeManager.sol"; +import "../../src/osp/OneStepProofEntry.sol"; +import "forge-std/console.sol"; contract ChallengeManagerTest is Test { IChallengeResultReceiver resultReceiver = IChallengeResultReceiver(address(137)); @@ -23,20 +25,106 @@ contract ChallengeManagerTest is Test { ); chalman.initialize(resultReceiver, sequencerInbox, bridge, osp); assertEq( - address(chalman.resultReceiver()), - address(resultReceiver), - "Result receiver not set" + address(chalman.resultReceiver()), address(resultReceiver), "Result receiver not set" ); assertEq( - address(chalman.sequencerInbox()), - address(sequencerInbox), - "Sequencer inbox not set" + address(chalman.sequencerInbox()), address(sequencerInbox), "Sequencer inbox not set" ); assertEq(address(chalman.bridge()), address(bridge), "Bridge not set"); assertEq(address(chalman.osp()), address(osp), "OSP not set"); return chalman; } + function testCondOsp() public { + ChallengeManager chalman = deploy(); + + /// legacy root and OSP that will be used as conditional + IOneStepProofEntry legacyOSP = IOneStepProofEntry( + address( + new OneStepProofEntry( + IOneStepProver(makeAddr("0")), + IOneStepProver(makeAddr("mem")), + IOneStepProver(makeAddr("math")), + IOneStepProver(makeAddr("hostio")) + ) + ) + ); + bytes32 legacyRoot = keccak256(abi.encodePacked("legacyRoot")); + + // legacy hashes + bytes32 legacySegment0 = legacyOSP.getStartMachineHash( + keccak256(abi.encodePacked("globalStateHash[0]")), legacyRoot + ); + bytes32 legacySegment1 = legacyOSP.getEndMachineHash( + MachineStatus.FINISHED, keccak256(abi.encodePacked("globalStateHashes[1]")) + ); + + /// new OSP + IOneStepProofEntry _newOSP = IOneStepProofEntry( + address( + new OneStepProofEntry( + IOneStepProver(makeAddr("0")), + IOneStepProver(makeAddr("mem")), + IOneStepProver(makeAddr("math")), + IOneStepProver(makeAddr("hostio")) + ) + ) + ); + + // new hashes + bytes32 newSegment0 = _newOSP.getStartMachineHash( + keccak256(abi.encodePacked("globalStateHash[0]")), randomRoot + ); + bytes32 newSegment1 = _newOSP.getEndMachineHash( + MachineStatus.FINISHED, keccak256(abi.encodePacked("new_globalStateHashes[1]")) + ); + + /// do upgrade + vm.prank(proxyAdmin); + TransparentUpgradeableProxy(payable(address(chalman))).upgradeToAndCall( + address(chalmanImpl), + abi.encodeWithSelector( + ChallengeManager.postUpgradeInit.selector, _newOSP, legacyRoot, legacyOSP + ) + ); + + /// check cond osp + IOneStepProofEntry _condOsp = chalman.getOsp(legacyRoot); + assertEq(address(_condOsp), address(legacyOSP), "Legacy osp not set"); + assertEq( + _condOsp.getStartMachineHash( + keccak256(abi.encodePacked("globalStateHash[0]")), legacyRoot + ), + legacySegment0, + "Unexpected start machine hash" + ); + assertEq( + _condOsp.getEndMachineHash( + MachineStatus.FINISHED, keccak256(abi.encodePacked("globalStateHashes[1]")) + ), + legacySegment1, + "Unexpected end machine hash" + ); + + /// check new osp + IOneStepProofEntry _newOsp = chalman.getOsp(randomRoot); + assertEq(address(_newOsp), address(_newOSP), "New osp not set"); + assertEq( + _newOsp.getStartMachineHash( + keccak256(abi.encodePacked("globalStateHash[0]")), randomRoot + ), + newSegment0, + "Unexpected start machine hash" + ); + assertEq( + _newOsp.getEndMachineHash( + MachineStatus.FINISHED, keccak256(abi.encodePacked("new_globalStateHashes[1]")) + ), + newSegment1, + "Unexpected end machine hash" + ); + } + function testPostUpgradeInit() public { ChallengeManager chalman = deploy(); @@ -44,10 +132,7 @@ contract ChallengeManagerTest is Test { TransparentUpgradeableProxy(payable(address(chalman))).upgradeToAndCall( address(chalmanImpl), abi.encodeWithSelector( - ChallengeManager.postUpgradeInit.selector, - newOsp, - randomRoot, - condOsp + ChallengeManager.postUpgradeInit.selector, newOsp, randomRoot, condOsp ) ); From 7867bae59ab522f7eedd83d020329184c3a818da Mon Sep 17 00:00:00 2001 From: Goran Vladika Date: Fri, 26 Apr 2024 10:13:28 +0200 Subject: [PATCH 09/10] Add check --- test/foundry/ChallengeManager.t.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/foundry/ChallengeManager.t.sol b/test/foundry/ChallengeManager.t.sol index c4f127f9c..16e0916e8 100644 --- a/test/foundry/ChallengeManager.t.sol +++ b/test/foundry/ChallengeManager.t.sol @@ -123,6 +123,10 @@ contract ChallengeManagerTest is Test { newSegment1, "Unexpected end machine hash" ); + + /// check hashes are different + assertNotEq(legacySegment0, newSegment0, "Start machine hash should be different"); + assertNotEq(legacySegment1, newSegment1, "End machine hash should be different"); } function testPostUpgradeInit() public { From 09ff1db5fee0023eef2935bd236518f338cdd09c Mon Sep 17 00:00:00 2001 From: Goran Vladika Date: Tue, 30 Apr 2024 17:32:46 +0200 Subject: [PATCH 10/10] Remove debug --- test/foundry/ChallengeManager.t.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/test/foundry/ChallengeManager.t.sol b/test/foundry/ChallengeManager.t.sol index 16e0916e8..88557db99 100644 --- a/test/foundry/ChallengeManager.t.sol +++ b/test/foundry/ChallengeManager.t.sol @@ -5,7 +5,6 @@ import "forge-std/Test.sol"; import "./util/TestUtil.sol"; import "../../src/challenge/ChallengeManager.sol"; import "../../src/osp/OneStepProofEntry.sol"; -import "forge-std/console.sol"; contract ChallengeManagerTest is Test { IChallengeResultReceiver resultReceiver = IChallengeResultReceiver(address(137));