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 storage pruning node type selection #4947

Merged
merged 7 commits into from Feb 22, 2023

Conversation

sstanculeanu
Copy link
Contributor

@sstanculeanu sstanculeanu commented Feb 1, 2023

Reasoning behind the pull request

  • when creating the old data cleaner provider, it used to use the observer config value for nodes running in multikey mode

Proposed changes

  • added IsMultiKeyMode method to ManagedPeersHolder interface
  • added few more unittests

Testing procedure

  • will be tested with feat branch, old data should be cleaned on multikey nodes

Pre-requisites

Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

added IsMultiKey method to managedPeersHolder interface
added few extra tests
@sstanculeanu sstanculeanu added type:bug Something isn't working low prio not critical - to be solved with low prio interested:protocol complexity:medium area:multikey labels Feb 1, 2023
@sstanculeanu sstanculeanu self-assigned this Feb 1, 2023
handler.UpdatePublicKeyLiveness(randomPublicKeyBytes, pid)
assert.Zero(t, len(mapResetCalled))
})
t.Run("another pid should call reset", func(t *testing.T) {
randomPid := core.PeerID("random pid")
t.Run("another Pid() should call reset", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

pid instead of Pid()? Also on L271

@@ -1,17 +1,29 @@
package keysManagement

import (
"time"

"github.com/multiversx/mx-chain-core-go/core"
crypto "github.com/multiversx/mx-chain-crypto-go"
)

// ManagedPeersHolder defines the operations of an entity that holds managed identities for a node
type ManagedPeersHolder interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

so now we have the interface in the same package as the implementation. Because we need it in several packages.

@@ -15,7 +15,7 @@ import (
// keep this test in a separate package as to not be influenced by other the tests from the same package
func TestFeeComputer_MemoryFootprint(t *testing.T) {
numEpochs := 10000
maxFootprintNumBytes := 20_000_000
maxFootprintNumBytes := 25_000_000
Copy link
Contributor

Choose a reason for hiding this comment

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

this indicates something is off with the memory foot print. Possible resources leak?

@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2023

Codecov Report

Base: 70.98% // Head: 70.86% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (ec619db) compared to base (822595c).
Patch coverage: 84.21% of modified lines in pull request are covered.

❗ Current head ec619db differs from pull request most recent head 65d6096. Consider uploading reports for the commit 65d6096 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@                Coverage Diff                @@
##           feat/multikey    #4947      +/-   ##
=================================================
- Coverage          70.98%   70.86%   -0.12%     
=================================================
  Files                651      675      +24     
  Lines              85403    87459    +2056     
=================================================
+ Hits               60621    61981    +1360     
- Misses             20229    20820     +591     
- Partials            4553     4658     +105     
Impacted Files Coverage Δ
api/gin/common.go 30.95% <ø> (ø)
api/gin/httpServer.go 23.80% <ø> (ø)
api/gin/webServer.go 0.00% <ø> (ø)
api/gin/writers.go 0.00% <ø> (ø)
api/groups/addressGroup.go 72.02% <ø> (ø)
api/groups/addressGroupOptions.go 84.61% <ø> (ø)
api/groups/baseGroup.go 79.48% <ø> (ø)
api/groups/blockGroup.go 74.67% <ø> (ø)
api/groups/hardforkGroup.go 81.25% <ø> (ø)
api/groups/networkGroup.go 95.48% <ø> (ø)
... and 220 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -339,6 +339,11 @@ func (holder *managedPeersHolder) SetNextPeerAuthenticationTime(pkBytes []byte,
pInfo.setNextPeerAuthenticationTime(nextTime)
}

// IsMultiKeyMode returns true if the key has at least one managed key
Copy link
Contributor

Choose a reason for hiding this comment

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

if the node* ?

@sstanculeanu sstanculeanu merged commit a46893c into feat/multikey Feb 22, 2023
@sstanculeanu sstanculeanu deleted the fix_storage_pruning_node_type_selection branch February 22, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:multikey complexity:medium interested:protocol low prio not critical - to be solved with low prio type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants