[Merged by Bors] - sync: new protocol to fetch layer hash and certificate #4846

countvonzero · 2023-08-14T20:02:21Z

Motivation

Changes

when a node poll layer opinions from peers, it gets back a response from each peer.

type LayerOpinion struct {
	PrevAggHash types.Hash32
	Certified *types.BlockID
}

node then go through each response and collect unique BlockIDs and fetch certificate for those blocks.

before and after

in TestAddNodes, total amount of data a new node downloaded at layer 39 reduced from 10_647_927 bytes to 208_435 bytes

before

after

testing

systests TestAddNodes starts 28 nodes with new protocol and later added 2 nodes to sync with old protocol
manually sync with mainnet with new protocol enabled

note on deployment

new nodes will fetch opinions with new protocol from nodes that support it. and with old protocol from nodes that don't support it.
in the case of rollback, set syncer config UseNewProtocol to false

		Sync: syncer.Config{
                        ...
			UseNewProtocol:   false,
		},

all nodes will then only request/serve the old protocol

countvonzero · 2023-08-14T20:09:07Z

bors try

codecov · 2023-08-14T20:19:06Z

Codecov Report

Merging #4846 (01a2145) into develop (bfa734e) will decrease coverage by 0.1%.
The diff coverage is 71.5%.

@@            Coverage Diff            @@
##           develop   #4846     +/-   ##
=========================================
- Coverage     76.9%   76.9%   -0.1%     
=========================================
  Files          261     261             
  Lines        29807   30103    +296     
=========================================
+ Hits         22951   23166    +215     
- Misses        5392    5453     +61     
- Partials      1464    1484     +20

Files Changed	Coverage Δ
fetch/interface.go	`100.0% <ø> (ø)`
fetch/metrics.go	`100.0% <ø> (ø)`
p2p/upgrade.go	`64.2% <0.0%> (-1.0%)`	⬇️
fetch/wire_types.go	`63.6% <33.3%> (-8.5%)`	⬇️
fetch/handler.go	`48.4% <53.6%> (+4.1%)`	⬆️
fetch/mesh_data.go	`64.6% <63.4%> (-1.1%)`	⬇️
fetch/fetch.go	`77.8% <71.4%> (-0.3%)`	⬇️
sql/certificates/certs.go	`76.5% <78.5%> (+0.2%)`	⬆️
syncer/state_syncer.go	`76.1% <81.2%> (-2.2%)`	⬇️
syncer/data_fetch.go	`77.0% <84.5%> (+2.9%)`	⬆️
... and 2 more

... and 3 files with indirect coverage changes

bors · 2023-08-14T20:55:18Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

config/mainnet.go

dshulyak · 2023-08-16T06:48:22Z

in the case of rollback, simply set --sync-update-layer to a big number via config.

could you please add how json config changes in pr description. tbh such options should not be even added to command line as it will be harder to drop them later. but i assume this needed for tests

dshulyak

i would consider doing upgrade without configured layer, it seems much more convenient for testing on mainnet in this case.

i think in system like this peers are expected to not respond or disconnect randomly. printing warnings doesn't make much sense, i would replace that with prometheus counter as it is mainly useful for operations

fetch/wire_types.go

dshulyak · 2023-08-16T07:10:46Z

syncer/state_syncer.go

+}
+
+func (s *Syncer) fetchOpinions(ctx context.Context, lid types.LayerID) ([]*peerOpinion, []*types.Certificate, error) {
+	if s.ticker.CurrentLayer() >= types.LayerID(s.cfg.UpdateLayer) {


i think that upgrade path without configured layer is more convenient and robust. if there is a regression we will be able to notice it immediately

i am referring to a upgrade path, where node checks if it has any peers with new protocol and if so then uses new

used PeerStore().GetProtocols() to make upgrade path automatic.
peer will first fetch from v2 peers, and then fetch from peers that don't support v2
as this is not on critical path, i did this sequentially instead of concurrently for simpler code.

syncer/state_syncer.go

dshulyak · 2023-08-16T07:23:33Z

fetch/mesh_data.go

+			var peerCert types.Certificate
+			err = codec.Decode(data, &peerCert)
+			if err != nil {
+				defer close(done)


consumer reads exactly one event from done and then continues, same for output. doesn't look good with this defers

thanks! fixed

fetch/mesh_data.go

dshulyak · 2023-08-16T07:32:55Z

fetch/mesh_data.go

+				done <- err
+				return
+			}
+			if peerCert.BlockID != bid {


this check also somewhat inconsistent now, other messages check it in handlers

this is true for generic hash fetches. (atx/block/proposal/ballot/tx).
however, certificate doesn't go through that path. it's specifically requested, as block certificate doesn't have an ID associated with it. keeping it and added comment.

syncer/data_fetch.go

countvonzero · 2023-08-18T00:47:39Z

bors try

bors · 2023-08-18T01:33:34Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

countvonzero · 2023-08-18T08:44:09Z

bors try

bors · 2023-08-18T09:32:44Z

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

countvonzero · 2023-08-18T09:34:33Z

bors merge

## Motivation Closes #4674 ## Changes when a node poll layer opinions from peers, it gets back a response from each peer. ```go type LayerOpinion struct { PrevAggHash types.Hash32 Certified *types.BlockID } ``` node then go through each response and collect unique BlockIDs and fetch certificate for those blocks. ## before and after in TestAddNodes, total amount of data a new node downloaded at layer 39 reduced from 10_647_927 bytes to 208_435 bytes before ![Screenshot from 2023-08-18 09-45-36](https://github.com/spacemeshos/go-spacemesh/assets/30611210/92eb3c9a-ce2e-4aee-994a-56504600427d) after ![Screenshot from 2023-08-18 09-48-18](https://github.com/spacemeshos/go-spacemesh/assets/30611210/e59ebab6-8d38-472e-b4c3-7dbcbc174f77) ## testing - systests TestAddNodes starts 28 nodes with new protocol and later added 2 nodes to sync with old protocol - manually sync with mainnet with new protocol enabled ## note on deployment - new nodes will fetch opinions with new protocol from nodes that support it. and with old protocol from nodes that don't support it. - in the case of rollback, set syncer config `UseNewProtocol` to false ``` Sync: syncer.Config{ ... UseNewProtocol: false, }, ``` all nodes will then only request/serve the old protocol

bors · 2023-08-18T10:24:31Z

Pull request successfully merged into develop.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

ci-status
systest-status

countvonzero added 3 commits August 14, 2023 21:58

decouple function args from fetch API

f17facb

new protocol to poll opinions from peers

51784bf

add systest

ede4ceb

countvonzero requested review from dshulyak, fasmat and poszu as code owners August 14, 2023 20:02

bors bot added a commit that referenced this pull request Aug 14, 2023

Try #4846:

3ccb3ef

countvonzero commented Aug 15, 2023

View reviewed changes

config/mainnet.go Outdated Show resolved Hide resolved

dshulyak approved these changes Aug 16, 2023

View reviewed changes

countvonzero added 4 commits August 17, 2023 16:33

review feedback

85a3033

use same protocol for opinions and certs

c4480f4

make upgrade path automatic

d581e1d

Merge branch 'develop' into less-opinionated-sync

6adca45

bors bot added a commit that referenced this pull request Aug 18, 2023

Try #4846:

1f19dff

separate req and serve with new opns protocol

01a2145

bors bot added a commit that referenced this pull request Aug 18, 2023

Try #4846:

ba024d5

bors bot changed the title ~~sync: new protocol to fetch layer hash and certificate~~ [Merged by Bors] - sync: new protocol to fetch layer hash and certificate Aug 18, 2023

bors bot closed this Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - sync: new protocol to fetch layer hash and certificate #4846

[Merged by Bors] - sync: new protocol to fetch layer hash and certificate #4846

countvonzero commented Aug 14, 2023 •

edited

countvonzero commented Aug 14, 2023

codecov bot commented Aug 14, 2023 •

edited

bors bot commented Aug 14, 2023

dshulyak commented Aug 16, 2023

dshulyak left a comment

dshulyak Aug 16, 2023

countvonzero Aug 17, 2023

dshulyak Aug 16, 2023

countvonzero Aug 17, 2023

dshulyak Aug 16, 2023

countvonzero Aug 17, 2023

countvonzero commented Aug 18, 2023

bors bot commented Aug 18, 2023

countvonzero commented Aug 18, 2023

bors bot commented Aug 18, 2023

countvonzero commented Aug 18, 2023

bors bot commented Aug 18, 2023

[Merged by Bors] - sync: new protocol to fetch layer hash and certificate #4846

[Merged by Bors] - sync: new protocol to fetch layer hash and certificate #4846

Conversation

countvonzero commented Aug 14, 2023 • edited

Motivation

Changes

before and after

testing

note on deployment

countvonzero commented Aug 14, 2023

codecov bot commented Aug 14, 2023 • edited

Codecov Report

bors bot commented Aug 14, 2023

try

dshulyak commented Aug 16, 2023

dshulyak left a comment

Choose a reason for hiding this comment

dshulyak Aug 16, 2023

Choose a reason for hiding this comment

countvonzero Aug 17, 2023

Choose a reason for hiding this comment

dshulyak Aug 16, 2023

Choose a reason for hiding this comment

countvonzero Aug 17, 2023

Choose a reason for hiding this comment

dshulyak Aug 16, 2023

Choose a reason for hiding this comment

countvonzero Aug 17, 2023

Choose a reason for hiding this comment

countvonzero commented Aug 18, 2023

bors bot commented Aug 18, 2023

try

countvonzero commented Aug 18, 2023

bors bot commented Aug 18, 2023

try

countvonzero commented Aug 18, 2023

bors bot commented Aug 18, 2023

countvonzero commented Aug 14, 2023 •

edited

codecov bot commented Aug 14, 2023 •

edited