feat(blob): improve gas estimation and track min gas price #2511

cmwaters · 2023-07-24T15:33:51Z

Overview

This PR incorporates the more recent gas estimation method in celestia-app. It also makes use of a new gRPC endpoint for consensus nodes that reveals the nodes min gas price. This enables the blob module to submit transactions with the correct gas price. If the node changes that price, the blob module is able to parse the error and retry using the new gas price.

Additionally, a stubbed CoreAccessor was introduced for when a core endpoint is not provided in order to return a better/more readable error, whereas previously, the CoreAccessor depended on the grpc dial to silently fail during start and any call to the state module would return a "cannot dial address" error.

It now looks like this:

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": 1,
    "message": "node is running without state access"
  }
}

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

blob/service.go

codecov-commenter · 2023-07-25T09:48:02Z

Codecov Report

Merging #2511 (b5cb4d5) into main (13e9b1f) will increase coverage by 0.17%.
The diff coverage is 39.32%.

@@            Coverage Diff             @@
##             main    #2511      +/-   ##
==========================================
+ Coverage   51.15%   51.32%   +0.17%     
==========================================
  Files         157      157              
  Lines       10433    10489      +56     
==========================================
+ Hits         5337     5384      +47     
- Misses       4628     4634       +6     
- Partials      468      471       +3

Files Changed	Coverage Δ
blob/helper.go	`53.96% <ø> (+6.08%)`	⬆️
blob/service.go	`72.83% <0.00%> (+3.37%)`	⬆️
state/metrics.go	`0.00% <0.00%> (ø)`
state/core_access.go	`35.14% <43.20%> (+7.17%)`	⬆️

... and 4 files with indirect coverage changes

cmwaters · 2023-07-25T09:49:15Z

Writing tests...

Wondertan

How complicated it is to do gas estimations and retries for all transactions and not just for SubmitPFB?

state/core_access_test.go

state/core_access.go

cmwaters · 2023-07-26T16:05:38Z

How complicated it is to do gas estimations and retries for all transactions and not just for SubmitPFB?

There's currently no one solution for all gas estimation. The best thing I can think of is hitting the /simulate endpoint but that becomes cumbersome if you need an extra round trip for every transaction. Getting the min gas price is easier to handle for all txs

cmwaters · 2023-07-27T12:29:06Z

I am not able to write all the tests I would like because of the functionality that celestia-app needs to provide in their test infra. I'm going to work on this (on celestia-app side). We can still merge this if we think logically it makes sense and I can make an issue and follow up on the missing tests in a different PR

state/core_access.go

renaynay · 2023-08-14T16:34:41Z

{
  "jsonrpc": "2.0",
  "id": 1,
  "error": {
    "code": 1,
    "message": "node is running without state access"
  }
}

nodebuilder/node_test.go

state/core_access.go

nodebuilder/state/core.go

nodebuilder/state/module.go

nodebuilder/state/core_accessor_stub.go

state/metrics.go

Wondertan

Requesting extraction of stub in a separate PR

…ndling error

constructor

…oint provided, add swamp test

…int not provided (celestiaorg#2577) This is a pre-requisite for celestiaorg#2511 It makes a check in CoreAccessor constructor to see if a core endpoint was provided, and returns nil CoreAccessor if not. A stubbed state module will then be provided if no core endpoint was provided so that errors are more readable. Previously, node start logic relied on the fact that the grpc Dial inside CoreAccessor.Start was non-blocking so it could silently fail under the hood and any calls made on state Module would return errors from the inability to reach the address that is the default for the core config (which was confusing).

…rg#2511) ## Overview This PR incorporates the more recent gas estimation method in celestia-app. It also makes use of a new gRPC endpoint for consensus nodes that reveals the nodes min gas price. This enables the blob module to submit transactions with the correct gas price. If the node changes that price, the blob module is able to parse the error and retry using the new gas price. Additionally, a stubbed CoreAccessor was introduced for when a core endpoint is not provided in order to return a better/more readable error, whereas previously, the CoreAccessor depended on the grpc dial to silently fail during start and any call to the state module would return a "cannot dial address" error. It now looks like this: ``` { "jsonrpc": "2.0", "id": 1, "error": { "code": 1, "message": "node is running without state access" } } ``` ## Checklist  - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords --------- Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>

…int not provided (celestiaorg#2577) This is a pre-requisite for celestiaorg#2511 It makes a check in CoreAccessor constructor to see if a core endpoint was provided, and returns nil CoreAccessor if not. A stubbed state module will then be provided if no core endpoint was provided so that errors are more readable. Previously, node start logic relied on the fact that the grpc Dial inside CoreAccessor.Start was non-blocking so it could silently fail under the hood and any calls made on state Module would return errors from the inability to reach the address that is the default for the core config (which was confusing). (cherry picked from commit 13e9b1f)

…rg#2511) ## Overview This PR incorporates the more recent gas estimation method in celestia-app. It also makes use of a new gRPC endpoint for consensus nodes that reveals the nodes min gas price. This enables the blob module to submit transactions with the correct gas price. If the node changes that price, the blob module is able to parse the error and retry using the new gas price. Additionally, a stubbed CoreAccessor was introduced for when a core endpoint is not provided in order to return a better/more readable error, whereas previously, the CoreAccessor depended on the grpc dial to silently fail during start and any call to the state module would return a "cannot dial address" error. It now looks like this: ``` { "jsonrpc": "2.0", "id": 1, "error": { "code": 1, "message": "node is running without state access" } } ``` ## Checklist  - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords --------- Co-authored-by: rene <41963722+renaynay@users.noreply.github.com> (cherry picked from commit bb9b4d4)

…int not provided (#2577) This is a pre-requisite for #2511 It makes a check in CoreAccessor constructor to see if a core endpoint was provided, and returns nil CoreAccessor if not. A stubbed state module will then be provided if no core endpoint was provided so that errors are more readable. Previously, node start logic relied on the fact that the grpc Dial inside CoreAccessor.Start was non-blocking so it could silently fail under the hood and any calls made on state Module would return errors from the inability to reach the address that is the default for the core config (which was confusing). (cherry picked from commit 13e9b1f)

…int not provided (celestiaorg#2577) This is a pre-requisite for celestiaorg#2511 It makes a check in CoreAccessor constructor to see if a core endpoint was provided, and returns nil CoreAccessor if not. A stubbed state module will then be provided if no core endpoint was provided so that errors are more readable. Previously, node start logic relied on the fact that the grpc Dial inside CoreAccessor.Start was non-blocking so it could silently fail under the hood and any calls made on state Module would return errors from the inability to reach the address that is the default for the core config (which was confusing). (cherry picked from commit 13e9b1f)

github-actions bot added the external Issues created by non node team members label Jul 24, 2023

vgonkivs added kind:feat Attached to feature PRs area:blob and removed external Issues created by non node team members labels Jul 24, 2023

Wondertan requested changes Jul 24, 2023

View reviewed changes

blob/service.go Outdated Show resolved Hide resolved

cmwaters marked this pull request as ready for review July 25, 2023 13:17

cmwaters requested review from renaynay, vgonkivs, distractedm1nd and walldiss as code owners July 25, 2023 13:17

Wondertan reviewed Jul 26, 2023

View reviewed changes

state/core_access_test.go Outdated Show resolved Hide resolved

state/core_access.go Outdated Show resolved Hide resolved

state/core_access.go Outdated Show resolved Hide resolved

state/core_access.go Show resolved Hide resolved

state/core_access.go Show resolved Hide resolved

cmwaters requested a review from Wondertan August 2, 2023 13:57

Wondertan reviewed Aug 2, 2023

View reviewed changes

state/core_access.go Outdated Show resolved Hide resolved

Wondertan reviewed Aug 2, 2023

View reviewed changes

state/core_access.go Outdated Show resolved Hide resolved

cmwaters requested a review from Wondertan August 2, 2023 14:01

Wondertan reviewed Aug 2, 2023

View reviewed changes

state/core_access.go Show resolved Hide resolved

cmwaters requested a review from Wondertan August 7, 2023 13:17

renaynay reviewed Aug 8, 2023

View reviewed changes

state/core_access.go Outdated Show resolved Hide resolved

state/core_access.go Outdated Show resolved Hide resolved

cmwaters force-pushed the cal/gas-estimation branch from ffa79a8 to 1b91c0d Compare August 10, 2023 17:14

cmwaters requested a review from renaynay August 10, 2023 17:15

Wondertan assigned cmwaters Aug 14, 2023

Wondertan mentioned this pull request Aug 15, 2023

chore: extract out signer into a separate package celestiaorg/celestia-app#2238

Closed

renaynay reviewed Aug 15, 2023

View reviewed changes

nodebuilder/node_test.go Outdated Show resolved Hide resolved

Wondertan reviewed Aug 16, 2023

View reviewed changes

Wondertan requested changes Aug 16, 2023

View reviewed changes

cmwaters and others added 14 commits August 22, 2023 14:18

lint

ac2e24d

use mutex also for read calls

3031148

clean up test

3d34344

remove todo

9cf824b

lint

17b2041

set min gas price in start method and ensure fee gets updated when ha…

d89bcfb

…ndling error

fix(state): reset minGasPrice during retry & remove minGasprice from

da6dda4

constructor

remove test

a738025

feat(nodebuilder/core): Provide stubbed core accessor if no core endp…

de11fbb

…oint provided, add swamp test

remove swamp test

8f3956a

reset swamp

5c61beb

reset core/testing

637e606

remove file

50826df

fix: resolve relics from merge conflict

ff233de

renaynay force-pushed the cal/gas-estimation branch from ddf10b4 to ff233de Compare August 22, 2023 12:27

Wondertan approved these changes Aug 22, 2023

View reviewed changes

vgonkivs approved these changes Aug 22, 2023

View reviewed changes

renaynay enabled auto-merge (squash) August 22, 2023 15:34

renaynay disabled auto-merge August 22, 2023 15:34

renaynay enabled auto-merge (squash) August 22, 2023 15:37

renaynay merged commit bb9b4d4 into celestiaorg:main Aug 22, 2023
13 of 15 checks passed

renaynay mentioned this pull request Sep 25, 2023

feat(state): Query app endpoint for min gas price on node start #2498

Closed

faddat mentioned this pull request Feb 22, 2024

feat: go 1.22 and pebble v1.1.0 for celestia-node #3202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(blob): improve gas estimation and track min gas price #2511

feat(blob): improve gas estimation and track min gas price #2511

cmwaters commented Jul 24, 2023 •

edited by renaynay

Loading

codecov-commenter commented Jul 25, 2023 •

edited

Loading

cmwaters commented Jul 25, 2023

Wondertan left a comment

cmwaters commented Jul 26, 2023

cmwaters commented Jul 27, 2023

renaynay commented Aug 14, 2023

Wondertan left a comment

feat(blob): improve gas estimation and track min gas price #2511

feat(blob): improve gas estimation and track min gas price #2511

Conversation

cmwaters commented Jul 24, 2023 • edited by renaynay Loading

Overview

Checklist

codecov-commenter commented Jul 25, 2023 • edited Loading

Codecov Report

cmwaters commented Jul 25, 2023

Wondertan left a comment

Choose a reason for hiding this comment

cmwaters commented Jul 26, 2023

cmwaters commented Jul 27, 2023

renaynay commented Aug 14, 2023

Wondertan left a comment

Choose a reason for hiding this comment

cmwaters commented Jul 24, 2023 •

edited by renaynay

Loading

codecov-commenter commented Jul 25, 2023 •

edited

Loading