eth/dex/client: Add initiateBatch function #1251

martonp · 2021-10-24T13:38:37Z

This PR adds a batch initiation function to the ETH swap contract. This function saves a bit of gas when initiating multiple swaps simultaneously, and also allows ETH to conform to the current wallet interface which requires swaps to be submitted together.

Here are some logs from the rpc test:

Gas used for initiate: 177726 

Gas used for batch initiating 1 swaps: 180037. 180037 more than previous 
Gas used for batch initiating 2 swaps: 338287. 158250 more than previous 
Gas used for batch initiating 3 swaps: 496537. 158250 more than previous 
Gas used for batch initiating 4 swaps: 654775. 158238 more than previous 
Gas used for batch initiating 5 swaps: 813025. 158250 more than previous 
Gas used for batch initiating 6 swaps: 971275. 158250 more than previous 
Gas used for batch initiating 7 swaps: 1129513. 158238 more than previous 
Gas used for batch initiating 8 swaps: 1287763. 158250 more than previous 
Gas used for batch initiating 9 swaps: 1446025. 158262 more than previous

We save ~20k gas for each additional swap that is batched.

dex/networks/eth/contracts/ETHSwapV0.sol

chappjc

Good stuff. A couple Solidity questions, mainly wondering if the solc optimization would just end up with the same code anyway.

dex/networks/eth/contracts/ETHSwapV0.sol

chappjc · 2021-10-24T21:52:19Z

dex/networks/eth/contracts/ETHSwapV0.sol

+            require(initiations[i].refundTimestamp > 0);
+            require(swaps[initiations[i].secretHash].state == State.Empty);
+            initVal = initVal + initiations[i].value;
+            swaps[initiations[i].secretHash].initBlockNumber = block.number;


Unless it changes the actual execution and therefore gas spent, can we make initiations[i].secretHash a local variable?

Actually, and I'm just trying to learn some Solidity, but I see some examples where you bind to the entry in the map and assign like:

var swap = swaps[secretHash]; swap.initBlockNumer = block.number;

I've updated it. It saves some gas and looks nicer.

In solidity, reference variables are either in the storage, memory, or calldata. Storage is the memory that gets persisted in the global ethereum state. In my change I've created a local reference to a location in storage.

JoeGruffins

Looks reasonable to me. I can't think of any loopholes created by this.

It changes the amount of gas needed for funding initiations, or the swap size, by making it variable, but we can deal with that.

JoeGruffins · 2021-10-25T05:50:26Z

dex/networks/eth/contracts/ETHSwapV0.sol

+            swaps[initiations[i].secretHash].state = State.Filled;
+        }
+
+        require(initVal == msg.value);


It looks like this function will succeed with a zero length initiations array, which may be fine but not sure.

Also, there is no upward limit on the number of initiations. I wonder if that is ok?

I think both are fine. If someone calls with a zero length array, they just waste some gas and change nothing. No reason to waste other people's gas with an extra check.

For the upward limit, I think anything is fine as long as the user has enough gas to pay for it. Maybe there are some limits on the amount of data that's allowed to be passed into a transaction..I'll look into it.

dex/networks/eth/contracts/ETHSwapV0.sol

martonp · 2021-10-25T12:42:02Z

I've taken out the secretHash field in the Swap struct and now the gas usage looks like this:

Gas used for initiate: 155610 
Gas used for batch initiating 1 swaps: 156476. 156476 more than previous 
Gas used for batch initiating 2 swaps: 291165. 134689 more than previous 
Gas used for batch initiating 3 swaps: 425842. 134677 more than previous 
Gas used for batch initiating 4 swaps: 560531. 134689 more than previous 
Gas used for batch initiating 5 swaps: 695220. 134689 more than previous 
Gas used for batch initiating 6 swaps: 829909. 134689 more than previous 
Gas used for batch initiating 7 swaps: 964598. 134689 more than previous 
Gas used for batch initiating 8 swaps: 1099299. 134701 more than previous 
Gas used for batch initiating 9 swaps: 1233952. 134653 more than previous

I think for the swap size we can just use the amount of gas needed for a single swap, and then the user will just save some money if multiple swaps are submitted simultaneously.

In the interest of simplicity, maybe we could remove the non-batch initiate function. The gas difference is negligible.

dex/networks/eth/contracts/ETHSwapV0.sol

chappjc · 2021-10-25T14:42:40Z

dex/networks/eth/contracts/ETHSwapV0.sol

@@ -118,13 +117,14 @@ contract ETHSwap {
        senderIsOrigin()
        isNotInitiated(secretHash)
    {
-        swaps[secretHash].initBlockNumber = block.number;
-        swaps[secretHash].refundBlockTimestamp = refundTimestamp;
-        swaps[secretHash].secretHash = secretHash;


I wanted to test how much gas was saved by removing secretHash, and it gives the same estimate...
I'm running both the abigen and genruntime.sh commands, and can see the changes reflected in both BinRuntimeV0.go and contract.go. I've restarted the harness.
What am I missing?

You need to update the line in harness.sh which specifies the bytecode.

Removing secretHash was the bulk of the savings.. the other changes saved only a little bit. The SSTORE op code which sets a value in storage from 0 to non zero costs 20k gas, and we're no longer doing one of those.

Thanks for clarifying. Seems like we have the same contract hex string in a few places. Maybe we can address that at some point.

I was curious about the secretHash savings because mappings don't save the key anywhere. Even though there's no ranging on contracts with Solidity because of this, I have to think that inspecting the mapping in the state with... I don't know... geth and EVM magic, there would surely be a way to debug and analyze the contents of the mapping, at least for a full node. Even though from the contract's point of view this does not matter because you must have the hash to retrieve a value type, I wonder if we'd regret not having the hash recorded anywhere for incomplete swaps.
Not a strong argument for storing it though.

Looks like it would be pretty hard to get those:
https://stackoverflow.com/a/65530927

We could emit an event every time someone initiates a swap, and then a full node could check them. This would much cheaper. Can you think of any use case for this?

Also if there's ETH left unclaimed in the contract after a year maybe we could have some kind of game, and the winner could take it haha.

Looks like you don't need an archival node to check current state, just https://github.com/ethereum/go-ethereum/blob/0a3993c558616868e35f9730e92c704ac16ee437/core/state/statedb.go#L634

An event log does sound nice, but I think you could also look for all txns that call the initiate method(s) and check the call data too, right? EDIT: oh that would be an archival thing too, which presumably you could do with etherscan somehow

Anyway, I'm just chatting. We can do without this field.

Yeah etherscan def runs an archival node. Maybe we can query etherscan if we're trying to do something with the secret hashes.

chappjc

LGTM and works alright.

But the existing TestInitiateGas returns a ridiculously low gas estimate for me until I run TestInitiateBatchGas and retry TestInitiateGas. My harness is running geth 1.10.10.

buck54321 · 2021-10-26T12:10:46Z

client/asset/eth/rpcclient_harness_test.go

+			t.Fatalf("unexpected error for test %v: %v", test.name, err)
+		}
+
+		// It appears the receipt is only accessable after the tx is mined.


eth/dex/client: Add initiateBatch function

2280f1d

martonp commented Oct 24, 2021

View reviewed changes

dex/networks/eth/contracts/ETHSwapV0.sol Outdated Show resolved Hide resolved

chappjc reviewed Oct 24, 2021

View reviewed changes

JoeGruffins reviewed Oct 25, 2021

View reviewed changes

Updates based on review.

88b8340

chappjc reviewed Oct 25, 2021

View reviewed changes

swap -> swapToUpdate

cb805c4

chappjc approved these changes Oct 25, 2021

View reviewed changes

JoeGruffins approved these changes Oct 26, 2021

View reviewed changes

buck54321 approved these changes Oct 26, 2021

View reviewed changes

Fix spelling.

b2dd7e9

chappjc merged commit a6eca9e into decred:master Oct 26, 2021

chappjc mentioned this pull request Oct 26, 2021

eth: Update geth to v1.10.11 #1257

Merged

chappjc added the ETH label Nov 24, 2021

chappjc added this to the 0.5 milestone Nov 28, 2021

chappjc removed this from the 0.5 milestone Aug 26, 2022

martonp deleted the initiateBatch branch December 20, 2022 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eth/dex/client: Add initiateBatch function #1251

eth/dex/client: Add initiateBatch function #1251

martonp commented Oct 24, 2021

chappjc left a comment

chappjc Oct 24, 2021

martonp Oct 25, 2021

JoeGruffins left a comment

JoeGruffins Oct 25, 2021

martonp Oct 25, 2021

martonp commented Oct 25, 2021

chappjc Oct 25, 2021 •

edited

Loading

martonp Oct 25, 2021

martonp Oct 25, 2021

chappjc Oct 25, 2021 •

edited

Loading

martonp Oct 25, 2021

chappjc Oct 25, 2021 •

edited

Loading

martonp Oct 26, 2021

chappjc left a comment

buck54321 Oct 26, 2021

eth/dex/client: Add initiateBatch function #1251

eth/dex/client: Add initiateBatch function #1251

Conversation

martonp commented Oct 24, 2021

chappjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoeGruffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented Oct 25, 2021

chappjc Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Oct 25, 2021 •

edited

Loading

chappjc Oct 25, 2021 •

edited

Loading

chappjc Oct 25, 2021 •

edited

Loading