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.

Sign up for GitHub

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

fix(protocol): Replace LibEthDeposit assembly #13781

Merged
merged 2 commits into from
May 19, 2023
Merged

fix(protocol): Replace LibEthDeposit assembly #13781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85eb87f
Replace LibEthDeposit assembly
adaki2004 May 18, 2023
d2030eb
Put back array resize assembly
adaki2004 May 18, 2023
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
13 changes: 11 additions & 2 deletions packages/protocol/contracts/L1/libs/LibEthDepositing.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,19 @@ library LibEthDepositing {
}
}

// resize the length of depositsProcessed to j.
assembly {
mstore(depositsProcessed, j)
depositsRoot := keccak256(depositsProcessed, mul(j, 32))
}
depositsRoot = hashDeposits(depositsProcessed);
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous hashing of depositsProcessed is buggy, but I think it can get fixed simply using:

 depositsRoot := keccak256(add(depositsProcessed, 32), mul(j, 32))

Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe the expected value in the test (0x8117066d69ff650d78f0d7383a10cc802c2b8c0eedd932d70994252e2438c636) is also calculated wrong. How is it calculated exactly, better to calculate the hash inside the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No you cannot calculate the has this way - indeed we need to chop the first 32 bytes because that’s the lenght, but the rest is just a pointer.
Then if you dump the data stired on that pointer is the address, and where the amount stored is somewhere else (maybe next bytes32 - but then it’s too complex to donwith assembly.I spent yestarrday 6 hours debugging assembly content i can try to make it somehow work with assembly but i dont know …

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a PR for the looping being done in assembly and keccak256 being done outside here Dani: https://github.com/taikoxyz/taiko-mono/pull/13782/files

Much less readable but saves some gas

}

function hashDeposits(TaikoData.EthDeposit[] memory deposits) internal pure returns (bytes32) {
bytes memory buffer;

for (uint256 i = 0; i < deposits.length; i++) {
buffer = abi.encodePacked(buffer, deposits[i].recipient, deposits[i].amount);
}

return keccak256(buffer);
}
}
45 changes: 45 additions & 0 deletions packages/protocol/test/TaikoL1.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,4 +247,49 @@ contract TaikoL1Test is TaikoL1TestBase {
// Not found
assertEq(bytes32(0), L1.getCrossChainSignalRoot((iterationCnt + 1)));
}

function test_deposit_hash_creation() external {
uint96 minAmount = conf.minEthDepositAmount;
uint96 maxAmount = conf.maxEthDepositAmount;

// We need 8 depostis otherwise we are not processing them !
depositTaikoToken(Alice, 1e6 * 1e8, maxAmount + 1 ether);
depositTaikoToken(Bob, 0, maxAmount + 1 ether);
depositTaikoToken(Carol, 0, maxAmount + 1 ether);
depositTaikoToken(Dave, 0, maxAmount + 1 ether);
depositTaikoToken(Eve, 0, maxAmount + 1 ether);
depositTaikoToken(Frank, 0, maxAmount + 1 ether);
depositTaikoToken(George, 0, maxAmount + 1 ether);
depositTaikoToken(Hilbert, 0, maxAmount + 1 ether);

// So after this point we have 8 deposits
vm.prank(Alice, Alice);
L1.depositEtherToL2{value: 1 ether}();
vm.prank(Bob, Bob);
L1.depositEtherToL2{value: 2 ether}();
vm.prank(Carol, Carol);
L1.depositEtherToL2{value: 3 ether}();
vm.prank(Dave, Dave);
L1.depositEtherToL2{value: 4 ether}();
vm.prank(Eve, Eve);
L1.depositEtherToL2{value: 5 ether}();
vm.prank(Frank, Frank);
L1.depositEtherToL2{value: 6 ether}();
vm.prank(George, George);
L1.depositEtherToL2{value: 7 ether}();
vm.prank(Hilbert, Hilbert);
L1.depositEtherToL2{value: 8 ether}();

assertEq(L1.getStateVariables().numEthDeposits, 8); // The number of deposits
assertEq(L1.getStateVariables().nextEthDepositToProcess, 0); // The index / cursos of the next deposit

// We shall invoke proposeBlock() because this is what will call the processDeposits()
TaikoData.BlockMetadata memory meta = proposeBlock(Alice, 1000000, 1024);

// Expected: 0x8117066d69ff650d78f0d7383a10cc802c2b8c0eedd932d70994252e2438c636 (pre calculated with these values)
//console2.logBytes32(meta.depositsRoot);
assertEq(
meta.depositsRoot, 0x8117066d69ff650d78f0d7383a10cc802c2b8c0eedd932d70994252e2438c636
);
}
}
3 changes: 3 additions & 0 deletions packages/protocol/test/TaikoL1TestBase.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ abstract contract TaikoL1TestBase is Test {
address public constant Carol = 0x300C9b60E19634e12FC6D68B7FEa7bFB26c2E419;
address public constant Dave = 0x400147C0Eb43D8D71b2B03037bB7B31f8f78EF5F;
address public constant Eve = 0x50081b12838240B1bA02b3177153Bca678a86078;
address public constant Frank = 0x430c9b60e19634e12FC6d68B7fEa7bFB26c2e419;
address public constant George = 0x520147C0eB43d8D71b2b03037bB7b31f8F78EF5f;
address public constant Hilbert = 0x61081B12838240B1Ba02b3177153BcA678a86078;

// Calculation shall be done in derived contracts - based on testnet or mainnet expected proof time
uint64 public initProofTimeIssued;
Expand Down