[Merged by Bors] - feat: hare preround proposal compaction

[acud]

Motivation

Current hare prerounds use 20-byte blake3 hashes of the proposals. This PR aims to reduce the message size by adopting the changes described in #5606 with further discussions in the research forum

Description

Adopts the changes by adding a field onto the Value type which encodes the shorter IDs (the first few bytes of the VRF signature for the block proposal. This field is to be used only on the preround.

Closes #5606
Supersedes #6060
Ref #5256 #4765

Test Plan

• unit tests
• running it on a devnet that would fork to choose the new protocol version at a given epoch

TODO

• unit tests
• change to 3 or 4 byte encoding to prevent collisions
• error handling from reconstructProposals could probably be improved to just fallback into a full proposals list exchange instead of returning the error
• add cmd configuration (for switchover epoch)
• Explain motivation or link existing issue(s)
• Test changes and document test plan
• do we need to increase the preround length?
• handle proposal with empty eligibility array
• can we raise the committee size again?
• Update documentation as needed
• Update changelog as needed

[codecov]

Codecov Report

Attention: Patch coverage is 82.13783% with 254 lines in your changes missing coverage. Please review.

Project coverage is 81.9%. Comparing base (13dfb49) to head (59fe1b1).
Report is 6 commits behind head on develop.

Files	Patch %	Lines
hare4/hare.go	80.4%	76 Missing and 29 partials ⚠️
hare4/eligibility/oracle.go	76.0%	56 Missing and 14 partials ⚠️
node/node.go	48.6%	36 Missing and 2 partials ⚠️
hare4/compat/weakcoin.go	0.0%	14 Missing ⚠️
hare4/malfeasance.go	80.4%	6 Missing and 2 partials ⚠️
hare4/legacy_oracle.go	76.6%	4 Missing and 3 partials ⚠️
hare4/tracer.go	0.0%	7 Missing ⚠️
hare4/types.go	94.0%	3 Missing and 1 partial ⚠️
p2p/server/server.go	50.0%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #6129     +/-   ##
=========================================
  Coverage     81.9%   81.9%             
=========================================
  Files          301     308      +7     
  Lines        32406   33807   +1401     
=========================================
+ Hits         26548   27706   +1158     
- Misses        4135    4327    +192     
- Partials      1723    1774     +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ivan4th]

As above, can't we just encode resp into the stream instead of using an intermediate buffer?

[acud]

To do this one must know to which length the type is going to encode into without really encoding it (since you must encode how many bytes are in the response before actually writing them into the response). While it is possible, it's probably something that the scale package should offer instead of writing it by hand which would be fragile. Any ideas on how to do this?

[poszu]

Why do you need to encode the length first?

[acud]

Because you need to know how many bytes to read out of the stream.

[poszu]

AFAIR, the scale decoding would fail if the response had the wrong length anyway.

[ivan4th]

well, while we could do smth like this in go-scale, it is not really fragile if the data are an array of fixed-size IDs.
pls check how it's done in the fetch package, e.g. epoch stream handler

go-spacemesh/fetch/handler.go

Line 115 in 13dfb49

if err := h.streamIDs(ctx, s, func(cbk retrieveCallback) error {

go-spacemesh/fetch/handler.go

Lines 140 to 185 in 13dfb49

	func (h *handler) streamIDs(ctx context.Context, s io.ReadWriter, retrieve retrieveFunc) error {
	started := false
	if err := retrieve(func(total int, id []byte) error {
	if !started {
	started = true
	respSize := scale.LenSize(uint32(total)) + uint32(total*len(id))
	if _, err := codec.EncodeLen(s, respSize); err != nil {
	return err
	}
	if _, err := codec.EncodeLen(s, uint32(total)); err != nil {
	return err
	}
	}
	if _, err := s.Write(id[:]); err != nil {
	return err
	}
	return nil
	},
	); err != nil {
	if !started {
	if err := server.WriteErrorResponse(s, err); err != nil {
	h.logger.Debug("failed to write error response", log.ZContext(ctx), zap.Error(err))
	}
	}
	return err
	}
	// If any IDs were sent:
	// Response.Data already sent
	// Response.Error has length 0
	lens := []uint32{0}
	if !started {
	// If no ATX IDs were sent:
	// Response.Data is just a single zero byte (length 0),
	// but the length of Response.Data is 1 so we must send it
	// Response.Error has length 0
	lens = []uint32{1, 0, 0}
	}
	for _, l := range lens {
	if _, err := codec.EncodeLen(s, l); err != nil {
	return err
	}
	}
	return nil
	}

and the client part

go-spacemesh/fetch/mesh_data.go

Line 275 in 13dfb49

return readIDSlice(s, &ed.AtxIDs, maxEpochDataAtxIDs)

go-spacemesh/fetch/mesh_data.go

Lines 461 to 481 in 13dfb49

	func readIDSlice[V any, H scale.DecodablePtr[V]](r io.Reader, slice *[]V, limit uint32) (int, error) {
	return server.ReadResponse(r, func(respLen uint32) (int, error) {
	d := scale.NewDecoder(r)
	length, total, err := scale.DecodeLen(d, limit)
	if err != nil {
	return total, err
	}
	if int(length*types.Hash32Length)+total != int(respLen) {
	return total, errors.New("bad slice length")
	}
	*slice = make([]V, length)
	for i := uint32(0); i < length; i++ {
	n, err := H(&(*slice)[i]).DecodeScale(d)
	total += n
	if err != nil {
	return total, err
	}
	}
	return total, err
	})
	}

The client part is not perfect b/c we could be handling each ID right away instead of waiting for the whole slice to be sent, but that would require further refactoring of the fetcher code; not sure whether it is applicable here.
Nevertheless, the server side can be improved here I think

Maybe we could move some helpers to the codec package

[acud]

AFAIR, the scale decoding would fail if the response had the wrong length anyway.

So the reason it is done this way is the following:
When you read a response from a peer you want to avoid a situation where you read a stream until an EOF - this is generally bad because you put yourself in the risk of having a malicious peer to just feed you data which is read all into memory, resulting in the node memory usage going through the roof.
That's why you want to know how long is the data in advance.

[acud]

@ivan4th the server.ReadResponse part implicitly reads the respLen which encodes the whole message size in advance https://github.com/spacemeshos/go-spacemesh/blob/develop/p2p/server/server.go#L511

Which is exactly what I'm doing here, except I don't want to use the leaky server abstraction because it does a bunch of things I'm not interested in.

[poszu]

How about creating proposals with invalid signatures instead of mocking the verifier?

If mocking is easier/better, consider rewriting this as:

	for _, n := range cluster.nodes {
		gomock.InOrder(
			n.mverifier.EXPECT().
				Verify(signing.PROPOSAL, gomock.Any(), gomock.Any(), gomock.Any()).
				Return(false).
				MaxTimes(1),
			n.mverifier.EXPECT().
				Verify(signing.PROPOSAL, gomock.Any(), gomock.Any(), gomock.Any()).
				Return(true).
				AnyTimes(),
		)
	}

[acud]

This is a bit more complicated because the mocked Verify is not because the proposal signatures are incorrect, but because the node which receives the proposal matches the wrong proposal because it has some collision locally, and then cannot verify the signature correctly.

[poszu]

OK, I understand. Why not generate colliding proposals in a way that would trigger the desired flow? I generally prefer to simulate real flow (by creating realistic conditions that trigger the tested behavior) rather than hacking the insides of the implementation. Such tests are easier to understand and maintain.

[acud]

Because those things require mining of the actual data, building external tools, the tests would start breaking in case we change small details inside of the actual domain object. Pluggable implementations are standard practice and personally I'd rather go down that way.

[acud]

bors try

[spacemesh-bors]

try

Build failed:

• systest-status

[acud]

bors try

[spacemesh-bors]

try

Build succeeded:

• ci-status
• systest-status

[acud]

@poszu can I have another review please?

[poszu]

How about just overwriting the first bytes?

Suggested change

	if !cl.collidingProposals {
	// if we want non-colliding proposals we copy from the rng
	// otherwise it is kept as an array of zeroes
	cl.t.rng.Read(vrf[:])
	}
	cl.t.rng.Read(vrf[:])
	if cl.collidingProposals {
	copy(vrf[:4], []byte("1234"))
	}

[poszu]

AFAIR, the scale decoding would fail if the response had the wrong length anyway.

[acud]

bors try

[spacemesh-bors]

try

Build failed:

• systest-status

[acud]

bors merge

[spacemesh-bors]

Pull request successfully merged into develop.

Build succeeded:

• ci-status
• systest-status