Bridge gas optimisations #301

eth-r · 2022-05-17T16:18:06Z

The bridge is mostly fairly close to the limit of achievable performance, but a few optimisations could be identified:

some hashing operations can be made more efficient
storage slots in BridgeState can be aligned and packed so that multiple accesses are minimised
some allocations can be avoided by adding at-functions in bitcoin-spv

All in all, single percent improvements can be made in some functions (particularly proof submissions) and rarely called operations involving storage (initialisation, updating parameters) become significantly cheaper

Refs #260

…nctions

pdyraga · 2022-05-18T11:39:50Z

Let's also inspect the usage of bytes memory across the project. I was able to get some small savings for Bank.approveBalanceAndCall in 237b3c1 (PR #302) but I haven't gone through all bytes memory usages in the project to not run into conflicts with this PR.

pdyraga · 2022-05-18T11:39:50Z

Let's also inspect the usage of bytes memory across the project. I was able to get some small savings for Bank.approveBalanceAndCall in 237b3c1 (PR #302) but I haven't gone through all bytes memory usages in the project to not run into conflicts with this PR.

eth-r · 2022-05-18T16:04:02Z

I specifically checked bitcoin-spv for possible calldata optimisations and determined the following:

unless there is a way to call precompiled contracts with calldata instead of memory, ValidateSPV.validateHeaderChain cannot benefit from calldata
ValidateSPV.prove could theoretically use calldata for the intermediate nodes
the input and output vectors of proofs could theoretically use calldata
removing all memory copying in validateHeaderChain gave savings of around 2000 gas in all proofs (deposit, sweep, moving funds) so there is minor potential for savings from avoiding the copying of calldata into memory as well
without making bitcoin-spv calldata compatible there doesn't seem to be a benefit from using calldata more widely in the bridge

Using keccak256(keccak256(script), walletPubKeyHash) gives higher performance in some circumstances. Update the key calculation method in tests to match what is implemented in the contracts.

solidity/contracts/bridge/Redemption.sol

solidity/contracts/bridge/BridgeState.sol

pdyraga · 2022-05-26T07:20:49Z

👀

pdyraga · 2022-05-26T07:41:48Z

solidity/contracts/bridge/BitcoinTx.sol

-        );
-        // Make sure the actual output script matches either the P2PKH
-        // or P2WPKH format.
+        uint256 scriptLen = output.length - 8;


We could note why it's - 8, something like:

// The output consists of an 8-byte value and a variable length script. // To extract just the script, we ignore the first 8 bytes. uint256 scriptLen = output.length - 8;

This could be helpful, especially that later we also slice32(8).

Addressed in #345 in 9aeb58c.

solidity/contracts/bridge/BridgeState.sol

pdyraga · 2022-05-26T08:06:11Z

solidity/contracts/bridge/BridgeState.sol

+        // Move movingFundsTxMaxTotalFee to the next storage slot for a more
+        // efficient variable layout
+        // slither-disable-next-line unused-state
+        bytes8 __depositAlignmentGap;


Is it actually moving movingFundsTxMaxTotalFee to the next slot? Shouldn't we use bytes16 instead?

Slot 1: Bank bank
Slot 2: IRelay relay | uint96 txProofDifficultyFactor
Slot 3: EcdsaWalletRegistry ecdsaWalletRegistry
Slot 4: address treasury | uint64 depositDustThreshold
Slot 5: uint64 depositTreasuryFeeDivisor | uint64 depositTxMaxFee | bytes8 __depositAlignmentGap | uint64 movingFundsTxMaxTotalFee

If this was bytes16, then the next alignment gap would match:
Slot 6: uint64 movingFundsTxMaxTotalFee | uint64 movingFundsDustThreshold | uint32 movingFundsTimeoutResetDelay | uint32 movingFundsTimeout
Slot 7: uint96 movingFundsTimeoutSlashingAmount
Slot 8: uint256 movingFundsTimeoutNotifierRewardMultiplier
Slot 9: uint64 movedFundsSweepTxMaxTotalFee | uint32 movedFundsSweepTimeout | uint96 movedFundsSweepTimeoutSlashingAmount | uint64 movedFundsSweepTimeoutNotifierRewardMultiplier
Slot 10: uint64 redemptionDustThreshold | uint64 redemptionTreasuryFeeDivisor | uint64 redemptionTxMaxFee | bytes8 __redemptionAlignmentGap

Addressed in #345 in 9d15d6e.

pdyraga · 2022-05-26T08:29:59Z

solidity/contracts/bridge/BridgeState.sol

        // Time after which the redemption request can be reported as
        // timed out. It is counted from the moment when the redemption
        // request was created via `requestRedemption` call. Reported
        // timed out requests are cancelled and locked TBTC is returned
        // to the redeemer in full amount.
-        uint256 redemptionTimeout;
+        uint64 redemptionTimeout;


All other timeouts (movingFundsTimeout, movedFundsSweepTimeout, fraudChallengeDefeatTimeout) are uint32. We could also make redemptionTimeout uint32 for consistency. I think it does not break the layout we want to achieve because the next uint96 will not fit into the 10th storage slot anyway.

Addressed in #345 in 9d15d6e.

pdyraga · 2022-05-26T08:42:11Z

solidity/contracts/bridge/BridgeState.sol

@@ -129,14 +119,7 @@ library BridgeState {
        // The percentage of the notifier reward from the staking contract


We could change movingFundsTimeoutNotifierRewardMultiplier into uint32 and add bytes16 gap. That would merge:
Slot 7: uint96 movingFundsTimeoutSlashingAmount
Slot 8: uint256 movingFundsTimeoutNotifierRewardMultiplier

together into one slot:

Slot 7: uint96 movingFundsTimeoutSlashingAmount | uint32 movingFundsTimeoutNotifierRewardMultiplier | bytes16 movingFundsTimeoutSlashingGap

Furthermore, we could change movedFundsSweepTimeoutNotifierRewardMultiplier to uint32. It's not going to destroy the layout of 9th slot. We could also change redemptionTimeoutNotifierRewardMultiplier to uint32 because it's not going to destroy the layout of 11th slot. And this way, all *RewardMultiplier* fields would be uint32 consistently.

Addressed in #345 in 9d15d6e.

pdyraga · 2022-05-26T09:39:46Z

solidity/contracts/bridge/Redemption.sol

+    /// @notice Calculate redemption key without allocations.
+    /// @param walletPubKeyHash the pubkey hash of the wallet.
+    /// @param script the output script of the redemption.
+    /// @return The key = keccak256(keccak256(script), walletPubKeyHash).


We need to adjust all occurrences of keccak256(walletPubKeyHash | redeemerOutputScript) in the documentation when we talk about pendingRedemptions or timedOutRedemptions.

Addressed in #345 in 73dee0b.

pdyraga · 2022-05-26T09:58:52Z

solidity/contracts/bridge/Redemption.sol

+            /* solhint-disable-next-line no-inline-assembly */
+            assembly {
+                outputScriptHash := keccak256(
+                    add(redemptionTxOutputVector, add(outputScriptStart, 32)),


Can we explain 32 in a comment? My understanding of this line is:

The first argument to assembly keccak256 is the pointer. We point to redemptionTxOutputVector but at the position indicated by outputScriptStart. To load that position, we need to call add(outputScriptStart, 32) because outputScriptStart has 32 bytes.

Addressed in #345 in 01b490c.

pdyraga · 2022-05-26T10:15:12Z

solidity/contracts/bridge/Redemption.sol

@@ -783,24 +782,33 @@ library Redemption {
            //       https://github.com/keep-network/tbtc-v2/issues/257


This TODO can now be removed.

Addressed in #345 in 7082d42.

pdyraga · 2022-05-26T10:15:41Z

solidity/contracts/bridge/Redemption.sol

+                processInfo.outputStartingIndex
+            );
+
+            uint256 scriptLength = outputLength - 8;


We could move this line below the comment explaining 8-byte value and variable length script.

Addressed in #345 in 5531f8f.

pdyraga · 2022-05-26T10:17:50Z

solidity/contracts/bridge/Redemption.sol

-        // This function should be called only if the given output is
-        // supposed to represent a redemption. Build the redemption key
-        // to perform that check.


We could add

/// @dev This function should be called only if the given output is supposed to represent redemption.

to the documentation of this function.

Addressed in #345 in f6a1740.

The detector was complaining about calls to bitcoin-spv which is a trusted library.

eth-r added 14 commits May 12, 2022 15:48

Improve script identification in extractPubKeyHash and add utility fu…

15d4347

…nctions

Remove unnecessary allocation from processRedemptionTxOutputs

46975d6

Use extractHashAt

22d22d8

Simplify script payload check in requestRedemption

94c7929

Change redemption key calculation to non-allocating

fc54959

Align storage slots in BridgeState to streamline deposits

9282004

Align BridgeState to streamline redemption operations

1794b5c

Align BridgeState to streamline fraud handling

d062d5b

Add alignment gap for redemption

5aedcc0

Align BridgeState to streamline moving funds

00fa2e8

Align BridgeState to streamline relay use

8934e9c

Remove allocations from submitRedemptionProof

299d696

Merge branch 'main' into bridge-opt

f3168fa

Build scripts in processRedemptionTxOutputs more efficiently

4043e38

Merge branch 'main' into bridge-opt

b606672

pdyraga assigned eth-r May 20, 2022

pdyraga added the ⛓️ solidity Solidity contracts label May 20, 2022

pdyraga added this to the solidity/v1.0.0 milestone May 20, 2022

eth-r added 2 commits May 25, 2022 15:22

Merge branch 'main' into bridge-opt

d8d15db

Update bitcoin-spv version

6c23186

eth-r marked this pull request as ready for review May 25, 2022 14:27

pdyraga and others added 6 commits May 25, 2022 17:04

Updated lockfile for 3.4.0-solc-0.8 of bitcoin-spv-sol

6e8c105

Merge branch 'main' into bridge-opt

b8091d2

Merge remote-tracking branch 'origin/bridge-opt' into bridge-opt

2da7683

Update redemption key calculations in vault tests

d5f4ccf

Using keccak256(keccak256(script), walletPubKeyHash) gives higher performance in some circumstances. Update the key calculation method in tests to match what is implemented in the contracts.

Fix linting issues

d38ba95

Actually fix formatting issues

2027551

tomaszslabon reviewed May 25, 2022

View reviewed changes

solidity/contracts/bridge/Redemption.sol Outdated Show resolved Hide resolved

tomaszslabon reviewed May 25, 2022

View reviewed changes

solidity/contracts/bridge/BridgeState.sol Show resolved Hide resolved

tomaszslabon reviewed May 25, 2022

View reviewed changes

solidity/contracts/bridge/BridgeState.sol Show resolved Hide resolved

eth-r added 2 commits May 25, 2022 18:06

Explain BridgeState alignment gaps in comments

7cae2a8

Fix function documentation comment formatting

f4ff905

tomaszslabon previously approved these changes May 26, 2022

View reviewed changes

pdyraga mentioned this pull request May 26, 2022

Bridge gas optimisations 2 #345

Merged

pdyraga previously approved these changes May 26, 2022

View reviewed changes

Ignore reentrancy-events detector in requestRedemption

ab98d8b

The detector was complaining about calls to bitcoin-spv which is a trusted library.

pdyraga dismissed stale reviews from tomaszslabon and themself via ab98d8b May 26, 2022 13:08

Merge branch 'main' into bridge-opt

617d2e4

pdyraga approved these changes May 26, 2022

View reviewed changes

tomaszslabon approved these changes May 26, 2022

View reviewed changes

tomaszslabon merged commit f6402dd into main May 26, 2022

tomaszslabon deleted the bridge-opt branch May 26, 2022 13:18

pdyraga mentioned this pull request May 27, 2022

Update construction of redemption key in Typescript library #347

Closed

pdyraga unassigned eth-r Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bridge gas optimisations #301

Bridge gas optimisations #301

eth-r commented May 17, 2022 •

edited by pdyraga

Loading

pdyraga commented May 18, 2022

pdyraga commented May 18, 2022

eth-r commented May 18, 2022 •

edited

Loading

pdyraga commented May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

pdyraga May 26, 2022

		@@ -129,14 +119,7 @@ library BridgeState {
		// The percentage of the notifier reward from the staking contract

		@@ -783,24 +782,33 @@ library Redemption {
		// https://github.com/keep-network/tbtc-v2/issues/257

Bridge gas optimisations #301

Bridge gas optimisations #301

Conversation

eth-r commented May 17, 2022 • edited by pdyraga Loading

pdyraga commented May 18, 2022

pdyraga commented May 18, 2022

eth-r commented May 18, 2022 • edited Loading

pdyraga commented May 26, 2022

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eth-r commented May 17, 2022 •

edited by pdyraga

Loading

eth-r commented May 18, 2022 •

edited

Loading