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

[actpool] persistent storage for actions #4366

Merged
merged 6 commits into from
Sep 18, 2024

Conversation

envestcc
Copy link
Member

Description

Now actions in actpool will be persistent on disk. There are three main functionalities:

  • persistence: It will save an action when it's accepted by actpool, with configureable datacap
  • load-on-start: all persistent actions will be load at actpool start
  • reload periodically: actions dropped in memory will be attemped to reload into memory periodically

Based on #4362

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • [] make test
  • [] fullsync
  • [] Other test (please specify)

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • [] My code follows the style guidelines of this project
  • [] I have performed a self-review of my code
  • [] I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • [] My changes generate no new warnings
  • [] I have added tests that prove my fix is effective or that my feature works
  • [] New and existing unit tests pass locally with my changes
  • [] Any dependent changes have been merged and published in downstream modules

Copy link
Collaborator

@CoderZhi CoderZhi left a comment

Choose a reason for hiding this comment

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

the others look good to me

chainservice/builder.go Outdated Show resolved Hide resolved
@@ -0,0 +1,245 @@
package actpool
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not a blobstore, but a general store

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to actionstore

@dustinxie
Copy link
Member

as discussed, a general comment is we should only enable this for blob tx, b/c each tx is 128kB and each block can take only 2 blobs, leading to a potential backlog of such txs.
For regular tx (size is ~100 bytes, and don't have limitation in each block), it is not needed to persist them locally. Because after a restart, the stored txs are all either already added to past blocks, or purged due to processing error (like wrong chainID), seems useless to me to persist these regular tx

var defaultBlobStoreConfig = blobStoreConfig{
Datadir: "blobpool",
var defaultActionStoreConfig = actionStoreConfig{
Datadir: "actionstore",
Datacap: 10 * 1024 * 1024 * 1024,
Copy link
Member

Choose a reason for hiding this comment

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

is this 10GB?

Copy link
Member Author

@envestcc envestcc Sep 11, 2024

Choose a reason for hiding this comment

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

ok, maybe 1GB(around 5000 blob tx) is enough

actpool/actionstore.go Outdated Show resolved Hide resolved
actpool/actpool.go Outdated Show resolved Hide resolved
@@ -412,6 +458,21 @@ func (ap *actPool) validate(ctx context.Context, selp *action.SealedEnvelope) er
return nil
}

func (ap *actPool) removeReplacedActs(acts []*action.SealedEnvelope) {
Copy link
Member

Choose a reason for hiding this comment

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

this is same as removeInvalidActs, see comment below?

Copy link
Member Author

Choose a reason for hiding this comment

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

as discussed, will delete the removeReplacedActs

actpool/actpool.go Outdated Show resolved Hide resolved
actpool/actpool.go Outdated Show resolved Hide resolved
@@ -153,6 +158,33 @@ func NewActPool(g genesis.Genesis, sf protocol.StateReader, cfg Config, opts ...
return ap, nil
}

func (ap *actPool) Start(ctx context.Context) error {
// open action store and load all actions
if ap.store != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

early return

Copy link

sonarcloud bot commented Sep 18, 2024

@envestcc envestcc merged commit 04b5f4f into iotexproject:master Sep 18, 2024
3 checks passed
@envestcc envestcc deleted the actpool_cache branch September 18, 2024 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants