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

fix: segfault on node restart #3736

Merged
merged 3 commits into from
Feb 2, 2024
Merged

Conversation

jimjbrettj
Copy link
Contributor

@jimjbrettj jimjbrettj commented Feb 2, 2024

Changes

  • We were running into segfaults when trying to restart the gossamer node, this PR resolves this problem
  • The issue was when calling pebbles Get() function, we were calling Close() and then copying the got value. However, the slice is only valid until we call close so we needed to reverse this order.
  • See: https://pkg.go.dev/github.com/cockroachdb/pebble@v1.0.0#DB.Get

Tests

Start a gossamer node, stop it, then restart it

  • ./bin/gossamer init --chain paseo
  • ./bin/gossamer --chain paseo
  • stop node
  • ./bin/gossamer --chain paseo

Make sure there are no segfaults. Please stress-test whatever start/stop cases you can think of :)

Issues

#3709

Primary Reviewer

@EclesioMeloJunior

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (12234de) 50.50% compared to head (f7b8767) 50.19%.

Additional details and impacted files
@@               Coverage Diff               @@
##           development    #3736      +/-   ##
===============================================
- Coverage        50.50%   50.19%   -0.32%     
===============================================
  Files              230      230              
  Lines            29005    29006       +1     
===============================================
- Hits             14650    14560      -90     
- Misses           12857    12947      +90     
- Partials          1498     1499       +1     

Copy link
Member

@EclesioMeloJunior EclesioMeloJunior left a comment

Choose a reason for hiding this comment

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

Really great finding!

@jimjbrettj jimjbrettj merged commit d1ca7aa into development Feb 2, 2024
24 checks passed
@jimjbrettj jimjbrettj deleted the jimmy/fixStartupSegfault branch February 2, 2024 21:21
github-actions bot pushed a commit that referenced this pull request Mar 1, 2024
# [0.9.0](v0.8.0...v0.9.0) (2024-3-1)

### Bug Fixes

* add a limit of number of bytes while scale decoding a slice ([#3733](#3733)) ([5edbf89](5edbf89))
* **docs:** Fixing link to polkadot runtime fundamentals to the right one ([#3763](#3763)) ([a785d32](a785d32))
* don't panic if we fail to convert hex to bytes ([#3734](#3734)) ([12234de](12234de))
* **dot/sync:** execute p2p handshake when there is no target ([#3695](#3695)) ([a9db0ec](a9db0ec))
* fix index out of range undeterministic error in rpc test ([#3718](#3718)) ([d099384](d099384))
* fix non deterministic  panic during TestStableNetworkRPC integration test ([#3756](#3756)) ([ee3d243](ee3d243))
* **lib/trie:** use `MustBeHashed` for V1 trie nodes with larger storage values ([#3739](#3739)) ([f5e48a9](f5e48a9))
* **mocks:** Set fixed version for uber mockgen in CI ([#3656](#3656)) ([ea9877e](ea9877e))
* **runtime/storage:** support nested storage transactions ([#3670](#3670)) ([3e99f6d](3e99f6d))
* segfault on node restart ([#3736](#3736)) ([d1ca7aa](d1ca7aa))
* **state-version:** should be uint8 instead of uint32 ([#3779](#3779)) ([c8fdb14](c8fdb14))
* update paseo chain spec ([#3770](#3770)) ([6a54f28](6a54f28))
* use last finalized block on startup ([#3737](#3737)) ([c262642](c262642))

### Features

* **config:** dynamically set version based on environment ([#3693](#3693)) ([5c534c9](5c534c9))
* **staging:** Expose RPC on Westend Staging Node ([#3687](#3687)) ([c374eaa](c374eaa))
* **tests/scripts:** create script to retrieve trie state via rpc ([#3714](#3714)) ([5ccea40](5ccea40))
Copy link

github-actions bot commented Mar 1, 2024

🎉 This PR is included in version 0.9.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

timwu20 pushed a commit that referenced this pull request Apr 18, 2024
timwu20 pushed a commit that referenced this pull request Apr 19, 2024
timwu20 pushed a commit that referenced this pull request Apr 19, 2024
# [0.9.0](v0.8.0...v0.9.0) (2024-3-1)

### Bug Fixes

* add a limit of number of bytes while scale decoding a slice ([#3733](#3733)) ([5edbf89](5edbf89))
* **docs:** Fixing link to polkadot runtime fundamentals to the right one ([#3763](#3763)) ([a785d32](a785d32))
* don't panic if we fail to convert hex to bytes ([#3734](#3734)) ([12234de](12234de))
* **dot/sync:** execute p2p handshake when there is no target ([#3695](#3695)) ([a9db0ec](a9db0ec))
* fix index out of range undeterministic error in rpc test ([#3718](#3718)) ([d099384](d099384))
* fix non deterministic  panic during TestStableNetworkRPC integration test ([#3756](#3756)) ([ee3d243](ee3d243))
* **lib/trie:** use `MustBeHashed` for V1 trie nodes with larger storage values ([#3739](#3739)) ([f5e48a9](f5e48a9))
* **mocks:** Set fixed version for uber mockgen in CI ([#3656](#3656)) ([ea9877e](ea9877e))
* **runtime/storage:** support nested storage transactions ([#3670](#3670)) ([3e99f6d](3e99f6d))
* segfault on node restart ([#3736](#3736)) ([d1ca7aa](d1ca7aa))
* **state-version:** should be uint8 instead of uint32 ([#3779](#3779)) ([c8fdb14](c8fdb14))
* update paseo chain spec ([#3770](#3770)) ([6a54f28](6a54f28))
* use last finalized block on startup ([#3737](#3737)) ([c262642](c262642))

### Features

* **config:** dynamically set version based on environment ([#3693](#3693)) ([5c534c9](5c534c9))
* **staging:** Expose RPC on Westend Staging Node ([#3687](#3687)) ([c374eaa](c374eaa))
* **tests/scripts:** create script to retrieve trie state via rpc ([#3714](#3714)) ([5ccea40](5ccea40))
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.

3 participants