[Merged by Bors] - block: generate certificate for hare output #3449

countvonzero · 2022-08-16T06:59:06Z

Motivation

Closes #3200
Part of #3199

Changes

certificate is saved in table layers instead of blocks because it's expected for hare to output empty block id for a layer, and that certifiers can certify an empty block id for a layer.
certifier waits till it has f+1 signatures and produce a certificate and save to db.
syncer fetches the certificate to use as hare output

countvonzero · 2022-08-16T07:08:38Z

@dshulyak can you review this PR please? thank you

countvonzero · 2022-08-16T12:50:34Z

bors try

bors · 2022-08-16T13:54:22Z

try

Build succeeded:

ci-stage2

sql/migrations/0001_initial.sql

common/types/block.go

sql/layers/layers.go

hare/haretypes.go

fetch/layers.go

blocks/certifier.go

countvonzero · 2022-08-17T18:11:46Z

bors try

bors · 2022-08-17T19:08:29Z

try

Build succeeded:

ci-stage2

cmd/node/node.go

blocks/certifier.go

dshulyak · 2022-08-17T04:35:13Z

blocks/certifier.go

+		return nil
+	}
+
+	logger.WithFields(bid)


logger = logger.WithFields(bid)
it looks like it always creates a new instance

dshulyak · 2022-08-17T04:44:26Z

common/types/block.go

@@ -166,3 +166,35 @@ type BlockContextualValidity struct {
 	ID       BlockID
 	Validity bool
 }
+


please call go generate on this file. you will need scalegen binary

dshulyak · 2022-08-18T07:24:14Z

blocks/certifier.go

+
+func (c *Certifier) getCertStateLocked(logger log.Log, lid types.LayerID, bid types.BlockID) (certState, error) {
+	if ci, ok := c.certMsgs[lid]; !ok {
+		logger.Error("layer not registered for cert")


this is expected if very old message is received for any reason, at most should be a warning
also why internal?

dshulyak · 2022-08-18T07:24:49Z

blocks/certifier.go

+		logger.Error("layer not registered for cert")
+		return unknown, errInternal
+	} else if ci.bid != bid {
+		logger.Error("block not registered for cert")


some part of the committee can be malicious and will sign different block
we should collect certificates for all blocks, until one of the them has enough space units

changed code to allow for multiple blocks.

blocks/certifier.go

dshulyak · 2022-08-18T07:29:16Z

sql/layers/layers.go

+	}
+}
+
+func prune(cert *types.Certificate) {


not sure what was the idea but it doesn't prune stuff, it just zeroes it out

yikes. changed to use new struct so the field is nil slice

hmmm... this doesn't matter. the BlockID is an array and so is still zeroed out values. will revert this.

sql/migrations/0001_initial.sql

dshulyak · 2022-08-18T07:44:23Z

p2p/pubsub/pubsub.go

@@ -62,6 +62,9 @@ const (
 	// HareProtocol is the protocol id for hare messages.
 	HareProtocol = "/sm/hr/1"

+	// BlockCertify is the protocol id for block certification.
+	BlockCertify = "/sm/bc/1"


i should have paid more attention to the original change :)

we really should remove all useless stuff from this names, they will add bloat on the wire. for example /sm/ and third slash is not needed. so as long as they are unique we are good

dshulyak · 2022-08-18T07:54:45Z

blocks/certifier.go

+	return nil
+}
+
+func (c *Certifier) tryGenCert() error {


i don't understand the purpose of this method
certificate should be generatable when handler received sufficient number of messages to cross threshold

glad you caught me. it was meant to prune. but then i over did it. now it's just pruning old data.

dshulyak · 2022-08-18T13:33:13Z

blocks/certifier.go

+	if _, ok := c.certMsgs[lid]; !ok {
+		logger.Warning("layer not registered for cert")
+		return unknown, errUnexpectedMsg
+	} else if _, ok = c.certMsgs[lid][bid]; !ok {


so you won't be collecting certificates for blocks that this node didn't generate? i misunderstood it during initial review

so if for any reason (missing state/bugs) this node wasn't able to form full certificate - it will download it only from other peers

in this case it probably makes no sense to store a map with blocks

that's right. i had assume that you want me to write code such that the malicious nodes who are able to connect to all network splits can also use. i will revert back to single block assumption.

dshulyak · 2022-08-18T13:38:05Z

blocks/certifier.go

+type CertConfig struct {
+	CommitteeSize         int
+	CertifyThreshold      int
+	SignatureWaitDuration int


better to use time.Duration and avoid multiplying it by time.Second later

dshulyak · 2022-08-18T13:41:29Z

blocks/certifier.go

+
+	c.certMsgs[lid][bid] = &certInfo{
+		deadline:   now.Add(time.Duration(c.cfg.SignatureWaitDuration) * time.Second),
+		signatures: make([]types.CertifyMessage, 0, c.cfg.CertifyThreshold),


i believe this makes no difference now

not sure what you mean. do you mean it makes no difference what the capacity is?

i meant that we don't expect number of signatures to be equal to this threshold. we expect eligibility count to be equal to threshold

right. but i still need to initialize it to some capacity, and given that we don't use fraction for eligibility count, this is a safe number to use. or the committee size.

Yeah, the CertifyThreshold is the maximum it can be. (i.e. everybody's signer's weight is 1)

exactly why do you think this need work? this is the maximum capacity that can be possible. as such, no comments is required. adding unnecessary comments will only serve to confuse.

you yourself said

If it was enterprise software, I would have left it at CommitteeSize when I read it the first time.

please unblock. this is not cool. @jonZlotnik

I would have left it at CommitteeSize because it doesn't matter in enterprise software. You have your colleagues to explain things to you so I would have just ignored that it was too big an array for what it was. CommitteeSize makes it more confusing than CertifyThreshold

The maximum possible capacity that it would grow to is CertifyThreshold because if we assume every committee member has one eligibility, your implementation collects signatures until CertifyThreshold number of signatures is reached, and then builds/saves the certificate. It drops any further signatures (as it should):

go-spacemesh/blocks/certifier.go

Lines 340 to 351 in a526925

func (c *Certifier) saveMessage(logger log.Log, msg types.CertifyMessage) error {

c.mu.Lock()

defer c.mu.Unlock()

lid := msg.LayerID

bid := msg.BlockID

if state, err := c.getCertStateLocked(logger, lid, bid); err != nil {

return err

} else if state != pending { // already certified

return nil

}

If any committee member has any more than 1 eligibility, the number of signatures required to reach CertifyThreshold decreases.

I'm okay with either:
make([]types.CertifyMessage, 0, c.cfg.CertifyThreshold)
or
make([]types.CertifyMessage, 0)

my rationale is, committee size is the max size it can grow for any implementation. the current impl, the max is the threshold yes. but say we decided to wait for ALL msgs before we certify? and because it matters so little (800 vs 401), i decided to go for the future-safe one.

countvonzero · 2022-08-18T14:59:49Z

bors try

bors · 2022-08-18T16:05:22Z

try

Build failed:

ci-stage2

fetch/layers.go

blocks/certifier.go

jonZlotnik

My last comments don't affect behavior.

jonZlotnik

I don't like circles

jonZlotnik

"do what's right and leave the consequences to code"
- some sticker, 2022

countvonzero · 2022-08-19T18:00:24Z

bors try

bors · 2022-08-19T18:57:11Z

try

Build succeeded:

ci-stage2

countvonzero · 2022-08-19T19:16:23Z

bors merge

## Motivation  Closes #3200 Part of #3199 ## Changes  - certificate is saved in table `layers` instead of `blocks` because it's expected for hare to output empty block id for a layer, and that certifiers can certify an empty block id for a layer. - certifier waits till it has f+1 signatures and produce a certificate and save to db. - syncer fetches the certificate to use as hare output

bors · 2022-08-19T20:10:02Z

Pull request successfully merged into develop.

Build succeeded:

ci-stage2

countvonzero requested review from dshulyak, evt, abergasov and jonZlotnik as code owners August 16, 2022 06:59

countvonzero changed the title ~~block: generate certifiable for hare output~~ block: generate certificate for hare output Aug 16, 2022

countvonzero force-pushed the certifiable-fraud branch from e91d90c to 68cbe07 Compare August 16, 2022 12:49

bors bot added a commit that referenced this pull request Aug 16, 2022

Try #3449:

a045edc

jonZlotnik suggested changes Aug 17, 2022

View reviewed changes

countvonzero force-pushed the certifiable-fraud branch from 68cbe07 to d183eb2 Compare August 17, 2022 18:02

bors bot added a commit that referenced this pull request Aug 17, 2022

Try #3449:

64803c9

jonZlotnik reviewed Aug 17, 2022

View reviewed changes

cmd/node/node.go Show resolved Hide resolved

jonZlotnik reviewed Aug 17, 2022

View reviewed changes

blocks/certifier.go Outdated Show resolved Hide resolved

dshulyak reviewed Aug 18, 2022

View reviewed changes

dshulyak approved these changes Aug 18, 2022

View reviewed changes

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

Try #3449:

bc2d9bf

jonZlotnik reviewed Aug 18, 2022

View reviewed changes

fetch/layers.go Outdated Show resolved Hide resolved

blocks/certifier.go Outdated Show resolved Hide resolved

blocks/certifier.go Outdated Show resolved Hide resolved

blocks/certifier.go Show resolved Hide resolved

jonZlotnik approved these changes Aug 18, 2022

View reviewed changes

This was referenced Aug 18, 2022

hare: always terminates when round is advanced to certification round #2942

Closed

hare: save certificate on completion #3199

Closed

jonZlotnik linked an issue Aug 18, 2022 that may be closed by this pull request

hare: save certificate on completion #3199

Closed

3 tasks

jonZlotnik suggested changes Aug 19, 2022

View reviewed changes

jonZlotnik approved these changes Aug 19, 2022

View reviewed changes

countvonzero force-pushed the certifiable-fraud branch from f02b690 to bfb910b Compare August 19, 2022 17:53

countvonzero added 16 commits August 19, 2022 12:53

add database support for certificate

e1d0d08

add hare output certifier

7e8346c

sync fetches certificate as hare output

51c2510

review feedback

79c4ec2

review feedback #1

095943b

use scale codec for CertifyMessage

2ff81bd

really prune BlockID/LayerID from CertifyMessage for db

5cb373c

use eligibility count as threshold

6deeb31

shorten protocol even more

01ab8db

allow multiple blocks

0271174

log error when multiple cert is generated

6f463b2

revert useless zeroing out op

44077ef

revert multiple blocks support

8f2c7f5

code review feedback - renames

6d05566

fix unit test

2c0c2ad

full circle

bfb910b

bors bot added a commit that referenced this pull request Aug 19, 2022

Try #3449:

5c789ce

bors bot changed the title ~~block: generate certificate for hare output~~ [Merged by Bors] - block: generate certificate for hare output Aug 19, 2022

bors bot closed this Aug 19, 2022

bors bot deleted the certifiable-fraud branch August 19, 2022 20:10

	func (c *Certifier) saveMessage(logger log.Log, msg types.CertifyMessage) error {
	c.mu.Lock()
	defer c.mu.Unlock()

	lid := msg.LayerID
	bid := msg.BlockID
	if state, err := c.getCertStateLocked(logger, lid, bid); err != nil {
	return err
	} else if state != pending { // already certified
	return nil
	}

[Merged by Bors] - block: generate certificate for hare output #3449

[Merged by Bors] - block: generate certificate for hare output #3449

Conversation

countvonzero commented Aug 16, 2022

Motivation

Changes

countvonzero commented Aug 16, 2022

countvonzero commented Aug 16, 2022

bors bot commented Aug 16, 2022

try

countvonzero commented Aug 17, 2022

bors bot commented Aug 17, 2022

try

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshulyak Aug 18, 2022 • 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

countvonzero commented Aug 18, 2022

bors bot commented Aug 18, 2022

try

jonZlotnik left a comment

Choose a reason for hiding this comment

jonZlotnik left a comment • edited Loading

Choose a reason for hiding this comment

jonZlotnik left a comment • edited Loading

Choose a reason for hiding this comment

countvonzero commented Aug 19, 2022

bors bot commented Aug 19, 2022

try

countvonzero commented Aug 19, 2022

bors bot commented Aug 19, 2022

dshulyak Aug 18, 2022 •

edited

Loading

jonZlotnik left a comment •

edited

Loading

jonZlotnik left a comment •

edited

Loading