Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change host API to separate fetching chain from fetching power tables #256

Merged
merged 1 commit into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 11 additions & 9 deletions gpbft/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,17 @@ type Receiver interface {
}

type Chain interface {
// Returns inputs to the next GPBFT instance.
// These are:
// - the EC chain to propose,
// - the power table specifying the participants,
// - the beacon value for generating tickets.
// These will be used as input to a subsequent instance of the protocol.
// The chain should be a suffix of the last chain notified to the host via
// ReceiveDecision (or known to be final via some other channel).
GetCanonicalChain() (chain ECChain, power PowerTable, beacon []byte)
// Returns the chain to propose for a new GPBFT instance.
// This should be a suffix of the chain finalised by the immediately prior instance.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want a stronger wording for chain suffix?

Suggested change
// This should be a suffix of the chain finalised by the immediately prior instance.
// This must be a suffix of the chain finalised by the immediately prior instance.

Thinking out loud: It would also be nice to be able to validate this inside F3 but I am not sure how if F3 delegates storage of finalised chains to the host. As is it feels error prone with dire consequences in case of error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is written from the narrow point of view of what does this code need. And in fact it will function correctly whether or not this is true (e.g. at genesis, or in case of a social agreement to re-start somewhere else, etc). This code doesn't need it to be a suffix, we can't write any test that will fail if it isn't, etc. Whether or not the multi-party protocol will make progress if this doesn't hold is a higher level matter.

I'd rather delete the comment than change it to a must.

// Returns an error if the chain for the instance is not available.
GetChainForInstance(instance uint64) (chain ECChain, err error)

// Returns the power table and beacon value to be used for a GPBFT instance.
// These values should be derived from a chain previously received as final by the host,
// or known to be final via some other channel (e.g. when bootstrapping the protocol).
// The offset (how many instances to look back) is determined by the host.
// Returns an error if the committee for the instance is not available.
GetCommitteeForInstance(instance uint64) (power *PowerTable, beacon []byte, err error)
}

// Endpoint to which participants can send messages.
Expand Down
113 changes: 86 additions & 27 deletions gpbft/mock_host_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 10 additions & 3 deletions gpbft/participant.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,10 @@ func (p *Participant) ReceiveAlarm() (err error) {
}

func (p *Participant) beginInstance() error {
chain, power, beacon := p.host.GetCanonicalChain()
chain, err := p.host.GetChainForInstance(p.nextInstance)
if err != nil {
return fmt.Errorf("failed fetching chain for instance %d: %w", p.nextInstance, err)
}
// Limit length of the chain to be proposed.
if chain.IsZero() {
return errors.New("canonical chain cannot be zero-valued")
Expand All @@ -153,11 +156,15 @@ func (p *Participant) beginInstance() error {
if err := chain.Validate(); err != nil {
return fmt.Errorf("invalid canonical chain: %w", err)
}

power, beacon, err := p.host.GetCommitteeForInstance(p.nextInstance)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the next PR, these values will be cached in the participant and used to validate message. So each power table need only be computed once.

if err != nil {
return fmt.Errorf("failed fetching power table for instance %d: %w", p.nextInstance, err)
}
if err := power.Validate(); err != nil {
return fmt.Errorf("invalid power table: %w", err)
}
var err error
if p.granite, err = newInstance(p, p.nextInstance, chain, power, beacon); err != nil {
if p.granite, err = newInstance(p, p.nextInstance, chain, *power, beacon); err != nil {
return fmt.Errorf("failed creating new granite instance: %w", err)
}
p.nextInstance += 1
Expand Down
38 changes: 31 additions & 7 deletions gpbft/participant_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,14 +82,16 @@ func newParticipantTestSubject(t *testing.T, seed int64, instance uint64) *parti

func (pt *participantTestSubject) expectBeginInstance() {
// Prepare the test host.
pt.host.On("GetCanonicalChain").Return(pt.canonicalChain, *pt.powerTable, pt.beacon)
pt.host.On("GetChainForInstance", pt.instance).Return(pt.canonicalChain, nil)
pt.host.On("GetCommitteeForInstance", pt.instance).Return(pt.powerTable, pt.beacon, nil)
pt.host.On("Time").Return(pt.time)
pt.host.On("NetworkName").Return(pt.networkName).Maybe()
pt.host.On("MarshalPayloadForSigning", mock.AnythingOfType("*gpbft.Payload")).
Return([]byte(gpbft.DOMAIN_SEPARATION_TAG + ":" + pt.networkName))

// Expect calls to get the host state prior to beginning of an instance.
pt.host.EXPECT().GetCanonicalChain()
pt.host.EXPECT().GetChainForInstance(pt.instance)
pt.host.EXPECT().GetCommitteeForInstance(pt.instance)
pt.host.EXPECT().Time()

// Expect alarm is set to 2X of configured delta.
Expand Down Expand Up @@ -189,14 +191,14 @@ func TestParticipant(t *testing.T) {
t.Run("panic is recovered", func(t *testing.T) {
t.Run("on Start", func(t *testing.T) {
subject := newParticipantTestSubject(t, seed, 0)
subject.host.On("GetCanonicalChain").Panic("saw me no chain")
subject.host.On("GetChainForInstance", subject.instance).Panic("saw me no chain")
require.NotPanics(t, func() {
require.ErrorContains(t, subject.Start(), "saw me no chain")
})
})
t.Run("on ReceiveAlarm", func(t *testing.T) {
subject := newParticipantTestSubject(t, seed, 0)
subject.host.On("GetCanonicalChain").Panic("saw me no chain")
subject.host.On("GetChainForInstance", subject.instance).Panic("saw me no chain")
require.NotPanics(t, func() {
require.ErrorContains(t, subject.ReceiveAlarm(), "saw me no chain")
})
Expand Down Expand Up @@ -249,23 +251,45 @@ func TestParticipant(t *testing.T) {
subject.requireInstanceRoundPhase(47, 0, gpbft.QUALITY_PHASE)
})
})
t.Run("instance is no begun", func(t *testing.T) {
t.Run("instance is not begun", func(t *testing.T) {
t.Run("on zero canonical chain", func(t *testing.T) {
subject := newParticipantTestSubject(t, seed, 0)
var zeroChain gpbft.ECChain
subject.host.On("GetCanonicalChain").Return(zeroChain, *subject.powerTable, subject.beacon)
subject.host.On("GetChainForInstance", subject.instance).Return(zeroChain, nil)
require.ErrorContains(t, subject.Start(), "cannot be zero-valued")
subject.assertHostExpectations()
subject.requireNotStarted()
})
t.Run("on invalid canonical chain", func(t *testing.T) {
subject := newParticipantTestSubject(t, seed, 0)
invalidChain := gpbft.ECChain{gpbft.TipSet{}}
subject.host.On("GetCanonicalChain").Return(invalidChain, *subject.powerTable, subject.beacon)
subject.host.On("GetChainForInstance", subject.instance).Return(invalidChain, nil)
require.ErrorContains(t, subject.Start(), "invalid canonical chain")
subject.assertHostExpectations()
subject.requireNotStarted()
})
t.Run("on failure to fetch chain", func(t *testing.T) {
subject := newParticipantTestSubject(t, seed, 0)
invalidChain := gpbft.ECChain{gpbft.TipSet{}}
subject.host.On("GetChainForInstance", subject.instance).Return(invalidChain, errors.New("fish"))
require.ErrorContains(t, subject.Start(), "fish")
subject.assertHostExpectations()
subject.requireNotStarted()
})
t.Run("on failure to fetch committee", func(t *testing.T) {
subject := newParticipantTestSubject(t, seed, 0)
chain := gpbft.ECChain{gpbft.TipSet{
Epoch: 0,
Key: []byte("key"),
PowerTable: []byte("pt"),
Commitments: [32]byte{},
}}
subject.host.On("GetChainForInstance", subject.instance).Return(chain, nil)
subject.host.On("GetCommitteeForInstance", subject.instance).Return(nil, nil, errors.New("fish"))
require.ErrorContains(t, subject.Start(), "fish")
subject.assertHostExpectations()
subject.requireNotStarted()
})
})
})
t.Run("when started", func(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions sim/adversary/decide.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func (i *ImmediateDecide) ID() gpbft.ActorID {
}

func (i *ImmediateDecide) Start() error {
_, powertable, _ := i.host.GetCanonicalChain()
powertable, _, _ := i.host.GetCommitteeForInstance(0)
// Immediately send a DECIDE message
payload := gpbft.Payload{
Instance: 0,
Expand Down Expand Up @@ -91,7 +91,7 @@ func (i *ImmediateDecide) AllowMessage(_ gpbft.ActorID, _ gpbft.ActorID, _ gpbft
return true
}

func (i *ImmediateDecide) broadcast(payload gpbft.Payload, justification *gpbft.Justification, powertable gpbft.PowerTable) {
func (i *ImmediateDecide) broadcast(payload gpbft.Payload, justification *gpbft.Justification, powertable *gpbft.PowerTable) {

pS := i.host.MarshalPayloadForSigning(&payload)
_, pubkey := powertable.Get(i.id)
Expand Down
2 changes: 1 addition & 1 deletion sim/adversary/repeat.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func (r *Repeat) ReceiveMessage(msg *gpbft.GMessage, _ bool) (bool, error) {
}

sigPayload := r.host.MarshalPayloadForSigning(&msg.Vote)
_, power, beacon := r.host.GetCanonicalChain()
power, beacon, _ := r.host.GetCommitteeForInstance(0)
_, pubkey := power.Get(r.id)

sig, err := r.host.Sign(pubkey, sigPayload)
Expand Down
4 changes: 2 additions & 2 deletions sim/adversary/withhold.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (w *WithholdCommit) Start() error {
return errors.New("victims must be set")
}

_, powertable, _ := w.host.GetCanonicalChain()
powertable, _, _ := w.host.GetCommitteeForInstance(0)
broadcast := w.broadcastHelper(w.id, powertable)
// All victims need to see QUALITY and PREPARE in order to send their COMMIT,
// but only the one victim will see our COMMIT.
Expand Down Expand Up @@ -165,7 +165,7 @@ func (w *WithholdCommit) sign(pubkey gpbft.PubKey, msg []byte) []byte {
return sig
}

func (w *WithholdCommit) broadcastHelper(sender gpbft.ActorID, powertable gpbft.PowerTable) func(gpbft.Payload, *gpbft.Justification) {
func (w *WithholdCommit) broadcastHelper(sender gpbft.ActorID, powertable *gpbft.PowerTable) func(gpbft.Payload, *gpbft.Justification) {
return func(payload gpbft.Payload, justification *gpbft.Justification) {
pS := w.host.MarshalPayloadForSigning(&payload)
_, pubkey := powertable.Get(sender)
Expand Down
9 changes: 6 additions & 3 deletions sim/ec.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ type ECInstance struct {
Instance uint64
// The base of all chains, which participants must agree on.
BaseChain gpbft.ECChain
// The power table at the base chain head.
// The power table to be used for this instance.
PowerTable *gpbft.PowerTable
// The beacon value of the base chain head.
// The beacon value to use for this instance.
Beacon []byte

ec *simEC
Expand All @@ -56,7 +56,10 @@ func newEC(opts *options) *simEC {
}
}

func (ec *simEC) BeginInstance(baseChain gpbft.ECChain, pt *gpbft.PowerTable, beacon []byte) *ECInstance {
func (ec *simEC) BeginInstance(baseChain gpbft.ECChain, pt *gpbft.PowerTable) *ECInstance {
// Take beacon value from the head of the base chain.
// Note a real beacon value will come from a finalised chain with some lookback.
beacon := baseChain.Head().Key
instance := &ECInstance{
Instance: uint64(ec.Len()),
BaseChain: baseChain,
Expand Down
Loading
Loading