feat: hubpool third iteration. Add dispute logic and refine bitmap structure. #14

chrismaree · 2022-01-31T20:10:21Z

No description provided.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree · 2022-02-01T21:59:57Z

contracts/BridgeAdmin.sol

@@ -1,13 +0,0 @@
-// SPDX-License-Identifier: GPL-3.0-only


I removed this for now...we dont have a definition of what this requires just yet and I thought it cleaner to directly add the finder to the hub.

chrismaree · 2022-02-01T22:00:52Z

contracts/HubPool.sol

+        uint64 unclaimedPoolRebalanceLeafCount;
        bytes32 poolRebalanceRoot;
        bytes32 destinationDistributionRoot;
-        mapping(uint256 => uint256) claimedBitMap;


this is an important change: we use a 1D bitMap after this PR, thereby reducing storage requirements and enabling us to re-use state between each proposal. tradeoff is we can only have up to 256 different chains supported, but this is likely fine :)

yeah this seems reasonable until v3 :D

chrismaree · 2022-02-01T22:01:06Z

contracts/HubPool.sol

-        uint64 indexed relayerRefundId,
        uint64 requestExpirationTimestamp,
-        uint64 poolRebalanceLeafCount,
+        uint64 unclaimedPoolRebalanceLeafCount,


variable name changed for @nicholaspai

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree · 2022-02-01T22:02:14Z

contracts/HubPool.sol

+        require(getCurrentTime() >= refundRequest.requestExpirationTimestamp, "Not passed liveness");

        // Verify the leafId in the poolRebalance has not yet been claimed.
-        require(!MerkleLib.isClaimed(relayerRefund.claimedBitMap, poolRebalance.leafId), "Already claimed");


we use a separate MerkleLib method that I added to deal with 1D bit maps.

I will add unit tests for this once @mrice32 's MerkleLib PR has been merged.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree · 2022-02-01T22:05:57Z

contracts/HubPool.sol

-    function getNumberOfRelayRefunds() public view returns (uint256) {
-        if (relayerRefundRequests.length == 0) return 0;
-        return relayerRefundRequests.length - 1;
+    function _getRefundProposalAncillaryData() public view returns (bytes memory ancillaryData) {


this was my initial pass at constructing the ancillary data. curious what you guys think & if we need more/less in this.

these seem reasonable. I'm thinking about ways we can reduce the amount of calls to appendKeyValue... because these functions are expensive I believe

Do we call this in the happy path? Or only on disputes? If only on disputes, we might not need to worry about gas quite as much (although it's still worth thinking about!!).

it only happens in the dispute case so the gas does not matter too much I don't think.

chrismaree · 2022-02-01T22:06:33Z

contracts/MerkleLib.sol

+     * @param index the index to check in the bitmap.
+     \* @return bool indicating if the index within the claimedBitMap has been marked as claimed.
+     */
+    function isClaimed1D(uint256 claimedBitMap, uint256 index) public pure returns (bool) {


these are the new merkleLib methods I added. v simple and adapted from the original ones to take in a uint256 claimedBitMap rather than mapping(uint256 => uint256) storage claimedBitMap

chrismaree · 2022-02-01T22:07:09Z

test/Uma.Fixture.ts

@@ -0,0 +1,33 @@
+import { getContractFactory, utf8ToHex } from "./utils";


~30 lines to deploy the whole UMA ecosystem <3. I love clean repos.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

contracts/HubPool.sol

nicholaspai · 2022-02-01T22:22:23Z

contracts/HubPool.sol

+        refundRequest.unclaimedPoolRebalanceLeafCount = poolRebalanceLeafCount;
+        refundRequest.poolRebalanceRoot = poolRebalanceRoot;
+        refundRequest.destinationDistributionRoot = destinationDistributionRoot;
+        refundRequest.proposer = msg.sender;


All things being equal I prefer using key-value syntax to set refundRequest here because it means that the developer is less likely to forget to set a parameter. Are there gas savings to setting each param individually and leaving default values as-is for keys like proposerBondRepaid?

so that would replace the delete call plus this assigmeneet with this:

// Remove the existing information relating to the previous relayer refund request and set to the new info. refundRequest = RefundRequest({ requestExpirationTimestamp: requestExpirationTimestamp, unclaimedPoolRebalanceLeafCount: poolRebalanceLeafCount, poolRebalanceRoot: poolRebalanceRoot, destinationDistributionRoot: destinationDistributionRoot, claimedBitMap: 0, proposer: msg.sender, proposerBondRepaid: false });

I tried out the gas costs:
using the old assignment + delete that I had cost: 155863
using the syntax above cost: 156041

I think the reason the original syntax is cheaper is you are deleting storage and then not re-asigining some things, such as the claimedBitMap and the proposerBondRepaid. for these variables the default (post delete values) are the correct in this context.

As a result, I'm going leave it with the original implementation, unless you have anything else I can try that is syntactically better but still gas efficient.

I also think there are some annoying solidity inefficiencies with key value syntax. I think they create the entire struct in mem and then assign it rather than just assigning each value directly to storage. This increased mem usage might be why the gas costs are higher.

sounds good to me

nicholaspai · 2022-02-01T22:24:36Z

contracts/HubPool.sol

+        delete refundRequest; // Remove the existing information relating to the previous relayer refund request.
+
+        refundRequest.requestExpirationTimestamp = requestExpirationTimestamp;
+        refundRequest.unclaimedPoolRebalanceLeafCount = poolRebalanceLeafCount;


Do we need to check that unclaimedPoolRebalanceLeafCount > 0? I only ask because the condition unclaimedPoolRebalanceLeafCount == 0 is an important one in this contract but I also don't think its exploitable to set it to 0.

ye, this is a good point and is actually something we should check. if the relayer provided a set of proofs with poolRebalanceLeafCount set to 0 then there is actually no L1->L2 actions that this bundle can do. they could be providing valid roots that do nothing. I agree that we should only process a bundle if it has at least one valid L2 action. I'll add this require.

nicholaspai

I haven't checked all the tests out carefully yet but first pass the contract logic makes sense!

Signed-off-by: chrismaree <christopher.maree@gmail.com>

mrice32

This looks awesome!! Only minor comments and responses to threads.

mrice32 · 2022-02-01T23:31:50Z

contracts/HubPool.sol


-    event RelayerRefundDisputed(uint256 relayerRefundId, address disputer);
+    event RelayerRefundDisputed(
+        address disputer,


mrice32 · 2022-02-01T23:32:45Z

contracts/HubPool.sol

+        SkinnyOptimisticOracleInterface.Request ooPriceRequest,
+        RefundRequest refundRequest


One minor comment on these: we might want to select out parts of these structs we want to emit to 1) reduce the data and 2) make etherscan better able to encode/decode the events rather than just showing bytes.

this is a good point and i'll change the same for SpokePool

I'm going to leave this for now and will try and see how much of a gas difference this makes.

i don't think it should change anything

mrice32 · 2022-02-01T23:33:06Z

contracts/HubPool.sol

-    }
-
-    function setBondTokenFinalFeeMultiplier(uint64 newBondAmount) public onlyOwner {
+    function setBond(IERC20 newBondToken, uint256 newBondAmount) public onlyOwner onlyIfNoActiveRequest {


+1 for this modifier

mrice32 · 2022-02-01T23:34:00Z

contracts/HubPool.sol

-            "Last bundle has unclaimed leafs"
-        );
+    ) public onlyIfNoActiveRequest {
+        require(poolRebalanceLeafCount > 0, "Bundle must have at least 1 leaf");


+1 good check. This will make us more spam resistant.

mrice32 · 2022-02-01T23:36:23Z

contracts/HubPool.sol

+        refundRequest.unclaimedPoolRebalanceLeafCount = poolRebalanceLeafCount;
+        refundRequest.poolRebalanceRoot = poolRebalanceRoot;
+        refundRequest.destinationDistributionRoot = destinationDistributionRoot;
+        refundRequest.proposer = msg.sender;


I also think there are some annoying solidity inefficiencies with key value syntax. I think they create the entire struct in mem and then assign it rather than just assigning each value directly to storage. This increased mem usage might be why the gas costs are higher.

mrice32 · 2022-02-01T23:38:28Z

contracts/HubPool.sol

-    function getNumberOfRelayRefunds() public view returns (uint256) {
-        if (relayerRefundRequests.length == 0) return 0;
-        return relayerRefundRequests.length - 1;
+    function _getRefundProposalAncillaryData() public view returns (bytes memory ancillaryData) {


Do we call this in the happy path? Or only on disputes? If only on disputes, we might not need to worry about gas quite as much (although it's still worth thinking about!!).

mrice32 · 2022-02-01T23:41:46Z

contracts/HubPool.sol

+            refundRequest.requestExpirationTimestamp
+        );
+
+        ancillaryData = AncillaryData.appendKeyValueUint(


If we do end up caring about hyper-optimizing gas usage here, we probably need to take a second look at the Uint method, since I don't think we optimized that last time around.

it only is called in the dispute case so I don't think it matters too much

nicholaspai

Thanks for responding to my comments, lgtm

mrice32 · 2022-02-02T15:31:07Z

contracts/MerkleLib.sol

+     \* @return bool indicating if the index within the claimedBitMap has been marked as claimed.
+     */
+    function isClaimed1D(uint256 claimedBitMap, uint256 index) public pure returns (bool) {
+        return claimedBitMap & (1 << index) > 0;


nit: can you explain the differences in logic from the uniswap one?

I think their version of this code is:

uint256 mask = (1 << index); return claimedBitMap & mask == mask;

Not trying to be nit-picky, this logic is just very dense, and I want to make sure we're super confident in the correctness.

going to add unit tests now on this and will verify the behaviour. can test out this other code example as well (note I created my implementation from modifying the uniswap code to be 1 dimensional :) )

both work equivalently. I reverted to your recommendation as it's easier to read.

mrice32 · 2022-02-02T15:34:54Z

contracts/HubPool.sol

        bytes32 poolRebalanceRoot;
        bytes32 destinationDistributionRoot;
-        mapping(uint256 => uint256) claimedBitMap;
+        uint256 claimedBitMap;


I think this is a good change, but we've started making some assumptions like "there will never be more than X tokens or Y chains". I want to write these down somewhere. Should we go back to the interface doc and write these down? Or start a known issues and constraints doc to outline this sort of stuff? Thoughts?

I will go ahead and start such a doc, but let me know if you disagree.

ok great. ye, you right, we need to write this down! the interface doc is now somewhat out of date though in some regards. our options are to update the interface doc or start a new one.

in either case, I'm going to add some more comments on this in the code.

mrice32 · 2022-02-02T15:36:51Z

contracts/HubPool.sol

-    event BondAmountSet(uint64 newBondMultiplier);
-
-    event BondTokenSet(address newBondMultiplier);
+    event BondSet(address newBondToken, uint256 newBondAmount);


Should we index newBondToken?

Signed-off-by: chrismaree <christopher.maree@gmail.com>

mrice32

LGTM! Minor comments on events!

mrice32 · 2022-02-02T21:13:10Z

contracts/HubPool.sol

        bytes32 destinationDistributionRoot,
        address indexed proposer
    );
    event RelayerRefundExecuted(uint256 relayerRefundId, MerkleLib.PoolRebalance poolRebalance, address caller);


Can we enumerate the fields in poolRebalance? Also, do you think there are fields we can remove? Do we think these emissions are important for bot verification or anything?

mrice32 · 2022-02-02T21:15:04Z

contracts/HubPool.sol

+        );

-        relayerRefundRequests.pop();
+        emit RelayerRefundDisputed(msg.sender, ooPriceRequest, refundRequest);


Can we enumerate these structs for better parseability? Also might be nice to do so so we can drop fields that are unnecessary.

will do this on a follow up.

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

chrismaree and others added 7 commits January 28, 2022 14:04

nit

ba54cb9

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

84aad83

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

e971ff7

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Update contracts/HubPool.sol

7a24fba

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

review nit

2f99c84

Signed-off-by: chrismaree <christopher.maree@gmail.com>

review nit

dcb052a

Signed-off-by: chrismaree <christopher.maree@gmail.com>

feat(hubpool): Refactor hub pool to use 1D bitmap integer

487e7ea

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree mentioned this pull request Jan 31, 2022

feat(HubPool): Second iteration on init, dispute and execute relayer refund bundles #13

Merged

nit

39ea052

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree marked this pull request as draft January 31, 2022 20:49

chrismaree added 6 commits February 1, 2022 09:03

WIP

febc4a6

Signed-off-by: chrismaree <christopher.maree@gmail.com>

review nit

1ba6779

Signed-off-by: chrismaree <christopher.maree@gmail.com>

Merge branch 'master' into chrismaree/third-pass-replayment-claim

1886cfd

review nit

439138b

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

b7e3ae2

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

1f6fc17

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree commented Feb 1, 2022

View reviewed changes

nit

f64bf85

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree commented Feb 1, 2022

View reviewed changes

nit

8bdcec7

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree commented Feb 1, 2022

View reviewed changes

chrismaree requested review from mrice32 and nicholaspai and removed request for nicholaspai February 1, 2022 22:07

chrismaree marked this pull request as ready for review February 1, 2022 22:07

nit

89e642a

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nicholaspai reviewed Feb 1, 2022

View reviewed changes

contracts/HubPool.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Feb 1, 2022

View reviewed changes

contracts/HubPool.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Feb 1, 2022

View reviewed changes

contracts/HubPool.sol Outdated Show resolved Hide resolved

nicholaspai reviewed Feb 1, 2022

View reviewed changes

chrismaree added 2 commits February 1, 2022 14:49

nit

d6bc9bd

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

8de1b8f

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree requested a review from nicholaspai February 1, 2022 23:08

chrismaree changed the title ~~chrismaree/third pass replayment claim~~ feat: hubpool third iteration. Add dispute logic and refine bitmap structure. Feb 1, 2022

mrice32 approved these changes Feb 1, 2022

View reviewed changes

mrice32 reviewed Feb 1, 2022

View reviewed changes

nicholaspai approved these changes Feb 2, 2022

View reviewed changes

mrice32 reviewed Feb 2, 2022

View reviewed changes

chrismaree added 4 commits February 2, 2022 08:34

nit

98b2b52

Signed-off-by: chrismaree <christopher.maree@gmail.com>

WIP

eefd0c0

Signed-off-by: chrismaree <christopher.maree@gmail.com>

WIP

5de87a2

Signed-off-by: chrismaree <christopher.maree@gmail.com>

nit

9d75d36

Signed-off-by: chrismaree <christopher.maree@gmail.com>

mrice32 approved these changes Feb 2, 2022

View reviewed changes

nit

8f01372

Signed-off-by: chrismaree <christopher.maree@gmail.com>

chrismaree merged commit e46a6cc into master Feb 3, 2022

chrismaree deleted the chrismaree/third-pass-replayment-claim branch February 3, 2022 00:32

pxrl pushed a commit that referenced this pull request Feb 20, 2024

feat: M01 Deadline Buffer for Fills Is Not Always Respected (#14)

71383bc

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

		@@ -0,0 +1,33 @@
		import { getContractFactory, utf8ToHex } from "./utils";

		SkinnyOptimisticOracleInterface.Request ooPriceRequest,
		RefundRequest refundRequest

feat: hubpool third iteration. Add dispute logic and refine bitmap structure. #14

feat: hubpool third iteration. Add dispute logic and refine bitmap structure. #14

Uh oh!

Conversation

chrismaree commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai left a comment

Choose a reason for hiding this comment

Uh oh!

mrice32 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicholaspai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

chrismaree commented Jan 31, 2022 •

edited

Loading

chrismaree Feb 2, 2022 •

edited

Loading