feat(miner): 1 Block == 1 Txn #1166
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Files selected for processing (3)
- cosmos/lib/tx/serializer.go (1 hunks)
- cosmos/miner/miner.go (4 hunks)
- e2e/testapp/app.go (7 hunks)
Additional comments (Suppressed): 12
cosmos/lib/tx/serializer.go (2)
30-35: The use of generics in Go is a new feature as of Go 1.18, and it's great to see it being utilized here. However, please ensure that the rest of your codebase, as well as any libraries or frameworks you're using, are compatible with this version of Go.
54-56: The comment about not needing extensions needs clarification. If extensions are not needed, consider removing the related code to avoid confusion.
cosmos/miner/miner.go (5)
35-37: The import of
libtx "pkg.berachain.dev/polaris/cosmos/lib/tx"is new and replaces the oldevmtypes "pkg.berachain.dev/polaris/cosmos/x/evm/types". Ensure that this new package provides all necessary functionality previously provided byevmtypes.45-45: The type of
serializerhas been changed fromevmtypes.TxSerializertolibtx.TxSerializer[*engine.ExecutionPayloadEnvelope]. This change will affect how transactions are serialized. Make sure that the new serializer correctly handles*engine.ExecutionPayloadEnvelopetypes.57-59: The
Initfunction now takes alibtx.TxSerializer[*engine.ExecutionPayloadEnvelope]instead of anevmtypes.TxSerializer. Ensure that all calls toInithave been updated accordingly.75-81: The
buildBlockfunction now callsresolveEnvelopeinstead ofresolveTxs, and it returns a single byte array instead of a slice of byte arrays. This change aligns with the new one transaction per block policy, but ensure that all parts of the system that callbuildBlockcan handle the new return type.113-123: The new
resolveEnvelopefunction resolves the payload into a single byte array representing a transaction. It uses the newlibtx.TxSerializer[*engine.ExecutionPayloadEnvelope]to serialize the transaction. Ensure that the serialization process correctly handles*engine.ExecutionPayloadEnvelopetypes and that the serialized transaction can be correctly deserialized elsewhere in the system.e2e/testapp/app.go (5)
- 42-49: The import of
"github.com/cosmos/cosmos-sdk/types"assdkis missing in the new hunk. This might cause issues ifsdkis used in the code below. Please verify.+ sdk "github.com/cosmos/cosmos-sdk/types"
56-68: The import of
"github.com/ethereum/go-ethereum/node"has been moved up, and several new imports have been added (antelib,signinglib,libtx). The import ofethcryptocodecandevmantehave been removed. Ensure that these changes are intentional and that all necessary dependencies are imported.134-142: The function
signinglib.ProvideNoopGetSignersis being called with two different arguments. This seems to replace the previous call toevmtypes.ProvideEthereumTransactionGetSigners. Make sure that this change is intended and that it doesn't break any existing functionality.214-220: The Ante Handler setup has been simplified to use
antelib.NewMinimalHandler(). This replaces the previous more complex setup withevmante.NewAnteHandler. Ensure that this simpler handler still meets all requirements.283-295: Two serializers (
txSerializerandpayloadSerializer) are now created instead of one. Ensure that these serializers are used correctly in the rest of the code.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files ignored due to filter (2)
- e2e/testapp/go.sum
- go.work.sum
Files selected for processing (5)
- cosmos/x/evm/types/tx.go (2 hunks)
- e2e/testapp/app_config.go (1 hunks)
- e2e/testapp/entrypoint.sh (1 hunks)
- e2e/testapp/polard/cmd/root.go (1 hunks)
- proto/polaris/evm/v1alpha1/tx.proto (2 hunks)
Files skipped from review due to trivial changes (2)
- e2e/testapp/app_config.go
- e2e/testapp/entrypoint.sh
Additional comments (Suppressed): 6
proto/polaris/evm/v1alpha1/tx.proto (3)
35-36: The new RPC method
ProcessPayloadEnvelopeis introduced. Ensure that the corresponding implementation in the service layer handles theWrappedPayloadEnvelopemessage correctly and returns aWrappedPayloadEnvelopeResponsemessage.45-49: The
WrappedPayloadEnvelopemessage encapsulates an Ethereum transaction as an SDK message, similar toWrappedEthereumTransaction. Make sure that the data encapsulated in this message is handled properly wherever it's used.51-52: The
WrappedPayloadEnvelopeResponsemessage is currently empty. If this is intentional and you plan to use status codes for communication, it's fine. Otherwise, consider adding necessary fields to convey more information in the response.cosmos/x/evm/types/tx.go (2)
53-55: The
Unwrap()function has been removed. Ensure that this does not break any existing code that relies on it.57-67: The new
WrapPayload()function takes anengine.ExecutionPayloadEnvelopeand wraps it into aWrappedPayloadEnvelope. It marshals the envelope data into JSON format. If there's an error during marshalling, it returns a formatted error message. This is a good practice as it provides more context about the error.e2e/testapp/polard/cmd/root.go (1)
- 91-94: The new hunk adds a
NoopGetSignersprovider forWrappedPayloadEnvelopemessages. Ensure that this change is compatible with the rest of the codebase and that it doesn't introduce any security issues, as it seems to bypass signature verification.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files ignored due to filter (2)
- cosmos/go.mod
- go.work.sum
Files selected for processing (1)
- e2e/testapp/app.go (6 hunks)
Additional comments (Suppressed): 6
e2e/testapp/app.go (6)
42-48: The import
sdk "github.com/cosmos/cosmos-sdk/types"has been added. Ensure that this new import does not conflict with any existing variables or packages namedsdk.65-68: The import
coretypes "pkg.berachain.dev/polaris/eth/core/types"has been removed. Make sure that this does not affect any existing code that relies on this package.136-142: A new dependency injection for
signinglib.ProvideNoopGetSigners[*evmv1alpha1.WrappedPayloadEnvelope]has been added. Ensure that this new dependency is correctly initialized and does not conflict with any existing dependencies.214-220: The AnteHandler setup has been simplified. Ensure that this change does not affect the functionality of the AnteHandler.
233-246: The application now loads the last state of the blockchain when it is initialized. Make sure that this new functionality is correctly implemented and does not introduce any potential issues.
283-295: The creation of serializers and initialization of services have been updated. Ensure that these changes do not affect the functionality of the serializers and services.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files ignored due to filter (2)
- cosmos/go.mod
- go.work.sum
Files selected for processing (1)
- e2e/testapp/app.go (6 hunks)
Additional comments (Suppressed): 6
e2e/testapp/app.go (6)
42-48: The import of
sdk "github.com/cosmos/cosmos-sdk/types"has been added. Ensure that this new import does not conflict with any existing variables or functions namedsdk.65-68: The import of
coretypes "pkg.berachain.dev/polaris/eth/core/types"has been removed. Make sure that this does not break any existing code that relies oncoretypes.136-142: The
signinglib.ProvideNoopGetSignersfunction is now being called with an additional argument*evmv1alpha1.WrappedPayloadEnvelope. Ensure that this change is reflected in all calls to this function.214-219: The
app.SetAnteHandlerfunction is now being called with a different argumentantelib.NewMinimalHandler(). Make sure that this change is reflected in all calls to this function and that the new handler is compatible with the existing code.233-246: The
NewPolarisAppfunction has been significantly modified. It now includes a new contextcmsCtxand a call toapp.EVMKeeper.Polaris().Blockchain().LoadLastState. Ensure that these changes are compatible with the existing code and that the new context is properly initialized.283-295: The
RegisterAPIRoutesfunction has been significantly modified. It now includes two new serializerstxSerializerandpayloadSerializer, and theapp.mm.Initandapp.mp.Initfunctions are now being called with different arguments. Ensure that these changes are compatible with the existing code and that the new serializers are properly initialized.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (2)
- cosmos/lib/signing/signers.go (1 hunks)
- cosmos/miner/miner.go (3 hunks)
Files skipped from review due to trivial changes (1)
- cosmos/lib/signing/signers.go
Additional comments (Suppressed): 4
cosmos/miner/miner.go (4)
28-34: The import of
github.com/ethereum/go-ethereum/beacon/engineis new. Ensure that this package is compatible with the rest of the codebase and that it provides the necessary functionality.38-57: The
EnvelopeSerializerinterface andGethMinerinterface are new. They replace theEthTxSerializerinterface and*miner.Minerstruct respectively. Ensure that all methods that were available in the old interfaces/structs are available in the new ones, or that appropriate replacements have been made.72-81: The
PrepareProposalfunction now returns a single transaction (payloadEnvelopeBz) instead of a slice of transactions (txs). This change aligns with the PR's goal of ensuring that each block corresponds to a single transaction. However, ensure that this change does not negatively impact other parts of the codebase that expect a slice of transactions.123-133: The
resolveEnvelopefunction replaces theresolveTxsfunction. It returns a single transaction instead of a slice of transactions. This change aligns with the PR's goal of ensuring that each block corresponds to a single transaction. However, ensure that this change does not negatively impact other parts of the codebase that expect a slice of transactions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files ignored due to filter (7)
- cosmos/go.mod
- cosmos/go.sum
- e2e/testapp/go.mod
- e2e/testapp/go.sum
- eth/go.mod
- eth/go.sum
- go.work.sum
Files selected for processing (6)
- cosmos/miner/miner.go (3 hunks)
- eth/core/chain.go (5 hunks)
- eth/log/imported.go (1 hunks)
- eth/polar/api_backend.go (1 hunks)
- eth/polar/backend.go (1 hunks)
- eth/polar/config.go (2 hunks)
Files skipped from review due to trivial changes (3)
- eth/log/imported.go
- eth/polar/api_backend.go (diff too large)
- eth/polar/backend.go (diff too large)
Additional comments (Suppressed): 17
eth/polar/config.go (2)
32-34: The import
"pkg.berachain.dev/polaris/eth/common"is new. Ensure that the package is available and correctly referenced.45-56: The
minerCfg.Etherbaseis hardcoded tocommon.Address{1}. This might not be ideal for different environments. Consider making this configurable.- minerCfg.Etherbase = common.Address{1} + minerCfg.Etherbase = common.Address{config.Etherbase}Also, the comment
// TODO: setup proper command line flagsindicates that there is a pending task to be completed. Ensure that this is tracked and addressed.eth/core/chain.go (7)
23-38: The new hunk introduces additional imports, including
context,errors,github.com/ethereum/go-ethereum/core, andpkg.berachain.dev/polaris/eth/consensus. Ensure these new imports are used appropriately in the code and that they don't introduce unnecessary dependencies.50-53: The
Blockchaininterface has been extended with a new methodPreparePlugins(ctx context.Context). Ensure that all implementations of this interface have been updated to include this new method.68-69: The
blockchainstruct has been extended with two new fields:engineof typeconsensus.Engineandprocessorof typecore.Processor. Ensure these new fields are used appropriately in the code and that they don't introduce unnecessary complexity.126-176: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [114-138]
The
NewChainfunction has been updated to accept an additional parameterengine consensus.Engineand uses it to initialize theengineandprocessorfields of theblockchainstruct. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
141-156: The
blockchainstruct has been extended with a new methodPreparePlugins(ctx context.Context). This method calls thePreparemethod on several plugins. Ensure that these plugins are prepared to handle being prepared multiple times, as this could potentially lead to unexpected behavior.158-161: No significant changes, the
Configmethod still returns the Ethereum chain config of the chain.163-176: The
loadLastStatefunction has been updated to accept anumberparameter and uses it to get a block by number. Ensure that this change doesn't introduce any issues with the loading of the last state.cosmos/miner/miner.go (8)
28-34: The import of
github.com/ethereum/go-ethereum/beacon/engineis new. Ensure that this package is compatible with the rest of the codebase and that it provides the necessary functionality.38-58: The
EnvelopeSerializerinterface andGethMinerinterface are new. TheEnvelopeSerializerinterface replaces theEthTxSerializerinterface, and theGethMinerinterface abstracts the underlyingminer.Minerfrom geth. Ensure that all implementations of these interfaces are updated accordingly.61-65: The
Newfunction now takes aGethMinerinterface as an argument instead of a*miner.Miner. This change increases the flexibility of the function, allowing it to accept any type that implements theGethMinerinterface. Ensure that all calls to this function are updated to pass an object that implements theGethMinerinterface.67-70: The
Initfunction now takes anEnvelopeSerializeras an argument instead of anEthTxSerializer. Ensure that all calls to this function are updated to pass an object that implements theEnvelopeSerializerinterface.73-82: The
PrepareProposalfunction now returns a single transaction envelope instead of multiple transactions. This change aligns with the PR summary, which states that each block now corresponds to a single transaction. Ensure that all calls to this function and its return value handling are updated accordingly.84-92: The
buildBlockfunction now returns a single transaction envelope instead of multiple transactions. This change aligns with the PR summary, which states that each block now corresponds to a single transaction. Ensure that all calls to this function and its return value handling are updated accordingly.113-120: The
constructPayloadArgsfunction now sets theFeeRecipienttom.Etherbase()instead ofcommon.Address{}. Ensure that this change does not introduce any unintended side effects.123-134: The
resolveEnvelopefunction replaces theresolveTxsfunction. It resolves the payload into a single transaction envelope instead of multiple transactions. This change aligns with the PR summary, which states that each block now corresponds to a single transaction. Ensure that all calls to this function and its return value handling are updated accordingly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- eth/core/chain.go (5 hunks)
Additional comments (Suppressed): 6
eth/core/chain.go (6)
23-38: The import section has been updated to include additional packages. Ensure that these new packages are compatible with the existing codebase and do not introduce any breaking changes.
49-53: The
Blockchaininterface has been updated to include a new methodPreparePlugins(ctx context.Context). Ensure that all implementations of this interface have been updated to include this new method.114-116: The function signature of
NewChainhas been updated to include an additional parameterengine consensus.Engine. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.130-132: The
blockchainstruct has been updated to include a new fieldengine consensus.Engine. Ensure that this new field is properly initialized and used throughout the codebase.150-158: The new method
PreparePlugins(ctx context.Context)has been added to theblockchainstruct. This method prepares the plugins for the blockchain. Ensure that this method is called at the appropriate times to prepare the plugins.166-178: The
loadLastStatefunction has been updated to include a new parameternumber uint64. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, ensure that thenumberparameter is properly used within the function.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Files ignored due to filter (7)
- cosmos/go.mod
- cosmos/go.sum
- e2e/testapp/go.mod
- e2e/testapp/go.sum
- eth/go.mod
- eth/go.sum
- go.work.sum
Files selected for processing (5)
- cosmos/x/evm/keeper/genesis.go (1 hunks)
- cosmos/x/evm/keeper/processor.go (1 hunks)
- eth/core/chain.go (5 hunks)
- eth/core/chain_reader.go (3 hunks)
- eth/core/chain_writer.go (1 hunks)
Additional comments (Suppressed): 16
cosmos/x/evm/keeper/genesis.go (1)
- 46-48: The new code introduces a call to
PreparePluginsandWriteGenesisBlock. Ensure that these methods are correctly implemented and handle potential errors. Also, verify that thegenState.ToBlock()conversion is correct and that the Genesis block is correctly written to the blockchain.eth/core/chain_reader.go (2)
52-53: The new method
HasBlockAndStatehas been added to theChainReaderinterface. Ensure that all implementations of this interface have been updated to include this method.296-303: The
WriteGenesisBlockmethod has been added. This method writes the genesis block to the database. Ensure that error handling is properly implemented in the calling code.cosmos/x/evm/keeper/processor.go (1)
- 35-65: The function
ProcessPayloadEnvelopereplaces the oldProcessTransactionfunction. It now takes aWrappedPayloadEnvelopeas input and returns aWrappedPayloadEnvelopeResponse. The function unmarshals the payload envelope, builds an EVM block, prepares plugins, inserts the block into the blockchain, consumes gas, and returns a response. Ensure that all calls to this function throughout the codebase have been updated to match the new signature and input/output types. Also, verify that the error handling and logging are consistent with the rest of the codebase.eth/core/chain_writer.go (6)
36-43: The
ChainWriterinterface has been updated with new methodsWriteGenesisBlock,LoadLastState, andInsertChain. Ensure that all implementations of this interface have been updated accordingly.49-79: The
InsertChainmethod has been updated to include a sanity check for the provided chain to ensure it is ordered and linked. This is a good practice to prevent errors during the insertion process.81-89: A new method
InsertBlockWithoutSetHeadhas been added. This method inserts a block without updating the canonical chain head. This could be useful in scenarios where you want to insert a block but not immediately update the chain head.91-146: The
insertChainmethod has been updated to only support inserting chains of length 1. This is a significant change and could potentially break existing code that relies on inserting chains of multiple blocks. Ensure that this change is compatible with the rest of your codebase.182-241: The
WriteBlockAndSetHeadmethod has been commented out and replaced with a no-op. This is a significant change and could potentially break existing code that relies on this method. Ensure that this change is compatible with the rest of your codebase.245-249: The
InsertBlockInternalmethod has been added. This method seems to be a replacement for theInsertBlockmethod in the old hunk. Ensure that all calls toInsertBlockhave been updated toInsertBlockInternal.eth/core/chain.go (6)
23-38: The new hunk introduces additional imports, including
"context","errors","github.com/ethereum/go-ethereum/core","pkg.berachain.dev/polaris/eth/consensus", and"pkg.berachain.dev/polaris/eth/core/vm". Ensure these new imports are used in the code and are necessary for the new functionality.50-53: The
Blockchaininterface has been extended with a new methodPreparePlugins(ctx context.Context). Ensure that all implementations of this interface have been updated to include this new method.68-70: New fields
engine,processor, andvalidatorhave been added to theblockchainstruct. Make sure these fields are properly initialized and used in the code.115-118: The
NewChainfunction now takes an additional argumentengine consensus.Engine. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.145-160: The
LoadLastStatefunction now callsPreparePlugins(ctx). This is a new behavior that might have side effects depending on the implementation ofPreparePlugins. Also, a new functionPreparePlugins(ctx context.Context)has been added to theblockchainstruct. Make sure this function is correctly implemented and does not introduce any unexpected behavior.169-180: The
loadLastStatefunction has been updated to handle the case wherenumber == 0separately. Ensure that this change does not introduce any unexpected behavior or bugs.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files ignored due to filter (15)
- cosmos/go.mod
- cosmos/go.sum
- e2e/localnet/go.mod
- e2e/localnet/go.sum
- e2e/precompile/go.mod
- e2e/precompile/go.sum
- e2e/testapp/go.mod
- e2e/testapp/go.sum
- eth/go.mod
- eth/go.sum
- go.work.sum
- lib/go.mod
- lib/go.sum
- tools/go.mod
- tools/go.sum
Files selected for processing (2)
- e2e/precompile/contracts/distribution/distribution_test.go (1 hunks)
- eth/core/chain_writer.go (1 hunks)
Files skipped from review due to trivial changes (1)
- e2e/precompile/contracts/distribution/distribution_test.go
Additional comments (Suppressed): 5
eth/core/chain_writer.go (5)
36-43: The
ChainWriterinterface has been updated with new methodsWriteGenesisBlock,LoadLastState, andInsertChain. Ensure that all implementations of this interface have been updated accordingly.49-80: The
InsertChainmethod has been updated to include a sanity check for the provided chain's order and linkage. This is a good practice to ensure the integrity of the chain.94-147: The
insertChainmethod has been updated to only support inserting chains of length 1. This is a significant change and could potentially break existing functionality if there are any parts of the codebase that rely on inserting chains of greater length. Ensure that this change is compatible with the rest of the codebase.183-242: The
WriteBlockAndSetHeadmethod and its internal implementationwriteBlockAndSetHeadhave been updated to be no-ops. This could potentially break existing functionality if there are any parts of the codebase that rely on these methods. Ensure that this change is compatible with the rest of the codebase.246-250: The
InsertBlockInternalmethod has been added. This method inserts a block into the canonical chain and updates the state of the blockchain. This seems to be a replacement for the oldInsertBlockmethod. Ensure that all calls toInsertBlockhave been updated toInsertBlockInternal.
itsdevbear
force-pushed
the
miner-1b-t
branch
from
6ce2ec8 to
4cb42f9
Compare
Summary by CodeRabbit
ProcessPayloadEnvelopefor processing payload envelopes.Minerstruct and its methods to use the newEnvelopeSerializerandGethMinerinterfaces.ProcessTransactionfunction toProcessPayloadEnvelopein theKeeperstruct, changing its functionality to handle payload envelopes.ChainWriterinterface and theblockchainstruct to handle batch block insertion and state updates.Polarisstruct.Distribution Precompiletest to wait for 3 blocks instead of 2.