feat(l2): privileged transaction inclusion deadline #3427

iovoid · 2025-07-01T20:40:55Z

Motivation

To prevent the sequencer from censoring transactions, we want to force it to include at least some of them.

Description

This PR introduces a deadline after which either INCLUSION_BATCH_SIZE (or all pending transactions, if there are less) privileged transactions are included, or the batch is rejected.

Closes #3230

github-actions · 2025-07-01T20:43:09Z

Lines of code report

Total lines added: 21
Total lines removed: 0
Total lines changed: 21

Detailed view

+---------------------------------------------------------+-------+------+
| File                                                    | Lines | Diff |
+---------------------------------------------------------+-------+------+
| ethrex/crates/l2/contracts/bin/deployer/cli.rs          | 376   | +9   |
+---------------------------------------------------------+-------+------+
| ethrex/crates/l2/contracts/bin/deployer/main.rs         | 626   | +1   |
+---------------------------------------------------------+-------+------+
| ethrex/crates/l2/prover/zkvm/interface/src/execution.rs | 349   | +9   |
+---------------------------------------------------------+-------+------+
| ethrex/crates/l2/prover/zkvm/interface/src/io.rs        | 83    | +2   |
+---------------------------------------------------------+-------+------+

fkrause98 · 2025-07-01T22:18:10Z

crates/l2/sequencer/l1_committer.rs

@@ -359,7 +361,7 @@ async fn prepare_batch_from_block(

        // Accumulate block data with the rest of the batch.
        acc_messages.extend(messages.clone());
-        acc_deposits.extend(deposits.clone());
+        acc_privileged_txs.extend(privileged_transactions.clone());


Can we avoid this clone?

acc_X needs to have an owned version. .iter().cloned() could also work though.

tomip01

Just one small comment

crates/l2/contracts/src/l1/OnChainProposer.sol

crates/l2/contracts/src/l1/based/OnChainProposer.sol

fkrause98 · 2025-07-03T19:28:00Z

crates/l2/contracts/src/l1/OnChainProposer.sol

+    uint256 public constant PRIVILEGED_TX_MAX_WAIT_BEFORE_INCLUSION = 300;
+    /// @notice Minimum of privileged transactions that must be included to reset the deadline
+    /// @dev If there aren't that many pending, pendingTxHashes.length is used
+    uint16 public constant MIN_INCLUDED_PRIVILEGED_TX = 10;


Why do we use an uint16 here? What's the benefit vs simply using uint256?

To avoid having to downcast it later. State diffs only support u16::MAX privileged transactions.

crates/l2/contracts/src/l1/OnChainProposer.sol

fkrause98 · 2025-07-03T19:30:56Z

crates/l2/contracts/src/l1/OnChainProposer.sol

@@ -390,6 +403,17 @@ contract OnChainProposer is
        emit BatchVerified(lastVerifiedBatch);
    }

+    function _checkAndUpdateInclusionQuota(uint16 privileged_transaction_count) private {
+        uint256 pending_count = ICommonBridge(BRIDGE).getPendingTransactionHashes().length;
+        uint16 mimimum_to_include = MIN_INCLUDED_PRIVILEGED_TX > pending_count ? uint16(pending_count) : MIN_INCLUDED_PRIVILEGED_TX;


This is a bit strange, pending_count is uint256, and we then cast it to uint 16. Why is that?
I'm not saying this is wrong, but I don't see the benefit vs simply setting the type to uint256 and avoid potential issues.

Of the types:

privileged_transaction_count is necessarily an u16, because of a limitation in statediffs

MIN_INCLUDED_PRIVILEGED_TX should be an u16, to avoid accidentally setting impossible values

pending_count is necessarily an u256, because X.length returns one. If it's less than MIN_INCLUDED_PRIVILEGED_TX, then it can be safely downcasted.

This way minimizes the number of casts and also prevents user error.

fkrause98 · 2025-07-03T19:31:55Z

crates/l2/contracts/src/l1/based/OnChainProposer.sol

+    uint256 public constant PRIVILEGED_TX_MAX_WAIT_BEFORE_INCLUSION = 300;
+    /// @notice Minimum of privileged transactions that must be included to reset the deadline
+    /// @dev If there aren't that many pending, pendingTxHashes.length is used
+    uint16 public constant MIN_INCLUDED_PRIVILEGED_TX = 10;


fkrause98

Looks good, left some comments though.

crates/l2/contracts/src/l1/OnChainProposer.sol

avilagaston9

I think our committer, as it is now, can easily get stuck with these changes.
Let's suppose a scenario where the deadline is reached, but when the committer builds the batch, only two deposits have been made. The committer then builds the transaction, and before it is included, one more deposit is made. The transaction will revert because privileged_transaction_count < minimum_to_include, but our committer will retry forever with the same batch.

avilagaston9 · 2025-07-08T20:43:28Z

crates/l2/contracts/src/l1/OnChainProposer.sol

+    uint256 public constant PRIVILEGED_TX_MAX_WAIT_BEFORE_INCLUSION = 300;
+    /// @notice Minimum of privileged transactions that must be included to reset the deadline
+    /// @dev If there aren't that many pending, pendingTxHashes.length is used
+    uint16 public constant MIN_INCLUDED_PRIVILEGED_TX = 10;


Does defining these constants like this force us to recompile the binaries each time we want to update them?

Moved to the initializer in 7ebe8c0 to avoid that.

crates/l2/contracts/src/l1/OnChainProposer.sol

docs/l2/deposits.md

crates/l2/contracts/src/l1/OnChainProposer.sol

MegaRedHand · 2025-07-14T20:23:29Z

docs/l2/deposits.md

+    CommonBridge ->> Sequencer: OK
+    Sequencer ->> CommonBridge: Sends batch of only privileged transactions
+    CommonBridge ->> Sequencer: OK
+    Note: Sequencer catches up


GitHub is giving me errors when parsing this line:

Unable to render rich display

Fixed in 25e6a46.

avilagaston9 · 2025-07-17T14:55:16Z

crates/l2/contracts/src/l1/based/OnChainProposer.sol

+        uint256 nonPrivilegedTransactions = uint256(bytes32(publicData[160:192]));
+        require(
+            ICommonBridge(BRIDGE).withinProcessingDeadline() || nonPrivilegedTransactions == 0,
+            "OnChainProposer: exceeded privileged transaction inclusion deadline, can't include non-privileged transactions"
+        );


Isn't nonPrivilegedTransactions == 0 equivalent to enforcing privileged_transaction_count > 0? I think we should also file an issue to prioritize deposits in the sequencer.

Technically, it's not the same. But isn't it better?

No, that would only ensure at least one is being processed. We want normal ones to not advance.
Otherwise the sequencer could just send 10000 privileged transactions, include a single one per batch and stall user-made ones forever.

crates/l2/contracts/src/l1/CommonBridge.sol

ManuelBilbao

Just a few comments on docs

docs/l2/deposits.md

**Motivation** To prevent the sequencer from censoring transactions, we want to force it to include at least some of them. **Description** This PR introduces a deadline after which either `INCLUSION_BATCH_SIZE` (or all pending transactions, if there are less) privileged transactions are included, or the batch is rejected. Closes lambdaclass#3230

iovoid requested review from jrchatruc, ManuelBilbao and a team as code owners July 1, 2025 20:40

fkrause98 reviewed Jul 1, 2025

View reviewed changes

tomip01 approved these changes Jul 2, 2025

View reviewed changes

crates/l2/contracts/src/l1/OnChainProposer.sol Outdated Show resolved Hide resolved

crates/l2/contracts/src/l1/based/OnChainProposer.sol Outdated Show resolved Hide resolved

iovoid added this to ethrex_l2 Jul 2, 2025

iovoid added the L2 Rollup client label Jul 2, 2025

iovoid self-assigned this Jul 2, 2025

iovoid changed the base branch from main to refactor/simplify-privileged July 2, 2025 19:45

Base automatically changed from refactor/simplify-privileged to main July 2, 2025 20:09

iovoid added 4 commits July 2, 2025 17:13

implement inclusion deadlines

2c1d109

fix variable names

aa6e80a

improve naming

341623f

fix rebase

848a2d5

iovoid force-pushed the feat/inclusion-deadline branch from eb057fa to 848a2d5 Compare July 2, 2025 20:16

iovoid moved this to In Review in ethrex_l2 Jul 2, 2025

iovoid added 2 commits July 3, 2025 09:16

fix rebase

f3c9769

fix mispaste

c56f492

fkrause98 reviewed Jul 3, 2025

View reviewed changes

crates/l2/contracts/src/l1/OnChainProposer.sol Outdated Show resolved Hide resolved

fkrause98 reviewed Jul 3, 2025

View reviewed changes

fkrause98 approved these changes Jul 3, 2025

View reviewed changes

ManuelBilbao reviewed Jul 3, 2025

View reviewed changes

crates/l2/contracts/src/l1/OnChainProposer.sol Outdated Show resolved Hide resolved

crates/l2/contracts/src/l1/OnChainProposer.sol Outdated Show resolved Hide resolved

iovoid added 2 commits July 3, 2025 17:05

make default explicit

b940727

use time at contract creation as defaut

3cf2099

avilagaston9 requested changes Jul 8, 2025

View reviewed changes

github-project-automation bot moved this from In Review to Requires Changes in ethrex_l2 Jul 8, 2025

tolerate being short of the inclusion target by a couple transactions

a984371

MegaRedHand reviewed Jul 14, 2025

View reviewed changes

crates/l2/contracts/src/l1/OnChainProposer.sol Outdated Show resolved Hide resolved

MegaRedHand reviewed Jul 14, 2025

View reviewed changes

docs/l2/deposits.md Show resolved Hide resolved

allow only-privileged-transaction after deadline has passed

d3be0f5

MegaRedHand reviewed Jul 14, 2025

View reviewed changes

crates/l2/contracts/src/l1/OnChainProposer.sol Outdated Show resolved Hide resolved

iovoid added 3 commits July 14, 2025 16:18

fmt

3a05406

fix comparison

f5035b0

Merge branch 'main' into feat/inclusion-deadline

13917e1

MegaRedHand approved these changes Jul 14, 2025

View reviewed changes

MegaRedHand reviewed Jul 14, 2025

View reviewed changes

iovoid added 3 commits July 14, 2025 17:38

fix mermaid

25e6a46

Merge branch 'main' into feat/inclusion-deadline

b49e904

Merge branch 'main' into feat/inclusion-deadline

871f649

avilagaston9 reviewed Jul 17, 2025

View reviewed changes

ManuelBilbao requested changes Jul 17, 2025

View reviewed changes

crates/l2/contracts/src/l1/CommonBridge.sol Outdated Show resolved Hide resolved

github-project-automation bot moved this from In Review to Requires Changes in ethrex_l2 Jul 17, 2025

iovoid added 4 commits July 17, 2025 16:02

allow normal transactions if expired ones were fully processed

d7ccecb

Merge branch 'main' into feat/inclusion-deadline

19f66af

rename function

fcf4781

fix

d1ce4fc

ManuelBilbao reviewed Jul 18, 2025

View reviewed changes

docs/l2/deposits.md Outdated Show resolved Hide resolved

docs/l2/deposits.md Outdated Show resolved Hide resolved

update docs

a6d4e74

ManuelBilbao approved these changes Jul 18, 2025

View reviewed changes

avilagaston9 mentioned this pull request Jul 18, 2025

L2: Prioritize Privileged Transactions in Sequencer #3730

Closed

avilagaston9 approved these changes Jul 18, 2025

View reviewed changes

iovoid added this pull request to the merge queue Jul 18, 2025

Merged via the queue into main with commit 894ac49 Jul 18, 2025
41 checks passed

iovoid deleted the feat/inclusion-deadline branch July 18, 2025 20:48

github-project-automation bot moved this from Requires Changes to Done in ethrex_l2 Jul 18, 2025

feat(l2): privileged transaction inclusion deadline #3427

feat(l2): privileged transaction inclusion deadline #3427

Uh oh!

Conversation

iovoid commented Jul 1, 2025

Uh oh!

github-actions bot commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Lines of code report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomip01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fkrause98 Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fkrause98 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

avilagaston9 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MegaRedHand Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iovoid Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ManuelBilbao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jul 1, 2025 •

edited

Loading

fkrause98 Jul 3, 2025 •

edited

Loading

MegaRedHand Jul 14, 2025 •

edited

Loading

iovoid Jul 14, 2025 •

edited

Loading