[PAN-2904] EIP-1108 - Reprice alt_bn128 #1704
Conversation
Add configurability to how the ECADD, ECMUL, and Pairing check precompiled contracts calculate their gas price, and expose static methods for byzantium and istanbul era prices.
| private final Gas baseGasCost; | ||
|
|
||
| private AltBN128PairingPrecompiledContract(final Gas pairingGasCost, final Gas baseGasCost) { | ||
| super("AltBN128Pairing", null); |
There was a problem hiding this comment.
It looks like you're using a new model for modifying the gas values by hardfork. Other precompiles pull versioned gas values from the GasCalculator. Any reason why that existing pattern won't work here?
There was a problem hiding this comment.
It becomes a very high touch solution. In this case I would have to introduce two more interface methods in a separate file. This will make it easier to implement EIPs that are laser focused on one particular set of opcode pricing without a large amount of interaction.
It also makes it difficult to split our version based logic cleanly. In the future gas prices may diverge based on Account Version of the contract, and they can vary from call frame to call frame. By putting it in the contract it reduces the logic since we have a different contract registries per version and per fork. While right now it would be a simple if check we may wind up having many of those in separate methods in a file separated from the objects representing the contract or operation.
This will also make it easier to stand up "Fork+EIPX" tests like Geth and parity have been talking about doing, when the operations and contracts become more independent of each other instead of needing to work with a shared gas calculator class.
There was a problem hiding this comment.
I'm not a big fan of the GasCalculator design as it is for the reasons you mention - it spreads out op-related data and logic in this kind of messy way. But, it seems that if we want to change that pattern, we should have a plan to refactor. Rather than introducing 2 patterns at once.
There was a problem hiding this comment.
Discussed offline - we'll keep this new pattern for now, and follow-up with a ticket to do a more comprehensive refactor.
Edit: We decided that for now we shouldn't null out the base class gasCalculator variable. We can potentially get rid of that variable when we do the refactor.
There was a problem hiding this comment.
PAN-2911 is the ticket.
| } | ||
|
|
||
| public static AltBN128MulPrecompiledContract istanbul(final GasCalculator gasCalculator) { | ||
| return new AltBN128MulPrecompiledContract(gasCalculator, Gas.of(8_000)); |
There was a problem hiding this comment.
Yep. I missed it on review because our schedule was split across 3 files. - #1874
correct istanbul ECMUL gas cost
PR description
Add configurability to how the ECADD, ECMUL, and Pairing check precompiled
contracts calculate their gas price, and expose static methods for Byzantium
and Istanbul era prices.