Roster change preparation #40

Gilthoniel · 2020-04-24T15:26:15Z

This refactors the roster abstraction to move it from the blockchain module to the consensus one as some could not actually depend on a roster (PoW). It's up to the consensus to control it, like cosipbft must verify the leader is proposing.

Also: some refactoring in Byzcoin
Note: Missing roster txs that will come later alongside Byzcoin unit tests

nkcr

Nice work! As usual, here is my "general-but-not-thorough" review.

nkcr · 2020-04-29T09:58:03Z

ledger/byzcoin/roster.go

+	index := i.iterator.GetNext()
+	if index >= 0 {
+		return i.roster.addrs[index]
+	}


Suggested change

index := i.iterator.GetNext()

if index >= 0 {

return i.roster.addrs[index]

}

if i.iterator.HasNext() {

return i.roster.addrs[i.iterator.GetNext()]

}

nkcr · 2020-04-29T09:59:47Z

ledger/byzcoin/roster.go

+	index := i.iterator.GetNext()
+	if index >= 0 {
+		return i.roster.pubkeys[index]
+	}


Suggested change

index := i.iterator.GetNext()

if index >= 0 {

return i.roster.pubkeys[index]

}

if i.iterator.HasNext() {

return i.roster.pubkeys[i.iterator.GetNext()]

}

nkcr · 2020-04-29T10:03:17Z

ledger/byzcoin/roster.go

+func (i *iterator) GetNext() int {
+	if i.HasNext() {
+		i.index++
+		return i.index
+	}
+	return -1
+}


It should be the user's responsibility to call HasNext() before a GetNext(), therefore I wouldn't add a special case there, as the responsibility is commonly defined for an iterator: if HasNext() returns true, then GetNext() returns something, else there can't be any assumption.

Suggested change

func (i *iterator) GetNext() int {

if i.HasNext() {

i.index++

return i.index

}

return -1

}

func (i *iterator) GetNext() int {

res := i.index // for an index that starts at 0

i.index++

return res

}

nkcr · 2020-04-29T10:08:02Z

ledger/byzcoin/roster.go

+}
+
+func (i *iterator) HasNext() bool {
+	return i.index+1 < i.roster.Len()


I would find it more natural to start the index at 0, it also has the advantage of using the default/zero value in a meaningful way when creating the iterator.

nkcr · 2020-04-29T11:13:12Z

ledger/byzcoin/roster.go

+// AddressIterator implements mino.Players. It returns an iterator of the
+// addresses of the roster in a deterministic order.
+func (r roster) AddressIterator() mino.AddressIterator {
+	return &addressIterator{iterator: &iterator{roster: &r, index: -1}}


based on my previous comment

Suggested change

return &addressIterator{iterator: &iterator{roster: &r, index: -1}}

return &addressIterator{iterator: &iterator{roster: &r}}

nkcr · 2020-04-29T14:55:57Z

consensus/cosipbft/forward_link.go

+	enc encoding.ProtoMarshaler) ([]*Player, error) {
+
+	if len(in) == 0 {
+		// Save some bytes in the message with a nil array.


a penny saved is a penny earned 😄

nkcr · 2020-04-29T14:57:35Z

consensus/cosipbft/forward_link.go

+		}
+	}
+
+	buffer := make([]byte, 4+(4*len(fl.changeset.Remove)))


I would comment why this magic number

nkcr · 2020-04-29T14:59:51Z

consensus/cosipbft/forward_link.go

+	var err error
+	fl.changeset, err = f.decodeChangeSet(src.GetChangeSet())
+	if err != nil {
+		return fl, xerrors.Errorf("couldn't decode changeset: %v", err)
+	}


Suggested change

var err error

fl.changeset, err = f.decodeChangeSet(src.GetChangeSet())

if err != nil {

return fl, xerrors.Errorf("couldn't decode changeset: %v", err)

}

changeset, err := f.decodeChangeSet(src.GetChangeSet())

if err != nil {

return fl, xerrors.Errorf("couldn't decode changeset: %v", err)

}

fl = forwardLink{

from: src.GetFrom(),

to: src.GetTo(),

changeset: changeset,

}

nkcr · 2020-04-29T15:01:37Z

consensus/cosipbft/forward_link.go

+	changeset := viewchange.ChangeSet{
+		Leader: in.GetLeader(),
+		Remove: in.GetRemove(),
+	}
+
+	var err error
+	changeset.Add, err = f.decodeAdd(in.GetAdd())
+	if err != nil {
+		return changeset, xerrors.Errorf("couldn't decode add: %v", err)
+	}
+
+	return changeset, nil


Suggested change

changeset := viewchange.ChangeSet{

Leader: in.GetLeader(),

Remove: in.GetRemove(),

}

var err error

changeset.Add, err = f.decodeAdd(in.GetAdd())

if err != nil {

return changeset, xerrors.Errorf("couldn't decode add: %v", err)

}

return changeset, nil

add, err := f.decodeAdd(in.GetAdd())

if err != nil {

return changeset, xerrors.Errorf("couldn't decode add: %v", err)

}

changeset := viewchange.ChangeSet{

Leader: in.GetLeader(),

Remove: in.GetRemove(),

Add: add,

}

return changeset, nil

nkcr · 2020-04-29T15:04:30Z

consensus/cosipbft/forward_link.go

+func (f *chainFactory) FromProto(pb proto.Message) (consensus.Chain, error) {
+	chain, err := f.unsecureChainFactory.FromProto(pb)
+	if err != nil {
+		return nil, err


a forgotten error wrap? Otherwise I would comment why.

commented ;)

nkcr

I wasn't sure you would welcome my weird comment about "get and process" lines, so I took the opportunity now to point out some other sections that I previously left. If you agree I'll update the guidelines to include this case.

nkcr · 2020-04-30T12:53:11Z

consensus/cosipbft/forward_link.go

@@ -60,6 +64,10 @@ func (fl forwardLink) Pack(enc encoding.ProtoMarshaler) (proto.Message, error) {
 	}

 	var err error
+	pb.ChangeSet, err = fl.packChangeSet(enc)


As a general guideline I would recommend that we first gather the needed variables and then define the structure. What do you think?

// Pack returns the protobuf message of the forward link. func (fl forwardLink) Pack(enc encoding.ProtoMarshaler) (proto.Message, error) { changeSet, err := fl.packChangeSet(enc) if err != nil { return nil, xerrors.Errorf("couldn't pack changeset: %v", err) } var prepare *any.Any if fl.prepare != nil { prepare, err = enc.PackAny(fl.prepare) if err != nil { return nil, xerrors.Errorf("couldn't pack prepare signature: %v", err) } } var commit *any.Any if fl.commit != nil { commit, err = enc.PackAny(fl.commit) if err != nil { return nil, xerrors.Errorf("couldn't pack commit signature: %v", err) } } pb := &ForwardLinkProto{ From: fl.from, To: fl.to, ChangeSet: changeSet, Prepare: prepare, Commit: commit, } return pb, nil }

General guideline, but like below it sometimes makes sense not to.

nkcr · 2020-04-30T12:59:12Z

consensus/cosipbft/mod.go

@@ -159,6 +186,27 @@ func (a pbftActor) Propose(p consensus.Proposal, players mino.Players) error {
 	return nil
 }

+func (a pbftActor) newPrepareRequest(prop consensus.Proposal,


func (a pbftActor) newPrepareRequest(prop consensus.Proposal, cs viewchange.ChangeSet) (Prepare, error) { forwardLink := forwardLink{ from: prop.GetPreviousHash(), to: prop.GetHash(), changeset: cs, } digest, err := forwardLink.computeHash(a.hashFactory.New(), a.encoder) if err != nil { return nil, xerrors.Errorf("couldn't compute hash: %v", err) } req := Prepare{ proposal: prop, digest: digest, } return req, nil }

This is an example why it really depends on the situation because there I return a struct so the nil does not work. Sometimes it's nice to declare the object at the beginning and fill it step by step so you can return the said object if an error occurred.

nkcr · 2020-04-30T13:07:35Z

docs/introduction.md

+- **governance** - Governance is a black box that provides to act on the
+  participants of a consensus.


Suggested change

- **governance** - Governance is a black box that provides to act on the

participants of a consensus.

- **governance** - Governance is a black box that gives the ability to act on

the participants of a consensus.

nkcr · 2020-04-30T13:16:35Z

ledger/byzcoin/mod.go

+		return xerrors.Errorf("couldn't store genesis payload: %v", err)
+	}
+
+	payload.Footprint = page.GetFootprint()


I find this section strange: You need a payload to create a page to create a footprint for that same payload. I don't have the whole picture in mind so that may make perfect sense, just noticing it.

Basically you apply the transactions of the payload to the inventory to get the footprint back.

nkcr · 2020-04-30T13:22:57Z

ledger/byzcoin/roster.go

+
+// Pack implements encoding.Packable. It returns the protobuf message for the
+// roster.
+func (r roster) Pack(enc encoding.ProtoMarshaler) (proto.Message, error) {


func (r roster) Pack(enc encoding.ProtoMarshaler) (proto.Message, error) { addrs := make([][]byte, r.Len()) pubKeys := make([]*any.Any, r.Len()) var err error for i, addr := range r.addrs { addrs[i], err = addr.MarshalText() if err != nil { return nil, xerrors.Errorf("couldn't marshal address: %v", err) } pubKeys[i], err = enc.PackAny(r.pubkeys[i]) if err != nil { return nil, xerrors.Errorf("couldn't pack public key: %v", err) } } pb := &Roster{ Addresses: addrs, PublicKeys: pubKey, } return pb, nil }

nkcr · 2020-04-30T13:25:02Z

ledger/byzcoin/roster.go

+	}
+}
+
+func (f rosterFactory) New(authority crypto.CollectiveAuthority) roster {


func (f rosterFactory) New(authority crypto.CollectiveAuthority) roster { addrs := make([]mino.Address, authority.Len()) pubKeys := make([]crypto.PublicKey, authority.Len()) addrIter := authority.AddressIterator() pubkeyIter := authority.PublicKeyIterator() for i := 0; addrIter.HasNext() && pubkeyIter.HasNext(); i++ { addrs[i] = addrIter.GetNext() pubkeys[i] = pubkeyIter.GetNext() } roster := roster{ addrs: addrs, pubkeys: pubKeys, } return roster }

Roster change preparation

Gilthoniel added 4 commits April 20, 2020 09:57

Byzcoin: setup after each node has started

6cb1172

Byzcoin: implement stop

d5b839f

Refactor to allow roster changes

5aea26d

Improve tests after the refactoring

826632c

Gilthoniel added the Byzcoin label Apr 24, 2020

Gilthoniel self-assigned this Apr 24, 2020

Gilthoniel added 5 commits April 27, 2020 16:32

Store leader in changeset to remember previous decisions

f38185a

Consensus: working roster change in cosipbft

fa7a51e

CoSiPBFT: finish the roster change

8fca22f

Byzcoin: governance unit test

9dba95e

Update some comments

fc1db89

Gilthoniel added the R4M 🚀 The PR is ready to be reviewed and merged label Apr 28, 2020

Gilthoniel requested a review from nkcr April 28, 2020 09:03

nkcr reviewed Apr 29, 2020

View reviewed changes

Address nkcr's comments

113521c

Gilthoniel requested a review from nkcr April 30, 2020 12:28

nkcr reviewed Apr 30, 2020

View reviewed changes

Address nkcr's comments

e52db0b

Gilthoniel requested a review from nkcr May 1, 2020 11:49

nkcr approved these changes May 1, 2020

View reviewed changes

nkcr merged commit 6509bf5 into master May 1, 2020

Gilthoniel deleted the roster_change branch May 1, 2020 12:02

bbjubjub2494 pushed a commit to bbjubjub2494/dela that referenced this pull request Aug 3, 2024

Merge pull request dedis#40 from dedis/roster_change

43cda60

Roster change preparation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roster change preparation #40

Roster change preparation #40

Gilthoniel commented Apr 24, 2020 •

edited

Loading

nkcr left a comment

nkcr Apr 29, 2020

nkcr Apr 29, 2020

nkcr Apr 29, 2020

nkcr Apr 29, 2020

nkcr Apr 29, 2020

nkcr Apr 29, 2020

nkcr Apr 29, 2020

nkcr Apr 29, 2020

nkcr Apr 29, 2020

nkcr Apr 29, 2020

Gilthoniel Apr 30, 2020

nkcr left a comment

nkcr Apr 30, 2020

Gilthoniel May 1, 2020

nkcr Apr 30, 2020

Gilthoniel May 1, 2020

nkcr Apr 30, 2020

nkcr Apr 30, 2020

Gilthoniel May 1, 2020

nkcr Apr 30, 2020

nkcr Apr 30, 2020

	return &addressIterator{iterator: &iterator{roster: &r, index: -1}}
	return &addressIterator{iterator: &iterator{roster: &r}}

		- governance - Governance is a black box that provides to act on the
		participants of a consensus.

Roster change preparation #40

Roster change preparation #40

Conversation

Gilthoniel commented Apr 24, 2020 • edited Loading

nkcr 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

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

nkcr 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

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

Gilthoniel commented Apr 24, 2020 •

edited

Loading