Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bridge gas optimisations #301

Merged
merged 27 commits into from
May 26, 2022
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
15d4347
Improve script identification in extractPubKeyHash and add utility fu…
eth-r May 12, 2022
46975d6
Remove unnecessary allocation from processRedemptionTxOutputs
eth-r May 12, 2022
22d22d8
Use extractHashAt
eth-r May 12, 2022
94c7929
Simplify script payload check in requestRedemption
eth-r May 12, 2022
fc54959
Change redemption key calculation to non-allocating
eth-r May 12, 2022
9282004
Align storage slots in BridgeState to streamline deposits
eth-r May 12, 2022
1794b5c
Align BridgeState to streamline redemption operations
eth-r May 12, 2022
d062d5b
Align BridgeState to streamline fraud handling
eth-r May 12, 2022
5aedcc0
Add alignment gap for redemption
eth-r May 12, 2022
00fa2e8
Align BridgeState to streamline moving funds
eth-r May 12, 2022
8934e9c
Align BridgeState to streamline relay use
eth-r May 12, 2022
299d696
Remove allocations from submitRedemptionProof
eth-r May 17, 2022
f3168fa
Merge branch 'main' into bridge-opt
eth-r May 17, 2022
4043e38
Build scripts in processRedemptionTxOutputs more efficiently
eth-r May 17, 2022
b606672
Merge branch 'main' into bridge-opt
eth-r May 18, 2022
d8d15db
Merge branch 'main' into bridge-opt
eth-r May 25, 2022
6c23186
Update bitcoin-spv version
eth-r May 25, 2022
6e8c105
Updated lockfile for 3.4.0-solc-0.8 of bitcoin-spv-sol
pdyraga May 25, 2022
b8091d2
Merge branch 'main' into bridge-opt
eth-r May 25, 2022
2da7683
Merge remote-tracking branch 'origin/bridge-opt' into bridge-opt
eth-r May 25, 2022
d5f4ccf
Update redemption key calculations in vault tests
eth-r May 25, 2022
d38ba95
Fix linting issues
eth-r May 25, 2022
2027551
Actually fix formatting issues
eth-r May 25, 2022
7cae2a8
Explain BridgeState alignment gaps in comments
eth-r May 25, 2022
f4ff905
Fix function documentation comment formatting
eth-r May 25, 2022
ab98d8b
Ignore reentrancy-events detector in requestRedemption
pdyraga May 26, 2022
617d2e4
Merge branch 'main' into bridge-opt
pdyraga May 26, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 72 additions & 39 deletions solidity/contracts/bridge/BitcoinTx.sol
Original file line number Diff line number Diff line change
Expand Up @@ -278,49 +278,82 @@ library BitcoinTx {

pubKeyHash = pubKeyHashBytes.slice20(0);

// We need to make sure that the 20-byte public key hash
// is actually used in the right context of a P2PKH or P2WPKH
// output. To do so, we must extract the full script from the output
// and compare with the expected P2PKH and P2WPKH scripts
// referring to that 20-byte public key hash. The output consists
// of an 8-byte value and a variable length script. To extract the
// script we slice the output starting from 9th byte until the end.
bytes32 outputScriptKeccak = keccak256(
output.slice(8, output.length - 8)
);
// Build the expected P2PKH script which has the following byte
// format: <0x1976a914> <20-byte PKH> <0x88ac>. According to
// https://en.bitcoin.it/wiki/Script#Opcodes this translates to:
// - 0x19: Byte length of the entire script
// - 0x76: OP_DUP
// - 0xa9: OP_HASH160
// - 0x14: Byte length of the public key hash
// - 0x88: OP_EQUALVERIFY
// - 0xac: OP_CHECKSIG
// which matches the P2PKH structure as per:
// https://en.bitcoin.it/wiki/Transaction#Pay-to-PubkeyHash
bytes32 P2PKHScriptKeccak = keccak256(
abi.encodePacked(hex"1976a914", pubKeyHash, hex"88ac")
);
// Build the expected P2WPKH script which has the following format:
// <0x160014> <20-byte PKH>. According to
// https://en.bitcoin.it/wiki/Script#Opcodes this translates to:
// - 0x16: Byte length of the entire script
// - 0x00: OP_0
// - 0x14: Byte length of the public key hash
// which matches the P2WPKH structure as per:
// https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#P2WPKH
bytes32 P2WPKHScriptKeccak = keccak256(
abi.encodePacked(hex"160014", pubKeyHash)
);
// Make sure the actual output script matches either the P2PKH
// or P2WPKH format.
uint256 scriptLen = output.length - 8;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #345 in 9aeb58c.


// The P2PKH script is 26 bytes long.
// The P2WPKH script is 23 bytes long.
// A valid script must have one of these lengths,
// and we can identify the expected script type by the length.
require(
outputScriptKeccak == P2PKHScriptKeccak ||
outputScriptKeccak == P2WPKHScriptKeccak,
scriptLen == 26 || scriptLen == 23,
"Output must be P2PKH or P2WPKH"
);

if (scriptLen == 26) {
// Compare to the expected P2PKH script.
bytes26 script = bytes26(output.slice32(8));

require(
script == makeP2PKHScript(pubKeyHash),
"Invalid P2PKH script"
);
}

if (scriptLen == 23) {
// Compare to the expected P2WPKH script.
bytes23 script = bytes23(output.slice32(8));

require(
script == makeP2WPKHScript(pubKeyHash),
"Invalid P2WPKH script"
);
}

return pubKeyHash;
}

/// @notice Build the P2PKH script from the given public key hash.
/// @param pubKeyHash The 20-byte public key hash
/// @return The P2PKH script
/// @dev The P2PKH script has the following byte format:
/// <0x1976a914> <20-byte PKH> <0x88ac>. According to
/// https://en.bitcoin.it/wiki/Script#Opcodes this translates to:
/// - 0x19: Byte length of the entire script
/// - 0x76: OP_DUP
/// - 0xa9: OP_HASH160
/// - 0x14: Byte length of the public key hash
/// - 0x88: OP_EQUALVERIFY
/// - 0xac: OP_CHECKSIG
/// which matches the P2PKH structure as per:
/// https://en.bitcoin.it/wiki/Transaction#Pay-to-PubkeyHash
function makeP2PKHScript(bytes20 pubKeyHash)
internal
pure
returns (bytes26)
{
bytes26 P2PKHScriptMask = hex"1976a914000000000000000000000000000000000000000088ac";

return ((bytes26(pubKeyHash) >> 32) | P2PKHScriptMask);
}

/// @notice Build the P2WPKH script from the given public key hash.
/// @param pubKeyHash The 20-byte public key hash
/// @return The P2WPKH script
/// @dev The P2WPKH script has the following format:
/// <0x160014> <20-byte PKH>. According to
/// https://en.bitcoin.it/wiki/Script#Opcodes this translates to:
/// - 0x16: Byte length of the entire script
/// - 0x00: OP_0
/// - 0x14: Byte length of the public key hash
/// which matches the P2WPKH structure as per:
/// https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#P2WPKH
function makeP2WPKHScript(bytes20 pubKeyHash)
internal
pure
returns (bytes23)
{
bytes23 P2WPKHScriptMask = hex"1600140000000000000000000000000000000000000000";

return ((bytes23(pubKeyHash) >> 24) | P2WPKHScriptMask);
}
}
24 changes: 12 additions & 12 deletions solidity/contracts/bridge/Bridge.sol
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ contract Bridge is
address _relay,
address _treasury,
address _ecdsaWalletRegistry,
uint256 _txProofDifficultyFactor
uint96 _txProofDifficultyFactor
) external initializer {
require(_bank != address(0), "Bank address cannot be zero");
self.bank = Bank(_bank);
Expand Down Expand Up @@ -1228,9 +1228,9 @@ contract Bridge is
uint64 redemptionDustThreshold,
uint64 redemptionTreasuryFeeDivisor,
uint64 redemptionTxMaxFee,
uint256 redemptionTimeout,
uint64 redemptionTimeout,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to revisit events because they still use old types. Example:


    event RedemptionParametersUpdated(
        uint64 redemptionDustThreshold,
        uint64 redemptionTreasuryFeeDivisor,
        uint64 redemptionTxMaxFee,
        uint256 redemptionTimeout,
        uint96 redemptionTimeoutSlashingAmount,
        uint256 redemptionTimeoutNotifierRewardMultiplier
    );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #345 in a3716d0.

uint96 redemptionTimeoutSlashingAmount,
uint256 redemptionTimeoutNotifierRewardMultiplier
uint64 redemptionTimeoutNotifierRewardMultiplier
) external onlyGovernance {
self.updateRedemptionParameters(
redemptionDustThreshold,
Expand Down Expand Up @@ -1315,7 +1315,7 @@ contract Bridge is
uint64 movedFundsSweepTxMaxTotalFee,
uint32 movedFundsSweepTimeout,
uint96 movedFundsSweepTimeoutSlashingAmount,
uint256 movedFundsSweepTimeoutNotifierRewardMultiplier
uint64 movedFundsSweepTimeoutNotifierRewardMultiplier
) external onlyGovernance {
self.updateMovingFundsParameters(
movingFundsTxMaxTotalFee,
Expand Down Expand Up @@ -1395,10 +1395,10 @@ contract Bridge is
/// - Fraud challenge defeat timeout must be greater than 0,
/// - Fraud notifier reward multiplier must be in the range [0, 100].
function updateFraudParameters(
uint256 fraudChallengeDepositAmount,
uint256 fraudChallengeDefeatTimeout,
uint96 fraudChallengeDepositAmount,
uint32 fraudChallengeDefeatTimeout,
uint96 fraudSlashingAmount,
uint256 fraudNotifierRewardMultiplier
uint32 fraudNotifierRewardMultiplier
) external onlyGovernance {
self.updateFraudParameters(
fraudChallengeDepositAmount,
Expand Down Expand Up @@ -1607,9 +1607,9 @@ contract Bridge is
uint64 redemptionDustThreshold,
uint64 redemptionTreasuryFeeDivisor,
uint64 redemptionTxMaxFee,
uint256 redemptionTimeout,
uint64 redemptionTimeout,
uint96 redemptionTimeoutSlashingAmount,
uint256 redemptionTimeoutNotifierRewardMultiplier
uint64 redemptionTimeoutNotifierRewardMultiplier
)
{
redemptionDustThreshold = self.redemptionDustThreshold;
Expand Down Expand Up @@ -1748,10 +1748,10 @@ contract Bridge is
external
view
returns (
uint256 fraudChallengeDepositAmount,
uint256 fraudChallengeDefeatTimeout,
uint96 fraudChallengeDepositAmount,
uint32 fraudChallengeDefeatTimeout,
uint96 fraudSlashingAmount,
uint256 fraudNotifierRewardMultiplier
uint32 fraudNotifierRewardMultiplier
)
{
fraudChallengeDepositAmount = self.fraudChallengeDepositAmount;
Expand Down
Loading