EIP2935 - Use system call instead of direct storage modification #8243
Conversation
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
| public Void process(final BlockProcessingContext context) { | ||
| super.process(context); | ||
| SystemCallProcessor processor = | ||
| new SystemCallProcessor(context.getProtocolSpec().getTransactionProcessor()); | ||
|
|
||
| Bytes inputData = context.getBlockHeader().getParentHash(); | ||
| processor.process(historyStorageAddress, context, inputData); | ||
| return null; |
There was a problem hiding this comment.
This is the important change of this PR. The rest is refactoring.
Gabriel-Trintinalia
requested review from
lu-pinto and
siladu
and removed request for
lu-pinto
Gabriel-Trintinalia
changed the title
...core/src/main/java/org/hyperledger/besu/ethereum/mainnet/systemcall/SystemCallProcessor.java
Show resolved
Hide resolved
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
...core/src/main/java/org/hyperledger/besu/ethereum/mainnet/systemcall/SystemCallProcessor.java
Show resolved
Hide resolved
| import org.hyperledger.besu.evm.blockhash.BlockHashLookup; | ||
| import org.hyperledger.besu.evm.tracing.OperationTracer; | ||
|
|
||
| public class BlockProcessingContext { |
There was a problem hiding this comment.
RequestProcessingContext extends BlockProcessingContext
There was a problem hiding this comment.
Oh I didn't notice that. So why do you extend a POJO then? This looks a code smell to me.
Why doesn't RequestProcessingContext use composition instead of inheritance? This is a preferred best practice BTW.
| import org.hyperledger.besu.evm.account.Account; | ||
| import org.hyperledger.besu.evm.blockhash.BlockHashLookup; | ||
| import org.hyperledger.besu.evm.code.CodeV0; | ||
| import org.hyperledger.besu.evm.frame.MessageFrame; | ||
| import org.hyperledger.besu.evm.processor.AbstractMessageProcessor; | ||
| import org.hyperledger.besu.evm.tracing.OperationTracer; | ||
| import org.hyperledger.besu.evm.worldstate.WorldUpdater; | ||
|
|
||
| import java.util.Deque; |
There was a problem hiding this comment.
Why is this call just not called SystemCall instead of SystemCallProcessor ? What data is it processing? To me the name is a bit misleading.
There was a problem hiding this comment.
SystemCallProcessor is similar to the MessageCallProcessor, it performs a message call to the contract address with some rules and default values
There was a problem hiding this comment.
Well but you are not processing any message (which is what the MessageProcessor does) where an EOA sends a message to another EOA or a contract. In this case it is internal, you are creator of the call. See https://github.com/hyperledger/besu/pull/8243/files/6bc3fa87a7f3ab9f7ef69af600c67bf07fc63272#diff-1e7c7ef5c489c82232fb87d33704b19e4a7edb2c72a599a82293f22b79da32abL76-R76 a new MessageProcessor is created inside this function. Just the only we have to get the EVM to execute code right now, but it's an implementation detail.
Anyway I agree that it's subjective and it's a nit for sure.
| @@ -29,6 +29,6 @@ public class Eip7709BlockHashProcessor extends PragueBlockHashProcessor { | |||
| @Override | |||
| public BlockHashLookup createBlockHashLookup( | |||
| final Blockchain blockchain, final ProcessableBlockHeader blockHeader) { | |||
| return new Eip7709BlockHashLookup(historyStorageAddress, historyServeWindow); | |||
There was a problem hiding this comment.
Don't you think this could change in the future? Having the BlockHashProcessor have a handle on the window size makes it more extendable in another fork if the window changes
| @@ -37,6 +37,7 @@ | |||
| public class Eip7709BlockHashLookup implements BlockHashLookup { | |||
| private static final Logger LOG = LoggerFactory.getLogger(Eip7709BlockHashLookup.class); | |||
| private static final long BLOCKHASH_SERVE_WINDOW = 256L; | |||
| private static final long HISTORY_SERVE_WINDOW = 8191; | |||
There was a problem hiding this comment.
nit: yes, making these constants static here means that we cannot easily change these values in a future fork just by creating another instance with different values.
There was a problem hiding this comment.
this constant was only moved from the processor class to the BlockHashLookup as it is not used by the Prague processor anymore
There was a problem hiding this comment.
ok what about storing it in the Eip7709BlockHashLookup instead?
.../src/main/java/org/hyperledger/besu/ethereum/mainnet/blockhash/PragueBlockHashProcessor.java
Outdated
Show resolved
Hide resolved
...core/src/main/java/org/hyperledger/besu/ethereum/mainnet/systemcall/SystemCallProcessor.java
Show resolved
Hide resolved
Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net>
…erledger#8243) Signed-off-by: Gabriel-Trintinalia <gabriel.trintinalia@consensys.net> Signed-off-by: Bhanu Pulluri <bhanu.pulluri@kaleido.io>
PR description
Changes
PragueBlockHashProcessorto perform a system call toHISTORY_STORAGE_ADDRESSinstead of changing the storage manually as defined here: ethereum/EIPs#8816Passing both
consume-rlpandconsume-engineexecution-spec-tests inpectra-devnet-6 v1.0.0