-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(SELFDESTRUCT): deloyment number #1336
Conversation
There were several problems: - we were updating the deployment number to a fresh new deployment number twice per SELFDESTRUCT - the ROM_LEX module had no idea about these fresh new deployments - we were assuming that DEPLOYMENT_NUMBER_NEW = 1 for any deployment transaction, but this can be at fault in the EVM test suite
Signed-off-by: Francois Bojarski <francois.bojarski@consensys.net>
Signed-off-by: Francois Bojarski <francois.bojarski@consensys.net>
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.
Made some clean up + Idu two things, see comments
arithmetization/src/main/java/net/consensys/linea/zktracer/module/romlex/RomLex.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
// @françois what replaces this for message calls to smart contracts ? |
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.
I don't understand your question. Why do you want to store the CFI of the smart contract recipient of the tx ? We compute it in the recipient AccountFragment. For the moment, the only place where we read the CFI of a deployment transaction is in TXN_DATA
, this is strange ...
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.
Why do you want to store the CFI of the smart contract recipient of the tx ?
Because the traces require it. The question is when/where does it get computed.
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.
we do it at traceStartTx
in the RomLex so this is ok, already done. We can delete the comment, as this method just set the CFI of the initCode in the TransactionProcessingMetadata. It is read only by TXN_DATA
for the moment.
Signed-off-by: Francois Bojarski <francois.bojarski@consensys.net>
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.
LGTM
arithmetization/src/main/java/net/consensys/linea/zktracer/module/hub/Hub.java
Show resolved
Hide resolved
} | ||
|
||
// @françois what replaces this for message calls to smart contracts ? |
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.
we do it at traceStartTx
in the RomLex so this is ok, already done. We can delete the comment, as this method just set the CFI of the initCode in the TransactionProcessingMetadata. It is read only by TXN_DATA
for the moment.
There were several problems:
the ROM_LEX module had no idea about these fresh new deploymentsthis isn't an issue and was removed