Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(state): Integrate new Signer for concurrent tx submission #3298

Merged
merged 14 commits into from
Apr 18, 2024

Conversation

cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Apr 5, 2024

Closes: #3296

This adds the Signer struct from celestia-app's user package.

Signer keeps track of both the network and local sequence number allowing for multiple messages to be signed within a single block.

This also uses the EstimateGas function of the signer to estimate the gas, when the user provides 0 as the gasLim

TODO: we should probably introduce some gas multiplier (default: 1.1) to the EstimateGas as it seems to underestimate the value (cc @vgonkivs)

@github-actions github-actions bot added the external Issues created by non node team members label Apr 5, 2024
@cmwaters cmwaters changed the title feat: integrate signer for concurrent tx submission feat(state): integrate signer for concurrent tx submission Apr 5, 2024
@renaynay renaynay changed the title feat(state): integrate signer for concurrent tx submission feat(state): Integrate new Signer for concurrent tx submission Apr 5, 2024
@renaynay renaynay added area:state Related to fetching state and state execution kind:feat Attached to feature PRs labels Apr 5, 2024
@renaynay
Copy link
Member

renaynay commented Apr 5, 2024

There are a lot more relevant issues that this PR would close.

@vgonkivs
Copy link
Member

vgonkivs commented Apr 5, 2024

TODO: we should probably introduce some gas multiplier (default: 1.1) to the EstimateGas as it seems to underestimate the value (cc @vgonkivs)

Agree, bc it seems that neither EstimateGas nor Simulate don't include tx options in the calculation and we are running out of gas. Should this multiplier be dynamically changed depending on the amount of options?
I'm also wondering, should the node be aware of this multiplier or it should be encapsulated in the EstimateGas?

@cmwaters
Copy link
Contributor Author

cmwaters commented Apr 5, 2024

Should this multiplier be dynamically changed depending on the amount of options?

I think it should be fixed. We should have a sane conservative default (like 1.2x) and add a field in the relevant config for users to change it if they need to. Fees are so cheap I don't think people will mind slightly overestimating the gas in exchange for better UX

@renaynay
Copy link
Member

renaynay commented Apr 8, 2024

We talked about this in-sync but we need a way for the signer to be able to start with a key that may not "exist" yet (unfunded account). Otherwise construction of Signer is not possible for a newly instantiated node where the account is not yet funded.

cc @cmwaters

@renaynay
Copy link
Member

renaynay commented Apr 8, 2024

After further discussion, @cmwaters and I agreed that it might make more sense to handle for the case that node is started with an unfunded account inside of node codebase rather than inside the signer as the signer is just a wrapper for writes to state which cannot happen when the account is not funded.

@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 22.60870% with 89 lines in your changes are missing coverage. Please review.

Project coverage is 44.88%. Comparing base (2469e7a) to head (3e1a15b).
Report is 28 commits behind head on main.

Files Patch % Lines
state/core_access.go 22.10% 70 Missing and 4 partials ⚠️
nodebuilder/state/core.go 0.00% 4 Missing ⚠️
nodebuilder/state/keyring.go 0.00% 4 Missing ⚠️
nodebuilder/state/opts.go 0.00% 4 Missing ⚠️
nodebuilder/state/module.go 0.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3298      +/-   ##
==========================================
+ Coverage   44.83%   44.88%   +0.05%     
==========================================
  Files         265      272       +7     
  Lines       14620    15120     +500     
==========================================
+ Hits         6555     6787     +232     
- Misses       7313     7567     +254     
- Partials      752      766      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@renaynay
Copy link
Member

renaynay commented Apr 9, 2024

@todo @renaynay @cmwaters:

Make signer configurable to not need to wait for inclusion of blobs to return (general config for the signer)

Is this actually necessary ^ ?

@renaynay
Copy link
Member

renaynay commented Apr 9, 2024

Waiting on response regarding defaults for state module blob submission

@renaynay
Copy link
Member

TODO: we should probably introduce some gas multiplier (default: 1.1) to the EstimateGas as it seems to underestimate the value (cc @vgonkivs)

Let's do this in a later PR

Copy link
Contributor Author

@cmwaters cmwaters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me @renaynay

I will cut v1.8.0 now

UPDATE: v1.8.0 has been released

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, it only took 4 months to get it out 😬

Is there anything that SubmitTx could benefit from using the Signer? Like retries?
cc @cmwaters

nodebuilder/state/keyring.go Outdated Show resolved Hide resolved
state/core_access.go Outdated Show resolved Hide resolved
state/core_access.go Show resolved Hide resolved
@renaynay renaynay requested a review from Wondertan April 17, 2024 11:03
nodebuilder/state/core.go Outdated Show resolved Hide resolved
@renaynay renaynay requested a review from vgonkivs April 18, 2024 11:01
go.mod Show resolved Hide resolved
@renaynay renaynay enabled auto-merge (squash) April 18, 2024 13:28
@renaynay renaynay merged commit 6382dc1 into celestiaorg:main Apr 18, 2024
26 checks passed
@renaynay renaynay deleted the cal/integrate-signer branch April 18, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:state Related to fetching state and state execution external Issues created by non node team members kind:feat Attached to feature PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: async blob submissions
6 participants