Skip to content

Commit 553e399

Browse files
tuxcanflytuxcanfly
authored
fix(manager): only reset custom gas price (#1595)
## Overview This PR fixes custom gas price reset after a successful submission. ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Modified the logic for adjusting `gasPrice` to prevent division operation when not required. - **Refactor** - Refactored test cases for submitting blocks to a mock DA to cover different scenarios. - **Validation** - Added a validation check for `DAGasMultiplier` in `nodeConfig` to meet specific criteria. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent b3ad562 commit 553e399

File tree

5 files changed

+80
-50
lines changed

5 files changed

+80
-50
lines changed

‎block/manager.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -901,7 +901,7 @@ func (m *Manager) submitBlocksToDA(ctx context.Context) error {
901901
res := m.dalc.SubmitBlocks(ctx, blocksToSubmit, maxBlobSize, gasPrice)
902902
switch res.Code {
903903
case da.StatusSuccess:
904-
m.logger.Info("successfully submitted Rollkit blocks to DA layer", "daHeight", res.DAHeight, "count", res.SubmittedCount)
904+
m.logger.Info("successfully submitted Rollkit blocks to DA layer", "gasPrice", gasPrice, "daHeight", res.DAHeight, "count", res.SubmittedCount)
905905
if res.SubmittedCount == uint64(len(blocksToSubmit)) {
906906
submittedAllBlocks = true
907907
}
@@ -920,15 +920,17 @@ func (m *Manager) submitBlocksToDA(ctx context.Context) error {
920920
// scale back gasPrice gradually
921921
backoff = initialBackoff
922922
maxBlobSize = initialMaxBlobSize
923-
gasPrice = gasPrice / m.dalc.GasMultiplier
924-
if gasPrice < initialGasPrice {
925-
gasPrice = initialGasPrice
923+
if m.dalc.GasMultiplier > 0 && gasPrice != -1 {
924+
gasPrice = gasPrice / m.dalc.GasMultiplier
925+
if gasPrice < initialGasPrice {
926+
gasPrice = initialGasPrice
927+
}
926928
}
927929
m.logger.Debug("resetting DA layer submission options", "backoff", backoff, "gasPrice", gasPrice, "maxBlobSize", maxBlobSize)
928930
case da.StatusNotIncludedInBlock, da.StatusAlreadyInMempool:
929931
m.logger.Error("DA layer submission failed", "error", res.Message, "attempt", attempt)
930932
backoff = m.conf.DABlockTime * time.Duration(m.conf.DAMempoolTTL)
931-
if m.dalc.GasMultiplier != -1 && gasPrice != -1 {
933+
if m.dalc.GasMultiplier > 0 && gasPrice != -1 {
932934
gasPrice = gasPrice * m.dalc.GasMultiplier
933935
}
934936
m.logger.Info("retrying DA layer submission with", "backoff", backoff, "gasPrice", gasPrice, "maxBlobSize", maxBlobSize)

‎block/manager_test.go‎

Lines changed: 67 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import (
2828

2929
// Returns a minimalistic block manager
3030
func getManager(t *testing.T, backend goDA.DA) *Manager {
31-
logger := test.NewFileLoggerCustom(t, test.TempLogFileName(t, t.Name()))
31+
logger := test.NewLogger(t)
3232
return &Manager{
3333
dalc: da.NewDAClient(backend, -1, -1, nil, logger),
3434
blockCache: NewBlockCache(),
@@ -138,49 +138,73 @@ func TestIsDAIncluded(t *testing.T) {
138138
func TestSubmitBlocksToMockDA(t *testing.T) {
139139
ctx := context.Background()
140140

141-
mockDA := &mock.MockDA{}
142-
m := getManager(t, mockDA)
143-
m.conf.DABlockTime = time.Millisecond
144-
m.conf.DAMempoolTTL = 1
145-
m.dalc.GasPrice = 1.0
146-
m.dalc.GasMultiplier = 1.2
147-
kvStore, err := store.NewDefaultInMemoryKVStore()
148-
require.NoError(t, err)
149-
m.store = store.New(kvStore)
150-
151-
t.Run("handle_tx_already_in_mempool", func(t *testing.T) {
152-
var blobs [][]byte
153-
block := types.GetRandomBlock(1, 5)
154-
blob, err := block.MarshalBinary()
155-
require.NoError(t, err)
156-
157-
err = m.store.SaveBlock(ctx, block, &types.Commit{})
158-
require.NoError(t, err)
159-
m.store.SetHeight(ctx, 1)
160-
161-
blobs = append(blobs, blob)
162-
// Set up the mock to
163-
// * throw timeout waiting for tx to be included exactly once
164-
// * wait for tx to drop from mempool exactly DABlockTime * DAMempoolTTL seconds
165-
// * retry with a higher gas price
166-
// * successfully submit
167-
mockDA.On("MaxBlobSize").Return(uint64(12345), nil)
168-
mockDA.
169-
On("Submit", blobs, 1.0, []byte(nil)).
170-
Return([][]byte{}, da.ErrTxTimedout).Once()
171-
mockDA.
172-
On("Submit", blobs, 1.0*1.2, []byte(nil)).
173-
Return([][]byte{}, da.ErrTxAlreadyInMempool).Times(int(m.conf.DAMempoolTTL))
174-
mockDA.
175-
On("Submit", blobs, 1.0*1.2*1.2, []byte(nil)).
176-
Return([][]byte{bytes.Repeat([]byte{0x00}, 8)}, nil)
141+
testCases := []struct {
142+
name string
143+
gasPrice float64
144+
gasMultiplier float64
145+
expectedGasPrices []float64
146+
isErrExpected bool
147+
}{
148+
{"defaults", -1, -1, []float64{
149+
-1, -1, -1,
150+
}, false},
151+
{"fixed_gas_price", 1.0, -1, []float64{
152+
1.0, 1.0, 1.0,
153+
}, false},
154+
{"default_gas_price_with_multiplier", -1, 1.2, []float64{
155+
-1, -1, -1,
156+
}, false},
157+
{"fixed_gas_price_with_multiplier", 1.0, 1.2, []float64{
158+
1.0, 1.2, 1.2 * 1.2,
159+
}, false},
160+
}
177161

178-
m.pendingBlocks, err = NewPendingBlocks(m.store, m.logger)
179-
require.NoError(t, err)
180-
err = m.submitBlocksToDA(ctx)
181-
require.NoError(t, err)
182-
mockDA.AssertExpectations(t)
183-
})
162+
for _, tc := range testCases {
163+
t.Run(tc.name, func(t *testing.T) {
164+
mockDA := &mock.MockDA{}
165+
m := getManager(t, mockDA)
166+
m.conf.DABlockTime = time.Millisecond
167+
m.conf.DAMempoolTTL = 1
168+
kvStore, err := store.NewDefaultInMemoryKVStore()
169+
require.NoError(t, err)
170+
m.store = store.New(kvStore)
171+
172+
var blobs [][]byte
173+
block := types.GetRandomBlock(1, 5)
174+
blob, err := block.MarshalBinary()
175+
require.NoError(t, err)
176+
177+
err = m.store.SaveBlock(ctx, block, &types.Commit{})
178+
require.NoError(t, err)
179+
m.store.SetHeight(ctx, 1)
180+
181+
m.dalc.GasPrice = tc.gasPrice
182+
m.dalc.GasMultiplier = tc.gasMultiplier
183+
184+
blobs = append(blobs, blob)
185+
// Set up the mock to
186+
// * throw timeout waiting for tx to be included exactly twice
187+
// * wait for tx to drop from mempool exactly DABlockTime * DAMempoolTTL seconds
188+
// * retry with a higher gas price
189+
// * successfully submit
190+
mockDA.On("MaxBlobSize").Return(uint64(12345), nil)
191+
mockDA.
192+
On("Submit", blobs, tc.expectedGasPrices[0], []byte(nil)).
193+
Return([][]byte{}, da.ErrTxTimedout).Once()
194+
mockDA.
195+
On("Submit", blobs, tc.expectedGasPrices[1], []byte(nil)).
196+
Return([][]byte{}, da.ErrTxTimedout).Once()
197+
mockDA.
198+
On("Submit", blobs, tc.expectedGasPrices[2], []byte(nil)).
199+
Return([][]byte{bytes.Repeat([]byte{0x00}, 8)}, nil)
200+
201+
m.pendingBlocks, err = NewPendingBlocks(m.store, m.logger)
202+
require.NoError(t, err)
203+
err = m.submitBlocksToDA(ctx)
204+
require.NoError(t, err)
205+
mockDA.AssertExpectations(t)
206+
})
207+
}
184208
}
185209

186210
func TestSubmitBlocksToDA(t *testing.T) {

‎cmd/rollkit/docs/rollkit_start.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ rollkit start [flags]
3434
--rollkit.da_address string DA address (host:port) (default "http://localhost:26658")
3535
--rollkit.da_auth_token string DA auth token
3636
--rollkit.da_block_time duration DA chain block time (for syncing) (default 15s)
37-
--rollkit.da_gas_multiplier float DA gas price multiplier for retrying blob transactions (default -1)
37+
--rollkit.da_gas_multiplier float DA gas price multiplier for retrying blob transactions
3838
--rollkit.da_gas_price float DA gas price for blob transactions (default -1)
3939
--rollkit.da_namespace string DA namespace to submit blob transactions
4040
--rollkit.da_start_height uint starting DA block height (for syncing)

‎config/defaults.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ var DefaultNodeConfig = NodeConfig{
2828
},
2929
DAAddress: "http://localhost:26658",
3030
DAGasPrice: -1,
31-
DAGasMultiplier: -1,
31+
DAGasMultiplier: 0,
3232
Light: false,
3333
HeaderConfig: HeaderConfig{
3434
TrustedHash: "",

‎node/full.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -228,6 +228,10 @@ func initDALC(nodeConfig config.NodeConfig, dalcKV ds.TxnDatastore, logger log.L
228228
return nil, fmt.Errorf("error decoding namespace: %w", err)
229229
}
230230

231+
if nodeConfig.DAGasMultiplier < 0 {
232+
return nil, fmt.Errorf("gas multiplier must be greater than or equal to zero")
233+
}
234+
231235
client, err := proxyda.NewClient(nodeConfig.DAAddress, nodeConfig.DAAuthToken)
232236
if err != nil {
233237
return nil, fmt.Errorf("error while establishing connection to DA layer: %w", err)

0 commit comments

Comments
 (0)