Safety Transaction Queue and State Commitment Chain#143
Conversation
K-Ho
changed the title
ben-chain
left a comment
ben-chain
left a comment
There was a problem hiding this comment.
This is awesome!!! Found only one critical component left to add, and a few minor ways that could improve tests, but this is 🔥
| dt.TimestampedHash memory l1ToL2Header = l1ToL2Queue.peek(); | ||
| if(!safetyQueue.isEmpty()) { | ||
| require(l1ToL2Header.timestamp <= safetyQueue.peekTimestamp(), "Must process older SafetyQueue batches first to enforce timestamp monotonicity"); | ||
| } | ||
| _appendQueueBatch(l1ToL2Header, true); | ||
| l1ToL2Queue.dequeue(); | ||
| } | ||
|
|
||
| function appendSafetyBatch() public { | ||
| dt.TimestampedHash memory safetyHeader = safetyQueue.peek(); | ||
| if(!l1ToL2Queue.isEmpty()) { | ||
| require(safetyHeader.timestamp <= l1ToL2Queue.peekTimestamp(), "Must process older L1ToL2Queue batches first to enforce timestamp monotonicity"); | ||
| } | ||
| _appendQueueBatch(safetyHeader, false); | ||
| safetyQueue.dequeue(); | ||
| } |
There was a problem hiding this comment.
I believe we are missing some sequencing authentication logic here--looks like these functions can be called by anyone right now, however they should not be callable by non-sequencers unless peekTimestamp() shows they are sufficiently old.
There was a problem hiding this comment.
Oh whoops you can ignore this, forgot this authentication is handled by _appendQueueBatch!! Nice!
| l1ToL2Queue.dequeue(); | ||
| } | ||
|
|
||
| function appendTransactionBatch(bytes[] memory _txBatch, uint _timestamp) public { |
There was a problem hiding this comment.
nit which I previously missed in review: I think appendSequencerTransactionBatch or something of that sort might be worthwhile to disambiguate here.
| batch | ||
| ) | ||
| await localBatch.generateTree() | ||
| return localBatch |
There was a problem hiding this comment.
I think we have relatively good unit tests around this from the class which StateChainBatch extends, but a potentially good sanity check here would be to confirm that the elementsMerkleRoot which gets stored on-chain matches the localBatch's root returned here.
There was a problem hiding this comment.
think this is solved by the "calculates Batch header hash correctly" test - hard to check if the Merkle root is calculated the same since it's not actually stored, it's immediately hashed w/ the batch header and stored as the batch header hash
| it('should return true for valid elements for different batches and elements', async () => { | ||
| await appendTxBatch(DEFAULT_TX_BATCH) | ||
| await appendTxBatch(DEFAULT_TX_BATCH) | ||
| const numBatches = 3 |
There was a problem hiding this comment.
A small improvement of coverage here would be to make the batches distinct, e.g. batches = [abatch, adifferentbatch, adifferentdifferentbatch] and using for (let batch in batches). Just takes care of the edge case where we somehow verify against the previous batch, but since they are the same the inclusion proof still passes.
willmeister
left a comment
willmeister
left a comment
There was a problem hiding this comment.
👍 LGTM 👍
Left a stylistic suggestion re: if-then-require cases, but no need to incorporate it if you don't want to 😄
| if(!safetyQueue.isEmpty()) { | ||
| require(l1ToL2Header.timestamp <= safetyQueue.peekTimestamp(), "Must process older SafetyQueue batches first to enforce timestamp monotonicity"); | ||
| } |
There was a problem hiding this comment.
| if(!safetyQueue.isEmpty()) { | |
| require(l1ToL2Header.timestamp <= safetyQueue.peekTimestamp(), "Must process older SafetyQueue batches first to enforce timestamp monotonicity"); | |
| } | |
| require(safetyQueue.isEmpty() || l1ToL2Header.timestamp <= safetyQueue.peekTimestamp(), "Must process older SafetyQueue batches first to enforce timestamp monotonicity"); |
| if(!l1ToL2Queue.isEmpty()) { | ||
| require(safetyHeader.timestamp <= l1ToL2Queue.peekTimestamp(), "Must process older L1ToL2Queue batches first to enforce timestamp monotonicity"); | ||
| } |
There was a problem hiding this comment.
| if(!l1ToL2Queue.isEmpty()) { | |
| require(safetyHeader.timestamp <= l1ToL2Queue.peekTimestamp(), "Must process older L1ToL2Queue batches first to enforce timestamp monotonicity"); | |
| } | |
| require(l1ToL2Queue.isEmpty() || safetyHeader.timestamp <= l1ToL2Queue.peekTimestamp(), "Must process older L1ToL2Queue batches first to enforce timestamp monotonicity"); |
| require(_timestamp <= l1ToL2Queue.peekTimestamp(), "Must process older L1ToL2Queue batches first to enforce timestamp monotonicity"); | ||
| } | ||
| if(!safetyQueue.isEmpty()) { | ||
| require(_timestamp <= safetyQueue.peekTimestamp(), "Must process older SafetyQueue batches first to enforce timestamp monotonicity"); |
There was a problem hiding this comment.
Can do same thing here where you remove the if and incorporate the if logic in the require
* add check and test * uint->uint256
fix: merge conflict lockbox
ethereum-optimism#143) * fix unexpected status code: 415 * update rpc
* WIP init commit for host tracking * add telemetry * address comments * address more comments and add docs * linter changes * exclude host in just file ci commands
### Description Cleans up workspace manifest.
3 participants
Description
Safety Transaction Queue: Open, free-for-all queue of transactions to enforce censorship resistance. Adds tests to enforce timestamp monotonicity in the
CanonicalTransactionChain.State Commitment Chain: Chain of state commitment batches. Implement functionality for proving that an element is at a given absolute position in the chain.
Fixes
Contributing Agreement