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

feat: decoder return l1<->l2 msgs #597

Merged
merged 6 commits into from
May 17, 2023
Merged

feat: decoder return l1<->l2 msgs #597

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2da9899
feat: decoder return l1<->l2 msgs
LHerskind May 16, 2023
9358979
chore: forge fmt
LHerskind May 16, 2023
567866f
fix: address comments
LHerskind May 17, 2023
cc1e3a9
fix: test clarity nit
LHerskind May 17, 2023
1826835
fix: address nits
LHerskind May 17, 2023
6e29297
fix: improve consistency on messages vs msgs
LHerskind May 17, 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
53 changes: 37 additions & 16 deletions l1-contracts/src/core/Decoder.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,23 +89,29 @@ contract Decoder {
uint256 l2BlockNumber,
bytes32 startStateHash,
bytes32 endStateHash,
bytes32 publicInputHash
bytes32 publicInputHash,
bytes32[] memory l2ToL1Msgs,
bytes32[] memory l1ToL2Msgs
)
{
l2BlockNumber = _getL2BlockNumber(_l2Block);
// Note, for startStateHash to match the storage, the l2 block number must be new - 1.
// Only jumping 1 block at a time.
startStateHash = _computeStateHash(l2BlockNumber - 1, 0x4, _l2Block);
endStateHash = _computeStateHash(l2BlockNumber, 0x120, _l2Block);
publicInputHash = _computePublicInputsHash(_l2Block);
(publicInputHash, l2ToL1Msgs, l1ToL2Msgs) = _computePublicInputsHash(_l2Block);
}

/**
* Computes a hash of the public inputs from the calldata
* @param _l2Block - The L2 block calldata.
* @return sha256(header[0x4: 0x23c], diffRoot, l1Tol2MessagesHash)
*/
function _computePublicInputsHash(bytes calldata _l2Block) internal pure returns (bytes32) {
function _computePublicInputsHash(bytes calldata _l2Block)
Copy link
Member

Choose a reason for hiding this comment

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

The name of this doesnt make sense anymore if its also returning the message arrays. Comment natspec also needs updated

internal
pure
returns (bytes32, bytes32[] memory l2ToL1Msgs, bytes32[] memory l1ToL2Msgs)
Copy link
Member

Choose a reason for hiding this comment

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

some named return values while others arent

{
// header size - block number size + one value for the diffRoot + one value for l1ToL2MessagesHash
uint256 size = 0x23c - 0x04 + 0x20 + 0x20;

Expand All @@ -115,15 +121,16 @@ contract Decoder {
calldatacopy(add(temp, 0x20), add(_l2Block.offset, 0x04), size)
}

// Diff root
(bytes32 diffRoot, bytes32 l1ToL2messagesHash) = _computeDiffRootAndMessagesHash(_l2Block);
bytes32 diffRoot;
bytes32 l1ToL2messagesHash;
(diffRoot, l2ToL1Msgs, l1ToL2Msgs, l1ToL2messagesHash) = _computeConsumables(_l2Block);
assembly {
let endOfTreesData := sub(0x23c, 0x04)
mstore(add(temp, add(0x20, endOfTreesData)), diffRoot)
mstore(add(temp, add(0x40, endOfTreesData)), l1ToL2messagesHash)
}

return bytes32(uint256(sha256(temp)) % P);
return (bytes32(uint256(sha256(temp)) % P), l2ToL1Msgs, l1ToL2Msgs);
Copy link
Member

Choose a reason for hiding this comment

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

may as well make the mod p variable named on its own then you can spare the return statement. Having a calc in the return statement while others are pre calculated is a bit messy can we be consistent?

}

/**
Expand Down Expand Up @@ -185,13 +192,17 @@ contract Decoder {
}

/**
* @notice Creates a "diff" tree and compute its root
* @notice Computes the consumables for the block
* @param _l2Block - The L2 block calldata.
* @return diffRoot - The root of the diff tree (new commitments, nullifiers etc)
* @return l2ToL1Msgs - The L2 to L1 messages of the block
* @return l1ToL2Msgs - The L1 to L2 messages of the block
* @return l1ToL2messagesHash - The hash of the L1 to L2 messages
*/
function _computeDiffRootAndMessagesHash(bytes calldata _l2Block)
function _computeConsumables(bytes calldata _l2Block)
internal
pure
returns (bytes32, bytes32)
returns (bytes32, bytes32[] memory, bytes32[] memory, bytes32)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
returns (bytes32, bytes32[] memory, bytes32[] memory, bytes32)
returns (bytes32, bytes32, bytes32[] memory, bytes32[] memory)

id probably prefer the hashes to be together and the arrays together

{
// Find the lengths of the different inputs
// TOOD: Naming / getting the messages root within this function is a bit weird
Expand Down Expand Up @@ -225,6 +236,9 @@ contract Decoder {
bytes32[] memory baseLeafs = new bytes32[](
lengths.commitmentCount / (COMMITMENTS_PER_KERNEL * 2)
);
bytes32[] memory l2ToL1Msgs = new bytes32[](
lengths.l2ToL1MessagesCount
);

// Data starts after header. Look at L2 Block Data specification at the top of this file.
{
Expand All @@ -236,6 +250,15 @@ contract Decoder {
offsets.contractDataOffset = offsets.contractOffset + lengths.contractCount * 0x20;
offsets.l1ToL2MessagesOffset = offsets.contractDataOffset + 0x4 + lengths.contractCount * 0x34;

// load the l2 to l1 msgs
assembly {
Copy link
Member

Choose a reason for hiding this comment

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

Can this array copy be moved down to where the other one is performed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm using that we just computer the offset values. We will be updating them every loop so was a nice way to have it right away.

Copy link
Member

Choose a reason for hiding this comment

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

understood

calldatacopy(
add(l2ToL1Msgs, 0x20),
add(_l2Block.offset, mload(add(offsets, 0x60))),
mul(mload(add(lengths, 0x60)), 0x20)
)
}

for (uint256 i = 0; i < baseLeafs.length; i++) {
/**
* Compute the leaf to insert.
Expand Down Expand Up @@ -341,25 +364,23 @@ contract Decoder {
}

bytes32 diffRoot = _computeRoot(baseLeafs);

bytes32[] memory l1ToL2Messages;
bytes32 messagesHash;
{
uint256 messagesHashPreimageSize = 0x20 * L1_TO_L2_MESSAGES_PER_ROLLUP;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Should messagesHashPreimageSize be renamed to L1ToL2MesssagesHashPreimageSize for consistency?
Same for messagesHash. It is unclear to me if it is for L1toL2 or L2toL1 or does it also include other kind of messages. In the natspec, you call this L1ToL2MessagesHash

bytes memory messagesHashPreimage = new bytes(
messagesHashPreimageSize
);
l1ToL2Messages = new bytes32[](L1_TO_L2_MESSAGES_PER_ROLLUP);
assembly {
calldatacopy(
add(messagesHashPreimage, 0x20),
add(l1ToL2Messages, 0x20),
add(_l2Block.offset, mload(add(offsets, 0xc0))),
messagesHashPreimageSize
)
}

messagesHash = sha256(messagesHashPreimage);
messagesHash = sha256(abi.encodePacked(l1ToL2Messages));
}

return (diffRoot, messagesHash);
return (diffRoot, l2ToL1Msgs, l1ToL2Messages, messagesHash);
}

/**
Expand Down
10 changes: 8 additions & 2 deletions l1-contracts/src/core/Rollup.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,14 @@ contract Rollup is Decoder {
* @param _l2Block - The L2Block data, formatted as outlined in `Decoder.sol`
*/
function process(bytes memory _proof, bytes calldata _l2Block) external {
(uint256 l2BlockNumber, bytes32 oldStateHash, bytes32 newStateHash, bytes32 publicInputHash) =
_decode(_l2Block);
(
uint256 l2BlockNumber,
bytes32 oldStateHash,
bytes32 newStateHash,
bytes32 publicInputHash,
bytes32[] memory l2ToL1Msgs,
bytes32[] memory l1ToL2Msgs
) = _decode(_l2Block);

// @todo Proper genesis state. If the state is empty, we allow anything for now.
if (rollupStateHash != bytes32(0) && rollupStateHash != oldStateHash) {
Expand Down
36 changes: 32 additions & 4 deletions l1-contracts/test/Decoder.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,14 @@ contract DecoderTest is Test {
"Invalid messages hash"
);

(uint256 l2BlockNumber, bytes32 startStateHash, bytes32 endStateHash, bytes32 publicInputsHash)
= helper.decode(block_empty_1);
(
uint256 l2BlockNumber,
bytes32 startStateHash,
bytes32 endStateHash,
bytes32 publicInputsHash,
bytes32[] memory l2ToL1Msgs,
bytes32[] memory l1ToL2Msgs
) = helper.decode(block_empty_1);

assertEq(l2BlockNumber, 1, "Invalid block number");
assertEq(
Expand All @@ -64,6 +70,13 @@ contract DecoderTest is Test {

rollup.process(bytes(""), block_empty_1);

for (uint256 i = 0; i < l2ToL1Msgs.length; i++) {
assertEq(l2ToL1Msgs[i], bytes32(0), "Invalid l2ToL1Msgs");
}
for (uint256 i = 0; i < l1ToL2Msgs.length; i++) {
assertEq(l1ToL2Msgs[i], bytes32(0), "Invalid l1ToL2Msgs");
}

assertEq(rollup.rollupStateHash(), endStateHash, "Invalid rollup state hash");
}

Expand All @@ -82,8 +95,14 @@ contract DecoderTest is Test {
"Invalid messages hash"
);

(uint256 l2BlockNumber, bytes32 startStateHash, bytes32 endStateHash, bytes32 publicInputsHash)
= helper.decode(block_mixed_1);
(
uint256 l2BlockNumber,
bytes32 startStateHash,
bytes32 endStateHash,
bytes32 publicInputsHash,
bytes32[] memory l2ToL1Msgs,
bytes32[] memory l1ToL2Msgs
) = helper.decode(block_mixed_1);

assertEq(l2BlockNumber, 1, "Invalid block number");
assertEq(
Expand All @@ -102,6 +121,15 @@ contract DecoderTest is Test {
"Invalid public input hash"
);

for (uint256 i = 0; i < l2ToL1Msgs.length; i++) {
assertEq(
l2ToL1Msgs[i], bytes32(uint256(0x300 + 32 * (1 + i / 2) + i % 2)), "Invalid l2ToL1Msgs"
Copy link
Member

@Maddiaa0 Maddiaa0 May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

can you lift the value calc up a line so its a bit more readable

(ps. bruh we gotta automate test generation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be adding the test in the integration test for L1 publisher when adding in rollup, then tested in there for the automatic.
For our use, even if I like foundry more, TS seems like a better fit to have the tests because we will have so much encoding and stuff to deal with 😬 I only really see our foundry tests right now as useful for better debugging when building and for checking stuff without all the encoding/decoding.

);
}
for (uint256 i = 0; i < l1ToL2Msgs.length; i++) {
assertEq(l1ToL2Msgs[i], bytes32(uint256(0x401 + i)), "Invalid l1ToL2Msgs");
}

rollup.process(bytes(""), block_mixed_1);

assertEq(rollup.rollupStateHash(), endStateHash, "Invalid rollup state hash");
Expand Down
4 changes: 2 additions & 2 deletions l1-contracts/test/DecoderHelper.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ contract DecoderHelper is Decoder {
function decode(bytes calldata _l2Block)
external
pure
returns (uint256, bytes32, bytes32, bytes32)
returns (uint256, bytes32, bytes32, bytes32, bytes32[] memory, bytes32[] memory)
{
return _decode(_l2Block);
}
Expand All @@ -19,7 +19,7 @@ contract DecoderHelper is Decoder {
pure
returns (bytes32, bytes32)
{
(bytes32 diffRoot, bytes32 l1ToL2MessagesHash) = _computeDiffRootAndMessagesHash(_l2Block);
(bytes32 diffRoot,,, bytes32 l1ToL2MessagesHash) = _computeConsumables(_l2Block);
return (diffRoot, l1ToL2MessagesHash);
}
}