feat(miner): Add NewPayloadTimeout to the miner.
#1244
Conversation
WalkthroughThis update introduces changes to the transaction handling and mining processes in the Cosmos blockchain. It modifies the transaction recheck limit, introduces a payload timeout for miners, and adjusts the mempool size. It also refactors the runtime setup and miner creation for better maintainability and performance. Changes
TipsChat with CodeRabbit Bot (
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1244 +/- ##
=======================================
Coverage 49.14% 49.14%
=======================================
Files 77 77
Lines 4631 4631
=======================================
Hits 2276 2276
Misses 2192 2192
Partials 163 163 |
Signed-off-by: Devon Bear <[email protected]>
![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: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- cosmos/go.mod
- cosmos/go.sum
Files selected for processing (4)
- cosmos/lib/ante/eject.go (1 hunks)
- cosmos/miner/miner.go (5 hunks)
- cosmos/runtime/runtime.go (1 hunks)
- e2e/testapp/polard/cmd/root.go (1 hunks)
Files skipped from review due to trivial changes (1)
- e2e/testapp/polard/cmd/root.go
Additional comments: 9
cosmos/runtime/runtime.go (2)
93-94: The creation of
WrappedMinerandWrappedTxPoolhas been moved from theBuildfunction to theNewfunction. This change could potentially affect the lifecycle of these objects. Ensure that this change does not introduce any lifecycle issues or race conditions, especially ifNewandBuildare called from different goroutines.103-103: The
ek.Setupfunction call has been removed. If this function was responsible for any crucial setup steps, its removal could lead to issues. Please verify that the setup is still correctly performed elsewhere.cosmos/lib/ante/eject.go (2)
62-62: The threshold for ejecting a transaction from the mempool has been reduced from 25 to 15 rechecks. Ensure that this change does not negatively impact the system's performance or the user experience.
64-64: The error message has been updated to provide more context about the error. This is a good practice as it helps in debugging and understanding the error's cause.
cosmos/miner/miner.go (5)
26-26: The
timepackage is imported to handle the newpayloadTimeoutfeature. This is necessary for the timeout functionality.54-61: A new field
payloadTimeoutis added to theMinerstruct and is initialized in theNewfunction. This is a good practice as it allows the timeout duration to be configurable.84-89: The
buildBlockfunction now accepts acontext.Contextargument and uses it to callsubmitPayloadForBuildingandresolveEnvelope. This is a good practice as it allows for better control over timeouts and cancellations.143-154: The
resolvePayloadfunction is a helper function that resolves the payload in a separate goroutine. It sends the result and any error to the provided channels. This is a good practice as it allows for better control over timeouts and cancellations.157-159: The
resolveEmptyPayloadfunction is a helper function that resolves an empty payload. This is a good practice as it allows for better control over timeouts and cancellations.
| func (m *Miner) resolveEnvelope(ctx context.Context, timeout time.Duration) ([]byte, error) { | ||
| sCtx := sdk.UnwrapSDKContext(ctx).Logger() | ||
| ctx, cancel := context.WithTimeout(ctx, timeout) | ||
| defer cancel() | ||
|
|
||
| resultChan := make(chan []byte, 1) | ||
| errChan := make(chan error, 1) | ||
|
|
||
| go m.resolvePayload(resultChan, errChan) | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| // If we timed out, return an empty payload. | ||
| // TODO: penalize validators for not being able to deliver the payload? | ||
| sCtx.Error("failed to resolve envelope, proposing empty payload", "err", ctx.Err()) | ||
| return m.resolveEmptyPayload() | ||
| case result := <-resultChan: | ||
| sdk.UnwrapSDKContext(ctx).Logger().Info("successfully resolved envelope") | ||
| return result, <-errChan | ||
| } | ||
| } |
There was a problem hiding this comment.
The resolveEnvelope function now accepts a context.Context and a timeout argument. It creates a new context with the provided timeout and uses it to resolve the payload in a separate goroutine. If the context times out, it logs an error and returns an empty payload. This is a good practice as it allows for better control over timeouts and cancellations. However, the error handling could be improved. Currently, if the context times out, the function logs an error and returns an empty payload. It might be better to return the error to the caller so that they can decide how to handle it.
- sCtx.Error("failed to resolve envelope, proposing empty payload", "err", ctx.Err())
- return m.resolveEmptyPayload()
+ return nil, fmt.Errorf("failed to resolve envelope: %w", ctx.Err())Committable suggestion (Beta)
| func (m *Miner) resolveEnvelope(ctx context.Context, timeout time.Duration) ([]byte, error) { | |
| sCtx := sdk.UnwrapSDKContext(ctx).Logger() | |
| ctx, cancel := context.WithTimeout(ctx, timeout) | |
| defer cancel() | |
| resultChan := make(chan []byte, 1) | |
| errChan := make(chan error, 1) | |
| go m.resolvePayload(resultChan, errChan) | |
| select { | |
| case <-ctx.Done(): | |
| // If we timed out, return an empty payload. | |
| // TODO: penalize validators for not being able to deliver the payload? | |
| sCtx.Error("failed to resolve envelope, proposing empty payload", "err", ctx.Err()) | |
| return m.resolveEmptyPayload() | |
| case result := <-resultChan: | |
| sdk.UnwrapSDKContext(ctx).Logger().Info("successfully resolved envelope") | |
| return result, <-errChan | |
| } | |
| } | |
| return nil, fmt.Errorf("failed to resolve envelope: %w", ctx.Err()) |
|
tbh we don't need this |
Summary by CodeRabbit
These changes aim to improve the system's efficiency, responsiveness, and maintainability, providing a smoother and more reliable user experience.