`HUB` consistency arguments #1746

OlivierBBB · 2025-01-27T23:27:02Z

No description provided.

This fix enforces that the sender be warm in all cases during TX_FINL

Previously we were writing account/CODE_FRAGMENT_INDEX only if requiresRomLex. This is wrong and breaks account consistency constraints.

fixes issue where SELFDESTRUCT's would have old and new AccountSnapshot's be empty on the account wiping account-row.

fixes various acp_ related bugs

OlivierBBB · 2025-01-28T17:04:43Z

26c3912 addresses Bug in scn/CALL_SMC_FAILURE_WILL_REVERT self calls.

The point being: our MemoryRange object now should always remember the relevant context number. MemoryRange.EMPTY, however, doesn't remember it. This is problematic when encountering a context row after the context row which updated the return data after an exception.

OlivierBBB · 2025-01-28T18:46:55Z

arithmetization/src/main/java/net/consensys/linea/zktracer/module/hub/Hub.java

@@ -733,8 +733,6 @@ public void tracePostExecution(MessageFrame frame, Operation.OperationResult ope
    if (isExceptional()) {
      this.currentTraceSection()
          .exceptionalContextFragment(ContextFragment.executionProvidesEmptyReturnData(this));
-      this.squashCurrentFrameOutputData();
-      this.squashParentFrameReturnData();
    }


squashParentFrameReturnData replaces the value of MemorySpan returnData provided to the parent frame in exceptionalContextFragment, which is

MemorySpan(currentFrame.contextnumber())

which has a nonzero context number, with

MemorySpan.EMPTY

which has a context number of 0. This was a mistake dating back to the time when we had both a returnDataContextNumber and a returnDataSpan rather than the current system of having both united in a MemorySpan object.

OlivierBBB · 2025-01-28T18:48:09Z

arithmetization/src/main/java/net/consensys/linea/zktracer/module/hub/Hub.java

@@ -1100,7 +1098,7 @@ public void squashCurrentFrameOutputData() {
  }

  public void squashParentFrameReturnData() {
-    callStack.parentCallFrame().outputDataRange(MemoryRange.EMPTY);
+    callStack.parentCallFrame().returnDataRange(MemoryRange.EMPTY);


OlivierBBB · 2025-01-28T18:49:18Z

.../src/main/java/net/consensys/linea/zktracer/module/hub/fragment/account/AccountFragment.java

+              newState.address(), newState.deploymentNumber(), newState.deploymentStatus());
+    } catch (RuntimeException e) {
+      codeFragmentIndex = 0;
+    }


CFI's are required on all account-rows, but they are looked up only on those where requiresRomlex = true.

I still don't understand the need for this change : in case requiresRomlex is false, the previous method gives 0 and shouldn't raise an exception

in case requiresRomlex is false, the previous method gives 0 and shouldn't raise an exception

I may have mixed things up then. I vaguely remember getting an exception at some point, but I may be misremembering.

The reason, as discussed earlier, seems to be that when we request the CFI of an account with either empty code or whose byte code does not get loaded into the ROM it may throw an exception, which is what I remember. I may change it to

if (cfiRequired) { // old way } else { // above way with error handling }

to make sure we don't have "required CFI requests" that don't find a match in the ROM_LEX map.

We now check that if cfiRequired = true that getting the CFI doesn't throw a run time exception.

...ion/src/main/java/net/consensys/linea/zktracer/module/hub/section/TxFinalizationSection.java

OlivierBBB · 2025-01-28T18:51:42Z

...ion/src/main/java/net/consensys/linea/zktracer/module/hub/section/TxFinalizationSection.java

@@ -123,7 +129,7 @@ private void setSnapshots(Hub hub, WorldView world) {
    senderGasRefundNew =
        senderIsCoinbase(hub)
            ? coinbaseGasRefund.deepCopy()
-            : AccountSnapshot.canonical(hub, world, senderAddress);
+            : AccountSnapshot.canonical(hub, world, senderAddress).turnOnWarmth();


The comment on coinbase address warmth at transaction end, in particular in case of address collisions, explains why we do this.

OlivierBBB · 2025-01-28T18:52:37Z

...tization/src/main/java/net/consensys/linea/zktracer/module/hub/section/call/CallSection.java

+      calleeThird = calleeSecondNew.deepCopy().setDeploymentNumber(hub);
+      calleeThirdNew = calleeFirst.deepCopy().setDeploymentNumber(hub);
+    }
+


See Consensys/linea-specification#83 for an explanation.

OlivierBBB · 2025-01-28T18:54:45Z

arithmetization/src/main/java/net/consensys/linea/zktracer/runtime/callstack/CallFrame.java

@@ -203,6 +203,7 @@ public void rememberGasNextBeforePausing(Hub hub) {
    this.parentId = parentId;
    this.callDataRange = callDataRange;
    this.returnAtRange = returnAtRange;
+    this.outputDataRange = new MemoryRange(contextNumber);


The outputDataRange, if not overwritten nontrivially at traceContextExit, should at least remember its own context number, as that will always be correct and provides its parent frame at least with the correct returnDataContextNumber by means of returnDataRange.contextNumber().

OlivierBBB · 2025-01-28T18:55:21Z

arithmetization/src/main/java/net/consensys/linea/zktracer/runtime/callstack/CallFrame.java

-  /** Get to fucking work GitHub */
-  public void squashReturnData() {
-    returnDataRange(MemoryRange.EMPTY);
-  }


A relic of past frustration when GitHub runners were down for a few days 😅

OlivierBBB · 2025-01-28T18:55:54Z

arithmetization/src/main/java/net/consensys/linea/zktracer/runtime/stack/Stack.java

-    Bytes val1 = getStack(frame, 0);
-    Bytes val2 = getStack(frame, depth);
+    Bytes topValue = getStack(frame, 0);
+    Bytes botValue = getStack(frame, depth);


Simple mix up. Was invisible before stack consistency constraints.

OlivierBBB · 2025-01-28T18:56:47Z

...tization/src/main/java/net/consensys/linea/zktracer/types/TransactionProcessingMetadata.java

-      destructedAccountsSnapshot.add(
-          AccountSnapshot.fromAddress(
-              address, true, hub.deploymentNumberOf(address), hub.deploymentStatusOf(address)));
+      destructedAccountsSnapshot.add(AccountSnapshot.canonical(hub, address));


Using canonical gives us access to the correct code, nonce, etc ...

OlivierBBB · 2025-02-06T10:22:22Z

arithmetization/src/main/java/net/consensys/linea/zktracer/module/hub/AccountSnapshot.java

-        Bytecode.EMPTY,
-        deploymentInfo.deploymentNumber(address),
-        deploymentStatus);
+    this.nonce(0).balance(Wei.ZERO).code(Bytecode.EMPTY).setDeploymentInfo(deploymentInfo);


The wipe method must be invoked after the SELFDESTRUCT'ing account's deployment information underwent a freshDeployment.

OlivierBBB · 2025-02-06T10:23:51Z

arithmetization/src/main/java/net/consensys/linea/zktracer/module/hub/Hub.java

    defers.resolveAtEndTransaction(this, world, tx, isSuccessful);
+    defers.resolveAfterTransactionFinalization(this, world);


The only items that are in the resolveAfterTransactionFinalization defer registry are the "account wiping rows" of successful SELFDESTRUCT's.

OlivierBBB · 2025-02-06T10:28:49Z

arithmetization/src/main/java/net/consensys/linea/zktracer/module/hub/Hub.java

+      coinbaseWarmthAtTransactionEnd =
+          isExceptional() || opCode() == REVERT
+              ? txStack.current().coinbaseWarmthAfterTxInit(this)
+              : frame.isAddressWarm(coinbaseAddress);
    }


If the root context reverts Besu's frame will revert and reset account warmths. This will clash with conventions where e.g. the sender is warm at transaction end in the ZKEVM, regardless of circumstances. This isn't an issue for the sender but it may be an issue for the coinbase in case the coinbase is either the sender or the recipient.

OlivierBBB · 2025-02-06T10:30:08Z

arithmetization/src/main/java/net/consensys/linea/zktracer/module/hub/defer/DeferRegistry.java

      defer.resolveAtEndTransaction(hub, world, tx, isSuccessful);
    }
-    postTransactionDefers.clear();
+    endTransactionDefers.clear();
+  }


Nothing happens here, it's just the renaming of postTransactionDefers to endTransactionDefers.

OlivierBBB · 2025-02-06T10:30:42Z

arithmetization/src/main/java/net/consensys/linea/zktracer/module/hub/defer/DeferRegistry.java

+    for (AfterTransactionFinalizationDefer defer : afterTransactionFinalizationDefers) {
+      defer.resolveAfterTransactionFinalization(hub, worldView);
+    }
+    afterTransactionFinalizationDefers.clear();


Wiping this registry is important 🙂.

OlivierBBB · 2025-02-06T10:31:10Z

.../src/main/java/net/consensys/linea/zktracer/module/hub/fragment/account/AccountFragment.java

        .pAccountDeploymentStatus(oldState.deploymentStatus())
+        .pAccountDeploymentNumberNew(newState.deploymentNumber())
        .pAccountDeploymentStatusNew(newState.deploymentStatus())
        .pAccountDeploymentNumberInfty(deploymentNumberInfinity)


Just reordering.

sonarqubecloud · 2025-02-06T11:13:15Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

OlivierBBB · 2025-02-06T11:17:07Z

...n/src/main/java/net/consensys/linea/zktracer/module/hub/section/TxInitializationSection.java

+            .deploymentStatus(true)
+            .code(recipientValueReceptionNew.code());
+      }
+


This is required deployment transactions get converted to either

deployed byte code

no deployed byte code

and this requires a transition from the state of the account where there is initialization code (recipientValueReceptionNew.code()) to one where there isn't. We thus grab the initialization code to fill the "pre-state" with it.

OlivierBBB · 2025-02-06T11:21:16Z

...hmetization/src/main/java/net/consensys/linea/zktracer/module/hub/section/TxSkipSection.java

@@ -112,7 +112,7 @@ public void resolveAtEndTransaction(
    // may have to be modified in case of address collision
    senderNew = canonical(hub, world, sender.address(), isPrecompile(sender.address()));
    recipientNew = canonical(hub, world, recipient.address(), isPrecompile(recipient.address()));
-    coinbaseNew = canonical(hub, world, coinbase.address(), isPrecompile(recipient.address()));
+    coinbaseNew = canonical(hub, world, coinbase.address(), isPrecompile(coinbase.address()));


OlivierBBB · 2025-02-06T11:21:52Z

...tization/src/main/java/net/consensys/linea/zktracer/module/hub/section/call/CallSection.java

-    calleeSecond = reEntryCalleeSnapshot.deepCopy().setDeploymentInfo(hub);
-    calleeSecondNew = calleeFirst.deepCopy().setDeploymentInfo(hub);
+    calleeSecond = reEntryCalleeSnapshot.deepCopy().setDeploymentNumber(hub);
+    calleeSecondNew = calleeFirst.deepCopy().setDeploymentNumber(hub);


The deployment number may have evolved, but it is the deployment status we are walking back.

OlivierBBB · 2025-02-06T11:24:09Z

...tization/src/main/java/net/consensys/linea/zktracer/module/hub/section/call/CallSection.java

-    calleeSecond = calleeFirstNew.deepCopy().setDeploymentInfo(hub);
-    calleeSecondNew = calleeFirst.deepCopy().setDeploymentInfo(hub);
+    calleeSecond = calleeFirstNew.deepCopy().setDeploymentNumber(hub);
+    calleeSecondNew = calleeFirst.deepCopy().setDeploymentNumber(hub);


The deployment number may have evolved, but it is the deployment status we are walking back.

OlivierBBB · 2025-02-06T11:26:39Z

.../src/main/java/net/consensys/linea/zktracer/module/hub/section/halt/SelfdestructSection.java


-    this.addFragment(finalUnexceptionalContextFragment);
+    accountWipingNew.wipe(hub.transients().conflation().deploymentInfo());


This is the raison d'être of the AfterTransactionFinalization defer registry. It is so that we don't raise the deployment number of SELFDESTRUCT'ing accounts prior to executing the TX_FINL phase. This messes things up in case the COINBASE is among the SELFDESTRUCT'ing accounts.

OlivierBBB · 2025-02-06T11:29:13Z

...n/java/net/consensys/linea/zktracer/module/tables/instructionDecoder/InstructionDecoder.java

@@ -126,7 +125,7 @@ public void commit(List<MappedByteBuffer> buffers) {
      traceStackSettings(op, trace);
      traceBillingSettings(op, trace);
      trace
-          .opcode(Bytes.ofUnsignedInt(i))
+          .opcode(UnsignedByte.of(i))


@DavePearce I've had to correct this a couple of times here and there. Just letting you know, not sure if it's a corset change that's at the source of this change in signature.

OlivierBBB · 2025-02-06T11:39:15Z

...tization/src/main/java/net/consensys/linea/zktracer/types/TransactionProcessingMetadata.java

-      destructedAccountsSnapshot.add(
-          AccountSnapshot.fromAddress(
-              address, true, hub.deploymentNumberOf(address), hub.deploymentStatusOf(address)));
+      destructedAccountsSnapshot.add(AccountSnapshot.canonical(hub, world, address));


This was a very stupid oversight / temporary solution to a problem that only became noticeable with account consistency on. The fromAddress assigns empty code, zero balance, zero nonce etc. to the AccountSnapshot it is creating. Unsurprisingly this broke account consistency constraints.

OlivierBBB · 2025-02-06T11:41:41Z

...tization/src/main/java/net/consensys/linea/zktracer/types/TransactionProcessingMetadata.java

+        || coinbaseIsPrecompile
+        || coinbaseIsSender
+        || coinbaseIsRecipient;
+  }


This method logs whether the COINBASE address will be warm at the end of TX_INIT. This is required in case of a REVERT'ing transaction with an address collision. It will be made obsolete by EIP-3651.

lorenzogentile404

accountWipingNew.wipe(hub.transients().conflation().deploymentInfo()); returns a new AccountSnapshot that is not used.

OlivierBBB · 2025-02-06T18:21:50Z

accountWipingNew.wipe(hub.transients().conflation().deploymentInfo()); returns a new AccountSnapshot that is not used.

As discussed on slack, this is not the case.

OlivierBBB and others added 16 commits January 24, 2025 17:05

release: bump version to beta-v1.2.0-rc2 and update changelog

fe12daa

Merge branch 'arith-dev' into rel/beta-v1.2.0-rc2

f019281

feat: disable rust corset validator + changed constraints commit

a2b27a1

feat: turn on HUB consistency constraints

857ad04

fix: ras (go-corset flags as suggested by @David.Pearce)

dc7b49f

fix: Besu reverets accrued states gefore it traceEndTransaction

ebfed91

This fix enforces that the sender be warm in all cases during TX_FINL

fix: the CFI must always be printed (and defults to 0)

fba2412

Previously we were writing account/CODE_FRAGMENT_INDEX only if requiresRomLex. This is wrong and breaks account consistency constraints.

ras: dead code

12e1d64

fix: CallFrame now initialized with outputDataRange pointing to them

f5e0f5e

fix: pre-wiping snapshot now uses canonical()

f83c1c2

fixes issue where SELFDESTRUCT's would have old and new AccountSnapshot's be empty on the account wiping account-row.

fix:ras

30a7c82

fix: constraints commit update

e54f2b3

fixes various acp_ related bugs

fix: mix-up in swapStackPattern between "top" and "bottom" values

cb7d513

ras: banal tests + spotless

5a1fb95

fix: subtle point about scn/CALL_SMC_FAILURE_WILL_REVERT for self calls

26c3912

ras

3fd8115

OlivierBBB mentioned this pull request Jan 28, 2025

Bug in scn/CALL_SMC_FAILURE_WILL_REVERT self calls Consensys/linea-specification#83

Closed

OlivierBBB commented Jan 28, 2025

View reviewed changes

...ion/src/main/java/net/consensys/linea/zktracer/module/hub/section/TxFinalizationSection.java Outdated Show resolved Hide resolved

OlivierBBB commented Jan 28, 2025

View reviewed changes

OlivierBBB requested a review from letypequividelespoubelles January 28, 2025 18:57

OlivierBBB commented Feb 6, 2025

View reviewed changes

OlivierBBB added 2 commits February 6, 2025 12:09

ras: renaming

b7524ca

fix: added check wrt getting CFI when requiresRomLex = true

990f7db

OlivierBBB commented Feb 6, 2025

View reviewed changes

ras: removing redundant debugging code

a69e5e1

OlivierBBB commented Feb 6, 2025

View reviewed changes

OlivierBBB added 2 commits February 6, 2025 12:44

ras: documentation

5b299a8

spotless

b768f75

OlivierBBB requested review from letypequividelespoubelles and lorenzogentile404 February 6, 2025 11:58

lorenzogentile404 approved these changes Feb 6, 2025

View reviewed changes

lorenzogentile404 requested changes Feb 6, 2025

View reviewed changes

merge arith-dev into HUB-consistency-arguments

ec46379

OlivierBBB requested a review from lorenzogentile404 February 6, 2025 18:24

remove corset-unit-tests

6068c45

OlivierBBB merged commit 1f032b9 into arith-dev Feb 6, 2025
7 checks passed

OlivierBBB deleted the HUB-consistency-arguments branch February 6, 2025 22:11

		defers.resolveAtEndTransaction(this, world, tx, isSuccessful);
		defers.resolveAfterTransactionFinalization(this, world);


		this.addFragment(finalUnexceptionalContextFragment);
		accountWipingNew.wipe(hub.transients().conflation().deploymentInfo());

HUB consistency arguments #1746

HUB consistency arguments #1746

Uh oh!

Conversation

OlivierBBB commented Jan 27, 2025

Uh oh!

OlivierBBB commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlivierBBB Jan 28, 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!

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!

OlivierBBB Jan 28, 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!

OlivierBBB Jan 28, 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!

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!

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!

sonarqubecloud bot commented Feb 6, 2025

Quality Gate passed

Uh oh!

OlivierBBB Feb 6, 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!

OlivierBBB Feb 6, 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!

Choose a reason for hiding this comment

Uh oh!

OlivierBBB Feb 6, 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!

lorenzogentile404 left a comment

`HUB` consistency arguments #1746

`HUB` consistency arguments #1746

OlivierBBB commented Jan 28, 2025 •

edited

Loading

OlivierBBB Jan 28, 2025 •

edited

Loading

OlivierBBB Jan 28, 2025 •

edited

Loading

OlivierBBB Jan 28, 2025 •

edited

Loading

OlivierBBB Feb 6, 2025 •

edited

Loading

OlivierBBB Feb 6, 2025 •

edited

Loading

OlivierBBB Feb 6, 2025 •

edited

Loading