chore(lib/grandpa): make interval configurable #1903

zhuber6 · 2021-10-16T03:23:13Z

Changes

Updated all files related to reading toml config and default config to handle GrandpaInterval.
Updated GRANDPA to use configurable interval via config file.

Tests

go test -short ./lib/grandpa
go test ./cmd/gossamer

Issues

Fixes #1869

Primary Reviewer

@noot

CLAassistant · 2021-10-16T03:23:16Z

All committers have signed the CLA.

arijitAD · 2021-10-18T12:23:51Z

chain/gssmr/defaults.go

@@ -85,6 +85,9 @@ var (
 	// DefaultDiscoveryInterval is the default interval for searching for DHT peers
 	DefaultDiscoveryInterval = time.Second * 10

+	// DefaultGrandpaInterval


// DefaultGrandpaInterval ...

yeah I'd mention that this is the time for a grandpa sub-round

Added this in there. Still working on understanding all of the polkadot pieces...

arijitAD · 2021-10-18T12:26:46Z

cmd/gossamer/config_test.go

@@ -390,6 +390,7 @@ func TestNetworkConfigFromFlags(t *testing.T) {
 				NoBootstrap:       testCfg.Network.NoBootstrap,
 				NoMDNS:            testCfg.Network.NoMDNS,
 				DiscoveryInterval: time.Second * 10,
+				GrandpaInterval:   time.Second,


Can this value be replaced with testCfg.Network.GrandpaInterval ?
If so then do it at other places.

Yes I have updated this.

noot · 2021-10-18T14:30:00Z

chain/gssmr/config.toml

@@ -30,6 +30,7 @@ port = 7001
 nobootstrap = false
 nomdns = false
 discovery-interval = 10
+grandpa-interval = 1


could you put this under core instead of network, as it's consensus related?

noot · 2021-10-18T14:30:10Z

cmd/gossamer/config.go

+	if tomlCfg.GrandpaInterval > 0 {
+		cfg.GrandpaInterval = time.Second * time.Duration(tomlCfg.GrandpaInterval)
+	} else {
+		cfg.GrandpaInterval = 0
+	}


same here, please move to core config

noot · 2021-10-18T14:30:29Z

cmd/gossamer/config.go

@@ -631,6 +631,12 @@ func setDotNetworkConfig(ctx *cli.Context, tomlCfg ctoml.NetworkConfig, cfg *dot
 		cfg.DiscoveryInterval = 0


this should be set to the default value if it's not set

noot · 2021-10-18T14:31:02Z

lib/grandpa/grandpa.go

@@ -166,6 +167,7 @@ func NewService(cfg *Config) (*Service, error) {
 		resumed:            make(chan struct{}),
 		network:            cfg.Network,
 		finalisedCh:        finalisedCh,
+		interval:           cfg.Interval,


can you add a check in this function that will error if cfg.Interval == 0?

noot

thank you for the contribution! overall looks good, just a few suggestions.

chain/gssmr/config.toml

qdm12 · 2021-10-18T18:36:49Z

cmd/gossamer/config.go

+	if tomlCfg.GrandpaInterval > 0 {
+		cfg.GrandpaInterval = time.Second * time.Duration(tomlCfg.GrandpaInterval)
+	} else {
+		cfg.GrandpaInterval = 0
+	}


Unless tomlCfg.GrandpaInterval can be negative, I think we can simplify to:

Suggested change

if tomlCfg.GrandpaInterval > 0 {

cfg.GrandpaInterval = time.Second * time.Duration(tomlCfg.GrandpaInterval)

} else {

cfg.GrandpaInterval = 0

}

cfg.GrandpaInterval = time.Second * time.Duration(tomlCfg.GrandpaInterval)

lib/grandpa/grandpa.go

cmd/gossamer/config.go

qdm12 · 2021-10-21T13:45:00Z

dot/config/toml/config.go

@@ -83,6 +82,7 @@ type CoreConfig struct {
 	SlotDuration     uint64 `toml:"slot-duration,omitempty"`
 	EpochLength      uint64 `toml:"epoch-length,omitempty"`
 	WasmInterpreter  string `toml:"wasm-interpreter,omitempty"`
+	GrandpaInterval  uint32 `toml:"grandpa-interval,omitempty"`


Any reason we have it as uint32? I guess that's fine given it's 1, I'm just wondering 🤔

I am assuming that it cannot be negative so I can just store it as that instead of an int.

EclesioMeloJunior

lgtm

noot · 2021-10-25T09:35:06Z

@zjhuber could you fix the merge conflicts when possible? can merge this right after :)

zhuber6 · 2021-10-25T16:38:25Z

@noot just resolved conflicts, this should be good to merge. Apologies for the delay.

codecov · 2021-10-25T17:08:46Z

Codecov Report

Merging #1903 (0e72a6f) into development (cd27ae4) will decrease coverage by 0.02%.
The diff coverage is 84.61%.

@@               Coverage Diff               @@
##           development    #1903      +/-   ##
===============================================
- Coverage        60.28%   60.26%   -0.03%     
===============================================
  Files              180      180              
  Lines            18198    18203       +5     
===============================================
- Hits             10971    10970       -1     
- Misses            5415     5419       +4     
- Partials          1812     1814       +2

Flag	Coverage Δ
unit-tests	`60.26% <84.61%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/grandpa/grandpa.go	`59.64% <71.42%> (+0.19%)`	⬆️
cmd/gossamer/config.go	`73.97% <100.00%> (-0.06%)`	⬇️
cmd/gossamer/export.go	`85.07% <100.00%> (+0.22%)`	⬆️
dot/config.go	`27.36% <100.00%> (+0.38%)`	⬆️
dot/services.go	`71.54% <100.00%> (+0.11%)`	⬆️
dot/telemetry/telemetry.go	`67.21% <0.00%> (-6.56%)`	⬇️
dot/core/service.go	`52.91% <0.00%> (-0.78%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd27ae4...0e72a6f. Read the comment docs.

noot · 2021-10-25T19:50:45Z

@zjhuber no problem at all! thank you :D

noot · 2021-10-27T20:09:46Z

@zjhuber it seems that some of the integration tests are failing due to the interval being zero, could you add something that sets the interval to some defaultInterval = 1s if it's zero?

zhuber6 · 2021-10-29T18:37:53Z

My bad, I didn't run those tests and missed that. I believe I just fixed the issue in the test. I wasn't able to run the polkadotjs because of some errors I was getting, probably local config issues. But I did see the same issue you were seeing and I believe it is taken care of now.

noot · 2021-11-03T20:31:14Z

@zjhuber thanks so much! will merge this in :)

github-actions · 2021-12-03T21:06:59Z

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

* chore(lib/grandpa): make interval configurable * Resolved teams concerns * Fixed integration test Co-authored-by: noot <36753753+noot@users.noreply.github.com> Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>

chore(lib/grandpa): make interval configurable

6aeb9b6

zhuber6 requested review from arijitAD, EclesioMeloJunior, edwardmack, jimjbrettj, kishansagathiya, noot, omar391 and timwu20 as code owners October 16, 2021 03:23

arijitAD reviewed Oct 18, 2021

View reviewed changes

noot reviewed Oct 18, 2021

View reviewed changes

qdm12 reviewed Oct 18, 2021

View reviewed changes

Resolved teams concerns

740393d

qdm12 mentioned this pull request Oct 21, 2021

Feature request: use Go duration strings for time parameters instead of seconds integers #1914

Open

qdm12 approved these changes Oct 21, 2021

View reviewed changes

EclesioMeloJunior approved these changes Oct 21, 2021

View reviewed changes

Merge branch 'development' into chore/issue-1869

5fed082

Merge branch 'development' into chore/issue-1869

09408cb

Merge branch 'development' into chore/issue-1869

775514d

Fixed integration test

b73826c

qdm12 and others added 2 commits November 1, 2021 11:25

Merge branch 'development' into chore/issue-1869

6f6f085

Merge branch 'development' into chore/issue-1869

4010a66

noot added 2 commits November 3, 2021 16:54

Merge branch 'development' into chore/issue-1869

798f675

Merge branch 'development' into chore/issue-1869

0e72a6f

noot merged commit dc89892 into ChainSafe:development Nov 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(lib/grandpa): make interval configurable #1903

chore(lib/grandpa): make interval configurable #1903

zhuber6 commented Oct 16, 2021 •

edited

Loading

CLAassistant commented Oct 16, 2021 •

edited

Loading

arijitAD Oct 18, 2021

noot Oct 18, 2021

zhuber6 Oct 19, 2021

arijitAD Oct 18, 2021

zhuber6 Oct 21, 2021

noot Oct 18, 2021

noot Oct 18, 2021

noot Oct 18, 2021

noot Oct 18, 2021

noot left a comment

qdm12 Oct 18, 2021

qdm12 Oct 21, 2021

zhuber6 Oct 21, 2021

EclesioMeloJunior left a comment

noot commented Oct 25, 2021

zhuber6 commented Oct 25, 2021

codecov bot commented Oct 25, 2021 •

edited

Loading

noot commented Oct 25, 2021

noot commented Oct 27, 2021

zhuber6 commented Oct 29, 2021

noot commented Nov 3, 2021

github-actions bot commented Dec 3, 2021

		@@ -631,6 +631,12 @@ func setDotNetworkConfig(ctx cli.Context, tomlCfg ctoml.NetworkConfig, cfg dot
		cfg.DiscoveryInterval = 0

chore(lib/grandpa): make interval configurable #1903

chore(lib/grandpa): make interval configurable #1903

Conversation

zhuber6 commented Oct 16, 2021 • edited Loading

Changes

Tests

Issues

Primary Reviewer

CLAassistant commented Oct 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EclesioMeloJunior left a comment

Choose a reason for hiding this comment

noot commented Oct 25, 2021

zhuber6 commented Oct 25, 2021

codecov bot commented Oct 25, 2021 • edited Loading

Codecov Report

noot commented Oct 25, 2021

noot commented Oct 27, 2021

zhuber6 commented Oct 29, 2021

noot commented Nov 3, 2021

github-actions bot commented Dec 3, 2021

zhuber6 commented Oct 16, 2021 •

edited

Loading

CLAassistant commented Oct 16, 2021 •

edited

Loading

codecov bot commented Oct 25, 2021 •

edited

Loading