Skip to content

state: save in batches within the state store#6067

Merged
cmwaters merged 7 commits intotendermint:masterfrom
githubsands:store.go
Feb 23, 2021
Merged

state: save in batches within the state store#6067
cmwaters merged 7 commits intotendermint:masterfrom
githubsands:store.go

Conversation

@githubsands
Copy link

@githubsands githubsands commented Feb 8, 2021

New issue arised from: #6018 (review)

Within state.store.go:

  • ABCI data
  • Consensus rules
  • Validators

are now saved using batches.

@codecov
Copy link

codecov bot commented Feb 8, 2021

Codecov Report

Merging #6067 (fedc260) into master (9498cd8) will decrease coverage by 0.10%.
The diff coverage is 56.52%.

@@            Coverage Diff             @@
##           master    #6067      +/-   ##
==========================================
- Coverage   60.74%   60.64%   -0.11%     
==========================================
  Files         276      276              
  Lines       25709    25715       +6     
==========================================
- Hits        15618    15594      -24     
- Misses       8476     8501      +25     
- Partials     1615     1620       +5     
Impacted Files Coverage Δ
state/store.go 57.50% <56.52%> (+0.95%) ⬆️
crypto/sr25519/pubkey.go 43.47% <0.00%> (-8.70%) ⬇️
libs/events/events.go 92.94% <0.00%> (-5.89%) ⬇️
p2p/switch.go 58.04% <0.00%> (-5.37%) ⬇️
privval/signer_server.go 89.47% <0.00%> (-5.27%) ⬇️
consensus/reactor.go 67.71% <0.00%> (-1.89%) ⬇️
p2p/transport_memory.go 84.93% <0.00%> (-1.37%) ⬇️
p2p/transport_mconn.go 82.67% <0.00%> (-1.00%) ⬇️
consensus/state.go 66.23% <0.00%> (-0.28%) ⬇️
blockchain/v0/pool.go 76.42% <0.00%> (ø)
... and 7 more

@githubsands githubsands changed the title [WIP] Changes needed for state.go [WIP] state: short term changes to use batches within state.store.go Feb 8, 2021
@lgtm-com
Copy link

lgtm-com bot commented Feb 8, 2021

This pull request introduces 3 alerts when merging b88bd2d into 38c5d28 - view on LGTM.com

new alerts:

  • 1 for Impossible interface nil check
  • 1 for Useless assignment to local variable
  • 1 for Expression has no effect

 * saving abciresponses, validators, and consensus rules for now
@githubsands
Copy link
Author

githubsands commented Feb 8, 2021

@cmwaters anyway to run all these tests locally? Specifically those like tests_apps

@lgtm-com
Copy link

lgtm-com bot commented Feb 8, 2021

This pull request introduces 1 alert when merging 1f486f5 into 38c5d28 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@cmwaters
Copy link
Contributor

cmwaters commented Feb 8, 2021

Yup.

The unit tests are pretty easy just go test ./...

The test_apps one you need to run make install install_abci and then run bash test/app/test.sh. You will find all the similar tests in the test/app directory

For the e2e tests you need to cd test/e2e and run make docker && make runner. This may take a bit to build. After that run ./build/runner -f networks/ci.toml to run the ci e2e tests

@githubsands
Copy link
Author

@cmwaters thanks mate

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale for use by stalebot label Feb 20, 2021
@cmwaters cmwaters removed the stale for use by stalebot label Feb 22, 2021
@cmwaters
Copy link
Contributor

Hey @githubsands, I see this PR hasn't been touched in a while. I hope you don't mind me taking it over so that we can get it merged. It seems that it's almost finished in any case. Thanks for the contribution :)

@cmwaters cmwaters self-assigned this Feb 22, 2021
@cmwaters cmwaters changed the title [WIP] state: short term changes to use batches within state.store.go state: save in batches within the state store Feb 23, 2021
state/store.go Outdated
err := store.db.SetSync(key, state.Bytes())
if err != nil {

if err := batch.Set(stateKey, state.Bytes()); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

why stateKey instead of key? Was this a bug?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we should probably using key to keep consistency. They are the same. It's only because it was accidentally removed in the first commit of this PR and so I re-added it afterwards that it is different

@cmwaters cmwaters merged commit d5cf783 into tendermint:master Feb 23, 2021
@githubsands
Copy link
Author

@cmwaters thanks was waiting for a new computer to come in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants