Synclayer Batch Aggregation #470
Conversation
|
|
||
| interface IBatchAggregator { | ||
| function commitBatch( | ||
| bytes calldata _totalL2ToL1PubdataAndStateDiffs, |
There was a problem hiding this comment.
why do we need to send both compressed diffs and their uncompressed version? I think we should decide on what we want. I would rather send only uncompressed ones
| // Add batch to BatchAggregator | ||
| IBatchAggregator(BATCH_AGGREGATOR_ADDRESS).commitBatch( | ||
| _newBatch.pubdataCommitments, | ||
| 0, |
There was a problem hiding this comment.
just in case, s.chainId is available in storage
|
|
||
| numInitialWritesProcessed++; | ||
| require(derivedKey == compressedStateDiffs.readBytes32(stateDiffPtr), "iw: initial key mismatch"); | ||
| if (keyStatus[chainId][derivedKey] == false) { |
There was a problem hiding this comment.
I understand it is draft, just in case better to rename to isKeyTouched
| bytes memory compressedStateDiff = new bytes(touchedKeys[chainId].length * COMPRESSED_STATE_DIFF_SIZE); | ||
| uint256 stateDiffPtr = 0; | ||
| for (uint256 keyIndex = 0; keyIndex < touchedKeys[chainId].length; keyIndex += 1) { | ||
| bytes32 derivedKey = touchedKeys[chainId][keyIndex]; |
There was a problem hiding this comment.
repeated writes should be encoded as pair (enum_index, value)
This reverts commit 0d760f7.
!!!! mload, mstore need to be handled in the next commit
| uint256 compressedStateDiffSize = 0; | ||
| initialWritesPtr = 0; | ||
| for(uint256 i = 0;i<numberOfInitialWrites*STATE_DIFF_ENTRY_SIZE;i+=STATE_DIFF_ENTRY_SIZE){ | ||
| bytes memory stateDiff = initialWrites[chainId][initialWritesPtr]; |
There was a problem hiding this comment.
just in case, does your code work fine with the same key used for both initial write and repeated write? I.e. in the first batch it is an initial write and it is a repeated write in another one
There was a problem hiding this comment.
I break it down into two different write instances. That is done because I haven't found a way to derive last enumIndex, and thus know enumIndex for initial writes.
| function addRepeatedWrite(uint256 chainId, bytes calldata stateDiff, bytes calldata compressedStateDiff) internal{ | ||
| uint64 enumIndex = stateDiff.readUint64(84); | ||
| if (slotStatus[chainId][enumIndex]==false){ | ||
| uncompressedWrites[chainId][enumIndex] = stateDiff; |
There was a problem hiding this comment.
why do we store the entire state diff? not too bad, but I think storing just initialValue,finalValue,enumIndex will be much easier to read
|
|
||
| function compressValue(uint256 initialValue, uint256 finalValue) internal returns (bytes memory compressedDiff){ | ||
| uint8 transform = bytesLength(finalValue); | ||
| uint8 add = bytesLength(finalValue-initialValue); |
There was a problem hiding this comment.
this will panic if finalValue < initialValue. please use unchecked
| function compressValue(uint256 initialValue, uint256 finalValue) internal returns (bytes memory compressedDiff){ | ||
| uint8 transform = bytesLength(finalValue); | ||
| uint8 add = bytesLength(finalValue-initialValue); | ||
| uint8 sub = bytesLength(initialValue-finalValue); |
| uint8 transform = bytesLength(finalValue); | ||
| uint8 add = bytesLength(finalValue-initialValue); | ||
| uint8 sub = bytesLength(initialValue-finalValue); | ||
| uint8 opt = (transform<add?transform:add); |
There was a problem hiding this comment.
please use extended variable names opt -> optimal
| import {UnsafeBytesCalldata} from "./libraries/UnsafeBytesCalldata.sol"; | ||
| import {ICompressor, OPERATION_BITMASK, LENGTH_BITS_OFFSET, MAX_ENUMERATION_INDEX_SIZE} from "./interfaces/ICompressor.sol"; | ||
|
|
||
| uint256 constant BYTE_BITMASK = (1<<8)-1; |
There was a problem hiding this comment.
type(uint8).max
| value = initialValue-finalValue; | ||
| } | ||
| else{ | ||
| require(false, "Optimal operation is not transform, add or sub"); |
There was a problem hiding this comment.
there are four types of compression:
/// 0 -> Nothing (32 bytes)
/// 1 -> Add
/// 2 -> Subtract
/// 3 -> Transform (< 32 bytes)
I am not sure this will work when the size of the change is 32 bytes
There was a problem hiding this comment.
It can't be 32 bytes. At most 2^32/2 = 2^31, because of add or subtract
There was a problem hiding this comment.
32 is bytes, not bits, i.e. 2^256 / 2 = 2^255
| uint256 messageSliceStart = calldataPtr; | ||
| uint32 numberOfMessages = uint32(bytes4(_totalL2ToL1PubdataAndStateDiffs[calldataPtr:calldataPtr + 4])); | ||
| calldataPtr += 4; | ||
| bytes32 reconstructedChainedMessagesHash; |
| stateDiffPtr += 32; | ||
| uint8 metadata = uint8(bytes1(_compressedStateDiffs[stateDiffPtr])); | ||
| stateDiffPtr++; | ||
| uint8 operation = metadata & OPERATION_BITMASK; |
| function addInitialWrite(uint256 chainId, bytes32 derivedKey, uint64 enumIndex, uint256 initialValue, uint256 finalValue) internal{ | ||
| bytes memory slotData = new bytes(STATE_DIFF_AGGREGATION_INFO_SIZE); | ||
| assembly{ | ||
| mstore(add(slotData,0),derivedKey) |
There was a problem hiding this comment.
I would really suggest to store those as separate mappings to increase readability.
Or even, better, a single mapping, but with a struct inside. There is little benefit in using bytes here
| } | ||
| uint8 diffPtr = optimal; | ||
| for(uint8 i = 0;i<optimal;i+=1){ | ||
| compressedDiff[diffPtr] = bytes1(uint8(value & type(uint8).max)); |
There was a problem hiding this comment.
this value doesnt change
What ❔
Implement batch forwarding from hyperchains to lower layer and compression for pubdata.
Why ❔
To allow deployment of hyperchains on top of synclayer and compress reused storage slots.