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.

Sign up for GitHub

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

Jump to bottom

DKG test loop #3034

Merged
merged 29 commits into from
Jul 6, 2022
Merged

DKG test loop #3034

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
af45efd
Get rid of the current group selection code
lukasz-zimnoch Jun 24, 2022
e895b9b
Merge branch 'main' into dkg-loop
lukasz-zimnoch Jun 29, 2022
c310f80
Start DKG every 100th block
lukasz-zimnoch Jun 29, 2022
e620aed
Cleanup the remaining parts related to ticket
lukasz-zimnoch Jun 29, 2022
762b0d8
Move `IsGroupRegistered` to
lukasz-zimnoch Jun 29, 2022
3d88e37
Remove current group selection deduplication logic
lukasz-zimnoch Jun 30, 2022
b0d2670
s/JoinGroupIfEligible/JoinDKGIfEligible
lukasz-zimnoch Jun 30, 2022
66e9c3a
Temporarily hardcode group size and honest
lukasz-zimnoch Jun 30, 2022
a46353d
Storing DKG test loop result on disk
lukasz-zimnoch Jun 30, 2022
a5b67cb
Detach DKG result submission from the v1 contract
lukasz-zimnoch Jun 30, 2022
0d940da
Remove the last parts related with ticket
lukasz-zimnoch Jun 30, 2022
002b15a
Integrate mock group selection
lukasz-zimnoch Jul 1, 2022
1900757
Remove obsolete deduplicator tests
lukasz-zimnoch Jul 1, 2022
b04ec33
Cache only approved groups
lukasz-zimnoch Jul 4, 2022
0aea172
Additional logging around DKG trigger
lukasz-zimnoch Jul 4, 2022
05cc9ca
Trigger DKG loop every 500th block
lukasz-zimnoch Jul 4, 2022
e002bfb
Return to 64 group members
lukasz-zimnoch Jul 4, 2022
1c5a815
Return after group registration error
lukasz-zimnoch Jul 4, 2022
c80c1ab
Fix candidate group storage
lukasz-zimnoch Jul 5, 2022
d2e607c
Merge branch 'main' into dkg-loop
lukasz-zimnoch Jul 5, 2022
6fd17ee
Fix failing group registry test
lukasz-zimnoch Jul 5, 2022
e49262d
Improve docs of `JoinDKGIfEligible` function
lukasz-zimnoch Jul 6, 2022
185fe7f
s/100th/500th
lukasz-zimnoch Jul 6, 2022
641466e
Revert changes related to group storage
lukasz-zimnoch Jul 6, 2022
5ac87df
s/channelName/groupPublicKey
lukasz-zimnoch Jul 6, 2022
ed315ca
TODO about key material snapshotting
lukasz-zimnoch Jul 6, 2022
fbe3062
Deduplication of `DKGStarted` events
lukasz-zimnoch Jul 6, 2022
7c122d5
Mention about disabling subscription monitoring
lukasz-zimnoch Jul 6, 2022
10e12b9
Make the DKG test loop working without v1
lukasz-zimnoch Jul 6, 2022
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
31 changes: 17 additions & 14 deletions cmd/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,20 +111,23 @@ func Start(c *cli.Context) error {
return err
}
}
hasMinimumStake, err := stakeMonitor.HasMinimumStake(
operatorPublicKey,
)
if err != nil {
return fmt.Errorf("could not check the stake [%v]", err)
}
if !hasMinimumStake {
return fmt.Errorf(
"no minimum KEEP stake or operator is not authorized to use it; " +
"please make sure the operator address in the configuration " +
"is correct and it has KEEP tokens delegated and the operator " +
"contract has been authorized to operate on the stake",
)
}

// TODO: Disable the minimum stake check to be able to start the client
// without v1 contracts deployed.
// hasMinimumStake, err := stakeMonitor.HasMinimumStake(
// operatorPublicKey,
// )
// if err != nil {
// return fmt.Errorf("could not check the stake [%v]", err)
// }
// if !hasMinimumStake {
// return fmt.Errorf(
// "no minimum KEEP stake or operator is not authorized to use it; " +
// "please make sure the operator address in the configuration " +
// "is correct and it has KEEP tokens delegated and the operator " +
// "contract has been authorized to operate on the stake",
// )
// }

netProvider, err := libp2p.Connect(
ctx,
Expand Down
62 changes: 13 additions & 49 deletions pkg/beacon/beacon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,16 @@ package beacon
import (
"context"
"encoding/hex"
"github.com/keep-network/keep-core/pkg/operator"
"time"

"github.com/keep-network/keep-core/pkg/operator"

"github.com/ipfs/go-log"

"github.com/keep-network/keep-common/pkg/persistence"
"github.com/keep-network/keep-core/pkg/beacon/relay"
relaychain "github.com/keep-network/keep-core/pkg/beacon/relay/chain"
dkgresult "github.com/keep-network/keep-core/pkg/beacon/relay/dkg/result"
"github.com/keep-network/keep-core/pkg/beacon/relay/event"
"github.com/keep-network/keep-core/pkg/beacon/relay/gjkr"
"github.com/keep-network/keep-core/pkg/beacon/relay/groupselection"
"github.com/keep-network/keep-core/pkg/beacon/relay/registry"
"github.com/keep-network/keep-core/pkg/chain"
"github.com/keep-network/keep-core/pkg/net"
Expand Down Expand Up @@ -64,19 +62,7 @@ func Initialize(
groupRegistry,
)

// We need to calculate group selection duration here as we can't do it
// inside the deduplicator due to import cycles. We don't include the
// time needed for publication as we are interested about the minimum
// possible off-chain group create protocol duration.
minGroupCreationDurationBlocks :=
chainConfig.TicketSubmissionTimeout +
gjkr.ProtocolBlocks() +
dkgresult.PrePublicationBlocks()

eventDeduplicator := event.NewDeduplicator(
relayChain,
minGroupCreationDurationBlocks,
)
eventDeduplicator := event.NewDeduplicator(relayChain)

node.ResumeSigningIfEligible(relayChain, signing)

Expand Down Expand Up @@ -150,54 +136,32 @@ func Initialize(
)
})

_ = relayChain.OnGroupSelectionStarted(func(event *event.GroupSelectionStart) {
onGroupSelected := func(group *groupselection.Result) {
for index, staker := range group.SelectedStakers {
logger.Infof(
"new candidate group member [0x%v] with index [%v]",
hex.EncodeToString(staker),
index,
)
}
node.JoinGroupIfEligible(
relayChain,
signing,
group,
event.NewEntry,
)
}

_ = relayChain.OnDKGStarted(func(event *event.DKGStarted) {
go func() {
if ok := eventDeduplicator.NotifyGroupSelectionStarted(
event.BlockNumber,
if ok := eventDeduplicator.NotifyDKGStarted(
event.Seed,
); !ok {
logger.Warningf(
"group selection event with seed [0x%x] and "+
"DKG started event with seed [0x%x] and "+
"starting block [%v] has been already processed",
event.NewEntry,
event.Seed,
event.BlockNumber,
)
return
}

logger.Infof(
"group selection started with seed [0x%x] at block [%v]",
event.NewEntry,
"DKG started with seed [0x%x] at block [%v]",
event.Seed,
event.BlockNumber,
)

err = groupselection.CandidateToNewGroup(
node.JoinDKGIfEligible(
relayChain,
blockCounter,
chainConfig,
staker,
event.NewEntry,
signing,
event.Seed,
event.BlockNumber,
onGroupSelected,
)
if err != nil {
logger.Errorf("tickets submission failed: [%v]", err)
}
}()
})

Expand Down
40 changes: 17 additions & 23 deletions pkg/beacon/relay/chain/chain.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ import (
)

// StakerAddress represents chain-specific address of the staker.
// DEPRECATED
// TODO: The "staker" should probably become "operator" to reflect random
// beacon v2 structure.
type StakerAddress []byte

// GroupMemberIndex is an index of a threshold relay group member.
Expand Down Expand Up @@ -52,21 +55,10 @@ type RelayEntryInterface interface {
// GroupSelectionInterface defines the subset of the relay chain interface that
// pertains to relay group selection activities.
type GroupSelectionInterface interface {
// OnGroupSelectionStarted is a callback that is invoked when an on-chain
// group selection started and the contract is ready to accept tickets.
OnGroupSelectionStarted(
func(groupSelectionStarted *event.GroupSelectionStart),
) subscription.EventSubscription
// SubmitTicket submits a ticket corresponding to the virtual staker to
// the chain, and returns a promise to track the submission. The promise
// is fulfilled with the entry as seen on-chain, or failed if there is an
// error submitting the entry.
SubmitTicket(ticket *Ticket) *async.EventGroupTicketSubmissionPromise
// GetSubmittedTickets gets the submitted group candidate tickets so far.
GetSubmittedTickets() ([]uint64, error)
// GetSelectedParticipants returns `GroupSize` slice of addresses of
// candidates which have been selected to the currently assembling group.
GetSelectedParticipants() ([]StakerAddress, error)
// SelectGroup returns the group members for the group generated by
// the given seed. This function can return an error if the relay chain's
// state does not allow for group selection at the moment.
SelectGroup(seed *big.Int) ([]StakerAddress, error)
Copy link
Member

Choose a reason for hiding this comment

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

#3056 introduces chain.Address, I think we could use it here.

Copy link
Member

Choose a reason for hiding this comment

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

Just a note for the future implementation with a real SortitionPool: fortunately, there is a function allowing to fetch the addresses in one shot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, there is a comment about the deprecation of StakerAddress. I can replace it with chain.Address during the cleanup, I plan to do in the pkg/beacon package.

Copy link
Contributor

Choose a reason for hiding this comment

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

When we call SelectGroup and assign a return value in node.go we call that var groupMembers and IMO that name is clearer because we return an array of members. WDYT about naming it SelectGroupMembers instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

There will be a follow-up PR that will add a real-world implementation of that function. I'll consider the rename then.

}

// GroupRegistrationInterface defines the subset of the relay chain interface
Expand All @@ -77,8 +69,11 @@ type GroupRegistrationInterface interface {
OnGroupRegistered(
func(groupRegistration *event.GroupRegistration),
) subscription.EventSubscription
// Checks if a group with the given public key is considered as
// stale on-chain. Group is considered as stale if it is expired and when
// IsGroupRegistered checks if group with the given public key is registered
// on-chain.
IsGroupRegistered(groupPublicKey []byte) (bool, error)
// IsStaleGroup checks if a group with the given public key is considered
// as stale on-chain. Group is considered as stale if it is expired and when
// its expiration time and potentially executed operation timeout are both
// in the past. Stale group is never selected by the chain to any new
// operation.
Expand All @@ -99,6 +94,11 @@ type GroupInterface interface {
// interface that pertains specifically to group formation's distributed key
// generation process.
type DistributedKeyGenerationInterface interface {
// OnDKGStarted registers a callback that is invoked when an on-chain
// notification of the DKG process start is seen.
OnDKGStarted(
func(event *event.DKGStarted),
) subscription.EventSubscription
// SubmitDKGResult sends DKG result to a chain, along with signatures over
// result hash from group participants supporting the result.
// Signatures over DKG result hash are collected in a map keyed by signer's
Expand All @@ -113,9 +113,6 @@ type DistributedKeyGenerationInterface interface {
OnDKGResultSubmitted(
func(event *event.DKGResultSubmission),
) subscription.EventSubscription
// IsGroupRegistered checks if group with the given public key is registered
// on-chain.
IsGroupRegistered(groupPublicKey []byte) (bool, error)
// CalculateDKGResultHash calculates 256-bit hash of DKG result in standard
// specific for the chain. Operation is performed off-chain.
CalculateDKGResultHash(dkgResult *DKGResult) (DKGResultHash, error)
Expand Down Expand Up @@ -147,9 +144,6 @@ type Config struct {
// HonestThreshold is the minimum number of active participants behaving
// according to the protocol needed to generate a new relay entry.
HonestThreshold int
// TicketSubmissionTimeout is the duration (in blocks) the staker has to
// submit any tickets to candidate to a new group.
TicketSubmissionTimeout uint64
// ResultPublicationBlockStep is the duration (in blocks) that has to pass
// before group member with the given index is eligible to submit the
// result.
Expand Down
17 changes: 0 additions & 17 deletions pkg/beacon/relay/chain/group_ticket.go
View file
Open in desktop

This file was deleted.

56 changes: 29 additions & 27 deletions pkg/beacon/relay/event/deduplicator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,16 @@ package event
import (
"encoding/hex"
"fmt"
"github.com/keep-network/keep-common/pkg/cache"
"math/big"
"sync"
"time"
)

const (
// DKGSeedCachePeriod is the time period the cache maintains
// the DKG seed corresponding to a DKG instance.
DKGSeedCachePeriod = 7 * 24 * time.Hour
)

// Local chain interface to avoid import cycles.
Expand All @@ -24,51 +32,45 @@ type chain interface {
// should be handled.
//
// Those events are supported:
// - group selection started
// - DKG started
// - relay entry requested
type Deduplicator struct {
chain chain
minGroupCreationDurationBlocks uint64
chain chain

groupSelectionMutex sync.Mutex
currentGroupSelectionStartBlock uint64
dkgSeedCache *cache.TimeCache

relayEntryMutex sync.Mutex
currentRequestStartBlock uint64
currentRequestPreviousEntry string
}

// NewDeduplicator constructs a new Deduplicator instance.
func NewDeduplicator(
chain chain,
minGroupCreationDurationBlocks uint64,
) *Deduplicator {
func NewDeduplicator(chain chain) *Deduplicator {
return &Deduplicator{
chain: chain,
minGroupCreationDurationBlocks: minGroupCreationDurationBlocks,
chain: chain,
dkgSeedCache: cache.NewTimeCache(DKGSeedCachePeriod),
}
}

// NotifyGroupSelectionStarted notifies the client wants to start group
// selection upon receiving an event. It returns boolean indicating whether the
// NotifyDKGStarted notifies the client wants to start the distributed key
// generation upon receiving an event. It returns boolean indicating whether the
// client should proceed with the execution or ignore the event as a duplicate.
func (d *Deduplicator) NotifyGroupSelectionStarted(
newGroupSelectionStartBlock uint64,
func (d *Deduplicator) NotifyDKGStarted(
newDKGSeed *big.Int,
) bool {
d.groupSelectionMutex.Lock()
defer d.groupSelectionMutex.Unlock()

minCurrentGroupCreationEndBlock := d.currentGroupSelectionStartBlock +
d.minGroupCreationDurationBlocks

shouldUpdate := d.currentGroupSelectionStartBlock == 0 ||
newGroupSelectionStartBlock > minCurrentGroupCreationEndBlock

if shouldUpdate {
d.currentGroupSelectionStartBlock = newGroupSelectionStartBlock
d.dkgSeedCache.Sweep()

// The cache key is the hexadecimal representation of the seed.
cacheKey := newDKGSeed.Text(16)
// If the key is not in the cache, that means the seed was not handled
// yet and the client should proceed with the execution.
if !d.dkgSeedCache.Has(cacheKey) {
d.dkgSeedCache.Add(cacheKey)
return true
}

// Otherwise, the DKG seed is a duplicate and the client should not proceed
// with the execution.
return false
}

Copy link
Member

Choose a reason for hiding this comment

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

Quoting from the PR description:

We removed the event deduplicator for the group selection events as it is no longer relevant for v2. We will need a new logic to handle v2 caveats. This was covered by a TODO in beacon.go

I am not sure if this is the case. As far as I remember, the deduplicator was mostly handling soft forks. We were receiving the same event twice but with a different block number: n and then n+1. The implementation is quite naive: it checks if the group selection had a chance to finish since the last emitted group selection started event and if not, it was considering the event as a duplicate of the last one.

Although this is naive, and we could improve it by, for example, waiting for N blocks, then checking the last N blocks if the event is included, I think the current implementation should do the work, even for v2.

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I remember, the deduplicator was mostly handling soft forks

Not only soft forks. The event subscription mechanism from contract bindings can replay the event when fetching past events. This happens more often than soft forks and results in an event having the same block.

I think the current implementation should do the work, even for v2.

This is something I'm not sure about. In v2 the minimum time between two DkgStarted events is DKG protocol time + publication time + challenge period time. In theory, we can expose NotifyGroupCreationStarted function from the event deduplicator to decide about group creation start. However, at the same time, we must be prepared for DkgResultChallenged events that actually restart the process and make the time between two DkgStarted events longer. The deduplicator must handle that. This is why I think we need to approach deduplication logic once we have challenges and approvals implemented together.

Copy link
Member

@pdyraga pdyraga Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not only soft forks. The event subscription mechanism from contract bindings can replay the event when fetching past events. This happens more often than soft forks and results in an event having the same block.

Yeah, that's a good point!

The time it takes the DKG protocol to produce the result is 72 blocks ([1], [2]). Subscription monitoring mechanism pulls 100 past blocks by default and although it could make sense for the relay request subscription, it does not make sense for DKG because after the first 6 blocks of inactivity, the game for the given member is over.

This solution is not ideal but at the same time, it's the simplest one. I am afraid that dealing with challenges will make this code pretty complicated.

I think that this should work:

  1. Leave the deduplicator for DKG as-is.
  2. Make it clear NotifyGroupSelectionStarted filters out just for minGroupCreationDurationBlocks to the past.
  3. When setting up a subscription for DKG, disable subscription monitoring loop. For 6 blocks, we would need to monitor pretty aggressively (every 10 seconds or so).

Does it make sense?

Copy link
Member

Choose a reason for hiding this comment

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

We synced with @lukasz-zimnoch and came up with a better solution:

  1. When setting up a subscription for DKG, disable subscription monitoring loop.
  2. Use a time-cache-based deduplicator and look at the DKG seed from the event.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here it is fbe3062

Copy link
Member Author

Choose a reason for hiding this comment

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

And added a TODO about disabling the monitoring loop 7c122d5

Expand Down Expand Up @@ -122,7 +124,7 @@ func (d *Deduplicator) NotifyRelayEntryStarted(
if newRequestPreviousEntry ==
hex.EncodeToString(currentRequestPreviousEntryOnChain[:]) &&
newRequestStartBlock ==
currentRequestStartBlockOnChain.Uint64() {
currentRequestStartBlockOnChain.Uint64() {
return true, nil
}
} else {
Expand Down
Loading