-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Optimize stack related WCP
operations
#1242
Merged
OlivierBBB
merged 12 commits into
arith-dev
from
1238-optimize-stack-health-related-wcp-operations
Sep 20, 2024
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
8ac1a0a
fix: stackedSet documentation
OlivierBBB f59bd54
fix: implementation of optimized stack height checks
OlivierBBB c560f6c
spotless
OlivierBBB e7f6fe6
Merge branch 'arith-dev' into 1238-optimize-stack-health-related-wcp-…
OlivierBBB 6146b47
ras
letypequividelespoubelles ec08489
fix: fixed erroneous precondition
OlivierBBB bd70311
fix: split the sets
OlivierBBB 4bc2798
Merge branch 'arith-dev' into 1238-optimize-stack-health-related-wcp-…
OlivierBBB 9acb768
fix: doc string for test
OlivierBBB 774e4a4
update constraints
letypequividelespoubelles 571158f
Merge branch 'arith-dev' into 1238-optimize-stack-health-related-wcp-…
OlivierBBB 2caecd0
feat: StackedSet extends NoLineCountStackedSet
letypequividelespoubelles File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
64 changes: 64 additions & 0 deletions
64
...n/src/main/java/net/consensys/linea/zktracer/container/stacked/NoLineCountStackedSet.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
/* | ||
* Copyright ConsenSys Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations under the License. | ||
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
|
||
package net.consensys.linea.zktracer.container.stacked; | ||
|
||
import java.util.HashSet; | ||
import java.util.Set; | ||
|
||
import lombok.Getter; | ||
import lombok.experimental.Accessors; | ||
|
||
@Accessors(fluent = true) | ||
@Getter | ||
public class NoLineCountStackedSet<E> { | ||
private final Set<E> operationsCommitedToTheConflation; | ||
private final Set<E> operationsInTransaction; | ||
|
||
public NoLineCountStackedSet() { | ||
operationsCommitedToTheConflation = new HashSet<>(); | ||
operationsInTransaction = new HashSet<>(); | ||
} | ||
|
||
/** Prefer this constructor as we preallocate more needed memory */ | ||
public NoLineCountStackedSet( | ||
final int expectedConflationNumberOperations, final int expectedTransactionNumberOperations) { | ||
operationsCommitedToTheConflation = new HashSet<>(expectedConflationNumberOperations); | ||
operationsInTransaction = new HashSet<>(expectedTransactionNumberOperations); | ||
} | ||
|
||
/** | ||
* Upon entering a new transaction, the set of operations generated by the previous transaction | ||
* {@link StackedSet#operationsInTransaction()} (which is empty if no such transaction exists) is | ||
* added to the set of committed operations {@link | ||
* StackedSet#operationsCommitedToTheConflation()}. {@link StackedSet#operationsInTransaction()} | ||
* is further reset to be empty. | ||
*/ | ||
public void enter() { | ||
operationsCommitedToTheConflation().addAll(operationsInTransaction()); | ||
operationsInTransaction().clear(); | ||
} | ||
|
||
public void pop() { | ||
operationsInTransaction().clear(); | ||
} | ||
|
||
public boolean add(E e) { | ||
if (!operationsCommitedToTheConflation().contains(e)) { | ||
return operationsInTransaction().add(e); | ||
} | ||
return false; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
49 changes: 49 additions & 0 deletions
49
...on/src/main/java/net/consensys/linea/zktracer/module/hub/transients/StackHeightCheck.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/* | ||
* Copyright Consensys Software Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on | ||
* an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations under the License. | ||
* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
*/ | ||
package net.consensys.linea.zktracer.module.hub.transients; | ||
|
||
import static com.google.common.base.Preconditions.*; | ||
import static net.consensys.linea.zktracer.runtime.stack.Stack.MAX_STACK_SIZE; | ||
|
||
import lombok.EqualsAndHashCode; | ||
|
||
@EqualsAndHashCode(onlyExplicitlyIncluded = true, callSuper = false) | ||
public class StackHeightCheck { | ||
private static final Integer SHIFT_FACTOR = | ||
8; // 5 would suffice but 8 makes it a byte shift, not sure if it matters | ||
|
||
@EqualsAndHashCode.Include final int comparison; | ||
|
||
/** | ||
* This constructor creates a {@link StackHeightCheck} for stack underflow detection. | ||
* | ||
* @param height stack height pre opcode execution | ||
* @param delta greatest depth at which touched stack items | ||
*/ | ||
public StackHeightCheck(int height, int delta) { | ||
checkArgument(0 <= height && height <= MAX_STACK_SIZE && 0 <= delta && delta <= 17); | ||
comparison = height << SHIFT_FACTOR | delta; | ||
} | ||
|
||
/** | ||
* This constructor creates a {@link StackHeightCheck} for stack overflow detection. | ||
* | ||
* @param heightNew stack height post opcode execution | ||
*/ | ||
public StackHeightCheck(int heightNew) { | ||
checkArgument(0 <= heightNew && heightNew <= MAX_STACK_SIZE + 1); | ||
comparison = heightNew; | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my understanding, why the height can go from 0 to 1025 ? I would expect 1024 instead of 1026 elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The height is not allowed to go to anything$\mathtt{nbRemoved} = 0$ , but adds onto it, $\mathtt{nbAdded} = 1$ . Or in EYP terminology $\delta < \alpha$ . In this case the zkevm will compute
> 1024
but certain instructions will bring it to 1025. E.g. your stack is full (height = 1024
) and you do aPUSHX
orTIMESTAMP
or so ... some instruction that doesn't remove any items,heightNew = height - delta + alpha = height + 1 = 1024 + 1
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It leads to a
stackOverflowException
and so it's not a legal value from the EVM pov. From the zkevm pov the WCP module will test whetherheightNew > 1024
regardless, in this case the answer would betrue
and so the zkevm knows to throw aSOX
(STACK_OVERFLOW_EXCEPTION
).