Conversation
|
The latest Buf updates on your PR. Results from workflow CI and Release / buf-check (pull_request).
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @alpe, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've been working on a significant spike to introduce a mechanism for "direct transactions." The core idea is to allow certain transactions to be submitted directly to the Data Availability (DA) layer, bypassing the traditional mempool. My changes introduce a DirectTxReaper that actively fetches these transactions from DA and a DirectTxSequencer that prioritizes their inclusion in blocks. This provides a robust way to ensure transactions are processed even if they don't go through the standard mempool, which is particularly useful for forced inclusion scenarios. I've also updated the node's configuration to support parameters for controlling this forced inclusion behavior and added an end-to-end test to validate the entire flow.
Highlights
- Direct Transaction Reaper: I've introduced the
DirectTxReapercomponent, which actively monitors the Data Availability (DA) layer for direct transactions. This reaper is responsible for fetching these transactions and submitting them to the sequencer for inclusion, ensuring that transactions posted directly to DA are eventually processed. - Direct Transaction Sequencer: I've implemented a new
DirectTxSequencerinterface and a concrete implementation that wraps an existing sequencer. This new sequencer prioritizes direct transactions retrieved by theDirectTxReaperbefore considering transactions from the regular mempool. This design allows for flexible integration without altering the underlying sequencer logic. - Configurable Forced Inclusion: I've updated the node's configuration to include
ForcedInclusionConfigparameters, such asMaxInclusionDelayandMinDADelay. These settings will allow for fine-grained control over when direct transactions, once seen on the DA layer, must be included in a block. - End-to-End Testing for Direct Transactions: I've added a new end-to-end test (
direct_tx_da_e2e_test.go) to validate the entire flow of direct transaction force inclusion. This test simulates submitting a transaction directly to the DA layer and then verifies its eventual inclusion in a block, confirming the functionality works as intended.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a 'Direct TX' feature, allowing transactions to be submitted directly to the Data Availability layer. The implementation is a good starting point for this spike, but it contains several critical issues that need to be addressed. These include potential panics, incorrect hashing logic, and bugs in data processing loops. There are also several hardcoded values that should be made configurable for better flexibility. The new tests have some issues as well, such as incorrect mock implementations. Addressing these points will significantly improve the robustness and correctness of this new feature.
| //} | ||
| h := m.headerCache.GetItem(daHeight) | ||
| if h == nil { | ||
| panic("header not found in cache") // todo (Alex): not sure if headers are always available before data. better assume not |
There was a problem hiding this comment.
Using panic here can crash the entire node if a header is not found in the cache. While the todo comment acknowledges this, it's a critical issue. The function should handle this case gracefully by logging an error and returning false, especially since it's possible for data to be available before its corresponding header.
| panic("header not found in cache") // todo (Alex): not sure if headers are always available before data. better assume not | |
| m.logger.Debug("header not found in cache, height:", daHeight) | |
| return false |
| //return false | ||
| } | ||
| for _, tx := range unsignedData.Txs { | ||
| txHash := sha256.New().Sum(tx) |
There was a problem hiding this comment.
The transaction hashing logic is incorrect. sha256.New().Sum(tx) computes the hash of an empty input and appends the result to tx, which is not the intended behavior. To compute the SHA256 hash of the transaction, you should use sha256.Sum256(tx).
txHashBytes := sha256.Sum256(tx)
txHash := txHashBytes[:]| txHash := sha256.New().Sum(tx) | ||
| d := DirectTransaction{ | ||
| TxHash: txHash, | ||
| TX: unsignedData.Txs[0], |
There was a problem hiding this comment.
There is a bug in this loop. The code is iterating over unsignedData.Txs with the loop variable tx, but inside the DirectTransaction struct, it's always assigning unsignedData.Txs[0]. This means every transaction in the DirectTransaction list will have the content of the first transaction. It should use the loop variable tx instead.
TX: tx,| if !has { | ||
| newTxs = append(newTxs, sequencer.DirectTX{ | ||
| TX: tx, | ||
| ID: result.IDs[i], |
There was a problem hiding this comment.
There's a critical bug in how the blob ID is being assigned to direct transactions. The code uses the transaction's index within a blob (i) to look up an ID from result.IDs, which is a list of blob IDs. This is incorrect because all transactions within a single blob should share the same blob ID. Furthermore, if a blob contains more transactions than there are total blob IDs for that height, this will cause a panic due to an index out-of-range error.
The logic should be to iterate through blobs with their index to get the corresponding blob ID, and then assign that same blob ID to all transactions found within that blob.
| func (m *MockDirectTxSequencer) SubmitDirectTxs(ctx context.Context, txs [][]byte) error { | ||
| args := m.Called(ctx, txs) | ||
| return args.Error(0) |
There was a problem hiding this comment.
The mock implementation of SubmitDirectTxs has an incorrect signature. The interface expects ...sequencer.DirectTX, but the mock is defined with [][]byte. This will cause a compilation error because the mock does not satisfy the DirectTxSequencer interface. The test needs to be updated to use the correct type.
| func (m *MockDirectTxSequencer) SubmitDirectTxs(ctx context.Context, txs [][]byte) error { | |
| args := m.Called(ctx, txs) | |
| return args.Error(0) | |
| func (m *MockDirectTxSequencer) SubmitDirectTxs(ctx context.Context, txs ...sequencer.DirectTX) error { | |
| args := m.Called(ctx, txs) | |
| return args.Error(0) |
| return err | ||
| } | ||
|
|
||
| directTXSeq := single.NewDirectTxSequencer(sequencer, logger, datastore, 100) // todo (Alex): find a good default |
There was a problem hiding this comment.
The maxSize for the DirectTxSequencer is hardcoded to 100. This should be exposed as a configuration parameter to avoid magic numbers in the code and allow for easier adjustments.
| DAIncludedHeight: m.daIncludedHeight.Load(), | ||
| Id: []byte(m.genesis.ChainID), | ||
| LastBatchData: m.lastBatchData, | ||
| MaxBytes: defaultMaxBytes, // todo (Alex): do we need to reserve some space for headers and other data? |
There was a problem hiding this comment.
The defaultMaxBytes constant is hardcoded. This value should be configurable, for example, through the node's configuration file. This would provide more flexibility for different deployment scenarios.
| err = bq.db.Delete(ctx, ds.NewKey(key)) | ||
| if err != nil { | ||
| // Log the error but continue | ||
| fmt.Printf("Error deleting processed batch: %v\n", err) |
There was a problem hiding this comment.
Using fmt.Printf for logging is not ideal. It bypasses the structured logger, making logs harder to parse, filter, and manage, especially in a production environment. Please use the provided logger instance instead.
| fmt.Printf("Error reading entry from datastore: %v\n", result.Error) | ||
| continue | ||
| } | ||
| var pbBatch pb.DirectTX | ||
| err := proto.Unmarshal(result.Value, &pbBatch) | ||
| if err != nil { | ||
| fmt.Printf("Error decoding batch for key '%s': %v. Skipping entry.\n", result.Key, err) | ||
| continue |
There was a problem hiding this comment.
Error conditions are being logged using fmt.Printf. This should be replaced with the structured logger (bq.logger if available, or passed in) to ensure consistent and manageable logging. Direct prints to stdout/stderr should be avoided.
| require.Eventually(t, func() bool { | ||
| if evm.CheckTxIncluded(t, tx.Hash()) { | ||
| return true | ||
| } | ||
| //go func() { // submit any TX to trigger block creation | ||
| // tx := evm.GetRandomTransaction(t, TestPrivateKey, TestToAddress, DefaultChainID, DefaultGasLimit, &globalNonce) | ||
| // evm.SubmitTransaction(t, tx) | ||
| //}() | ||
| return false | ||
| }, 1*time.Second, 500*time.Millisecond, "Transaction should be included in a block") |
There was a problem hiding this comment.
The timeout for this require.Eventually is set to 1 second, which might be too short for an end-to-end test that involves DA layer interaction and block production. This could lead to flaky tests. Consider increasing the timeout to a more generous value, like 10 or 15 seconds, to ensure the test is reliable.
| random bytes | ||
| - Should we always include direct TX although they may be duplicates to mempool TX? spike: yes | ||
| - Should we fill the block space with directTX if possible or reserve space for mempool TX | ||
|
|
There was a problem hiding this comment.
What should we do when a sequencer was not able to add a direct-TX within the time window?
|
|
||
| // SubmitDirectTxs adds direct transactions to the sequencer. | ||
| // This method is called by the DirectTxReaper. | ||
| SubmitDirectTxs(ctx context.Context, txs ...DirectTX) error |
There was a problem hiding this comment.
is this for submission to the DA layer?
There was a problem hiding this comment.
This method is for submitting direct-tx from the DA to the sequencer. The name is analogue to SubmitBatchTxs which is used to submit tx from the mempool to the sequencer. The name is not very intuitive though.
|
Good spike, super useful to me understand a few concepts. |
3 participants
🚧 WIP - early version
Introduces
DirectTxSequencer- an adapter to the existing sequencer that prepends direct-TX on next batch requestDirectTxReaper- worker that pulls direct-TX from the DA and sends them to theDirectTxSequencercoresequencer.DirectTX- transfer objectDirectTXBatchQueue- persistent queue forDirectTXpb.DirectTX- protobuf to persistent objects inDirectTXBatchQueueto maintain stateOverview