[WIP] Guard Tower: Initial interface and struct setups for distributed key generation by Shadowfiend · Pull Request #35 · threshold-network/keep-core

Shadowfiend · 2018-02-12T01:25:15Z

Wanting to get this PR a little further before we're good to go:

Fix some open TODOs in member code.
Put a frame similar to the one in [WIP] Locksmith: Distributed Key Generation frame #27 back into main.go.
Add signing to the interface.

Punted to future PRs:

Dem tests.
Set up broadcast-based signing.
Add group public key publishing to chain.

Extracted/refactored from the work in #27 .

Includes a healthy dose of documentation, as well as a NewMember constructor function. There are still some open questions around how to handle the id, but this should be a good starting point.

The one Member struct becomes a LocalMember, SharingMember, JustifyingMember, and Member struct. Each one of these represents a phase in DKG, except SharingMember which represents both the commitment and private share receiving/validation phase. These can occur at the same time without any issues, so we combine the struct. Each struct has an associated method to transition it to the next phase, in which we do any final work (e.g., when we go from LocalMember to SharingMember we compute the private member shares that are to be sent to each member of the threshold group). Each struct also exposes the various methods that make sense for its phase. The Member struct now represents a fully-fleshed-out member, ready to participate in threshold signatures. Note that there are still a few todos around properly tracking accusation/justification. There is also some validation that needs to happen so that we don't generate a final Member if we've failed to meet our threshold requirements for qualified members.

Shadowfiend · 2018-02-12T01:25:22Z

Interested in feedback around how I've broken down the member structure. I made ~1 struct per DKG phase, and allow the member to transition between the structs. They compose, so that the struct from justification contains the struct from previous phases. The idea is that this captures the current phase of the member, and allows us to set some type restrictions in our code so that we don't start making an uninitialized member sign things and such.

Rather than breaking out the structs, we could also use one big struct and make the transitions be transitions between interfaces that allow various operations. I'm interested in thoughts around whether this approach or the interface one is better… Or if there's another one altogether that I haven't considered.

@keep-network/go lemme know!

For now it's /keep-core/go/..., but we'll see if we come up with a different strategy.

This starts the formalization of distinctions between these two. Eventually private messages will need to encrypt, for example.

This allows message sends to not block the caller, so that the caller can move on to receiving from other participants.

There are a few places where we need it for filtering, though it's very likely these will be unnecessary if our underlying channels give us a way to not receive our own messages.

We were forgetting to set it, so even though we took it into the function we never propagated it to the member. We'll be using this for better public key creation once the DKG process is done.

OtherMemberIDs is just a convenient way to get the list of group member IDs other than a member's own ID, which we use when computing and shipping out private shares.

Without these methods we don't have a way externally of knowing that the member no longer needs to receive additional shares or commitments.

We were accidentally checking ourselves for received shares while deciding who to accuse. Of course we didn't receive shares for ourselves -- we simply have them available.

We were expecting to find this member's commitments in the commitments map, but they are tracked separately. We also change the combined commitments allocation to reference the threshold, since this is most directly correct.

BeaconConfig is a stub for getting the current configuration of the relay beacon from the chain. BlockCounter is a stub for setting up notifications of when a certain number of blocks has been observed.

DKG.Execute takes a block counter, broadcast channel, group size, and threshold, and attempts to initialize a single member in a threshold group. Multiple DKG.Execute calls executed in parallel, combined with a local broadcast channel, will result in a local DKG process occurring. This gives us the infrastructure to do a local non-networked test of DKG, with interface stubs to interact with a networked version as well. Note that currently the DKG relies entirely on block timeouts for phase synchronization. We'll need to consider whether that's necessary throughout.

We use a local broadcast channel and a channel for reporting the complete members as they wrap up. No group signing yet though!

Not a huge deal, but don't want a million packages.

We were expecting the group member's own share to be in the receivedShares map, but it was not. Instead, we have to query the member's secret shares to see the share it generated for itself. Then we add all the received shares.

No substantive change, but the explicit assignment makes it clear where the public key is coming from.

thresholdgroup.Member now has SignatureShare(message) to compute its share of a threshold signature, and VerifySignature(shares, message) to take a set of shares and verify that they are a valid signature of the message for this group. The shares are all passed around as byte arrays, so that the details of BLS are tucked away behind everything with the exception of BLS IDs (which we may hide later).

Notably, this doesn't do anything about thresholds, it does the whole thing.

Shadowfiend · 2018-02-13T00:35:36Z

Whelp, I was on a roll today…

This still has a few rough edges, but it mostly works. Specifically missing is the piece where group signing takes place over the broadcast channel (right now the main function just asks everyone for a signature share). But, that shouldn't be too hard, and since the signatures are broadcast it's fine.

When we receive an accusation for a member A from another member B, we need to track this fact so that we only consider member A to be a valid member if we receive their justification for member B. We were tracking these at a lower granularity, namely by simply tracking the member as being accused, and accepting any justification from them as validting all accusations against them. We now track the accusations between all member pairs, and record the justifications between member pairs. When we go to finalize the member, we do a final disqualification of all members who were accused without a corresponding valid justification. This yields a correct final list of qualified members.

The methods are defined in order from first used to last used during the DKG/relay process.

Shadowfiend · 2018-02-13T14:19:14Z

Yeah, build is broken due to the change in how I'm referencing dependencies… Going to stick with this approach (github.com/keep-network/keep-core/go/*) and update the Dockerfile accordingly for now, though it may not be what we want to do long-term. It's the most straightforward way to get things working out of the box atm.

Shadowfiend · 2018-02-13T14:20:48Z

Oh, and re: tests, yes 100%. I think we now have interfaces that will let those be written… I would also like to do them in this PR but feel like it may be too much to review at once. Partly my fault for biting too much off at once.

This is what we'll need to work with on-chain implementations, from our current understanding.

Shadowfiend · 2018-02-13T14:26:29Z

(Notably, the main function acts as a very limited pseudo-test right now hehe.)

We're not removing anything from the Keep GitHub paths for now.

rargulati

I gave this an initial run through. Great stuff for the prototype. I'm starting to see where we hook into this stuff.

I skimmed through relay/dkg and thresholdgroup/member a bit faster than I feel comfortable so I'll dig into it on the round 2.

rargulati · 2018-02-13T15:25:32Z

+	"sync"
+	"time"
+)
+


Documentation for the general high level of the interface - it'll make godoc output more informative!

Let me hold off here for another PR as this particular piece is still in a bit of flux.

Whoops jk I realized you meant for BlockCounter, not the chain package` :p

rargulati · 2018-02-13T15:30:16Z

+
+type localBlockCounter struct {
+	blockHeight int
+	heightMutex sync.Mutex


Could you add a doc that notes what this lock guards, ie. // guards blockHeight or // guards all?

I think the name should substitute for the doc… But this is misnamed because it was guarding the height and changed 😆

rargulati · 2018-02-13T15:40:05Z

+}
+
+func (counter *localBlockCounter) count() {
+	ticker := time.NewTicker(time.Duration(time.Second / 2))


500 * time.Millisecond?

rargulati · 2018-02-13T15:41:23Z

+		waiters, exists := counter.waiters[counter.blockHeight]
+		if exists {
+			for _, waiter := range waiters {
+				waiter <- counter.blockHeight


Talking outloud: we get a list of all pending waiters, if we have waiters at the blockheight that we're at, unblock them, and then remove them from the map. Nice.

Yah… Though I think the lock is actually wrong here (too broad).

Yeah, it's a pretty fat lock, but it's fine for now until perf testing shows otherwise. Though you can explore two locks or a RWMutex as well.

RW was actually my first move, but I realized it was wrong. I need this to be fully exclusive for correctness. Pushing a slightly finer grain for it.

rargulati · 2018-02-13T15:41:44Z

+			}
+			delete(counter.waiters, counter.blockHeight)
+		}
+		counter.heightMutex.Unlock()


Ah, ok. This is guarding both blockHeight and waiters

rargulati · 2018-02-13T16:59:41Z

+	fullSignature.Recover(deserializedShares, availableIDs)
+
+	return fullSignature.Verify(member.groupPublicKey, message)
+}


It looks like we can model a lot of this as a state machine - have a giant event loop that runs through state transitions. Thoughts?

Do you have an example of what this might look like? Giant state machines are often hard to reason about, but we may be able to get away with some nested state machines. Certainly once we get into signing-land we'll see some of this, and the DKG process is basically a state machine (though currently it's a very simple one).

rargulati · 2018-02-13T17:02:20Z

+	// other group member, we track their list of public commitments to their
+	// private secrets. This allows us to verify the share of their private
+	// secret that they send us.
+	commitments map[bls.ID][]bls.PublicKey


rargulati · 2018-02-13T17:04:24Z

+	// with no entry for their received share has either not sent their share
+	// or has sent an invalid share; they are therefore subject to an accusation
+	// requiring them to reveal their share to all group members.
+	receivedShares map[bls.ID]bls.SecretKey


Will this information need to be further encrypted in memory? In the odd event that someone gets on an individual's box, they shouldn't be able to scoop out the recvShares map from memory, right?

If they have access to the box, they have access to this data. Encrypting it in memory doesn't seem like it gains us much, since the machine will need to be able to decrypt as well. Would like @mhluongo's thoughts on that though.

Agreed, that attack is out of scope for now- the client simply isn't made to run in the same memory space as an attacker.

If we want we can add an issue to try to ensure the memory is never written to swap... I've seen that technique before

Feels like if we want to go down that path, we should open an issue as a catchall to capture hardening the client against local attacks… And then start breaking it down into clear subparts when we decide to take it on.

rargulati · 2018-02-13T17:07:07Z

+
+// Member represents a fully initialized threshold group member that is ready to
+// participate in group threshold signatures and signature validation.
+type Member struct {


can we call this GroupMember to differentiate between this and LocalMember

The package is thresholdgroup, making this thresholdgroup.Member, which I think means we shouldn't.

That said, this brings up the interesting idea that maybe most of the intermediate member states should live as private types in the DKG file, rather than public exports of the thresholdgroup package, and the final state is a thresholdgroup.Member.

rargulati · 2018-02-13T17:08:06Z

+	// 	receivedShares:      map[bls.ID]bls.SecretKey{},
+	// 	accusedIDs:          map[bls.ID]bool{},
+	// 	disqualifiedPlayers: map[bls.ID]bool{},
+	// }


kill this commented out code and put a TODO comment instead

These are actually good to die altogether; they're leftovers from before.

heightMutex implies the mutex locks only access to block height information, but in fact the mutex protects access to all struct members.

We append + assign further down in the function anyway.

We were wrapping the whole tick in the mutex, including waiter notification, but really we can read all the state we need quickly and notify waiters separately.

This most closely mimics waiting on individual clients, since each one will get its signal when it is ready for it. Without the goroutines, we find ourselves waiting for a slow waiter before notifying other waiters.

Shadowfiend · 2018-02-13T21:11:50Z

Pushed changes that aren't punted to a future PR. Still have an open question or two above though.

mhluongo

Maybe add comments around curve choice? It's got way more gravitas than the code suggests.

Otherwise I'd like to see this merged

mhluongo · 2018-02-15T06:30:16Z

 func main() {
-	r := rand.NewRand()
-	fmt.Printf("%v\n", r)
+	bls.Init(bls.CurveFp254BNb)


Seriously though, please give context on the curve choice- very important and every curve has 3 names wtf

If this is https://github.com/dfinity/bn/blob/master/mcl/include/mcl/bn256.hpp, then it's bn256 most places, and bn128 in the EIP- meaning it's compatible with the Ethereum pairing check 🎉

mhluongo · 2018-02-15T06:30:58Z

+	// with no entry for their received share has either not sent their share
+	// or has sent an invalid share; they are therefore subject to an accusation
+	// requiring them to reveal their share to all group members.
+	receivedShares map[bls.ID]bls.SecretKey


rargulati

Looks good to me

rargulati · 2018-02-16T15:39:32Z

+// channel watched by others. The message is signed and encrypted.
+func NewPrivateMessage(sender bls.ID, receiver bls.ID, data interface{}) Message {
+	// FIXME Actually encrypt here... Will require a key, best taken from
+	// FIXME chain...


Ahhhh....that makes sense.

rargulati · 2018-02-16T15:43:47Z

+// that is returned to the caller, so that all receive channels can receive
+// the message.
+func LocalChannel(name string) Channel {
+	return &localChannel{name, make([]chan Message, 0)}


Ah, I missed that! Carry on.

rargulati · 2018-02-16T15:47:31Z

There are some questions I have around channel synchronization, but that's stuff we can iron out when we run this + profile it later down the road! As far as the abstractions and functionality go, this is a great start and helps sets the tone for how we should structure our code.

mhluongo · 2018-02-16T16:43:09Z

Conflict fix and merge?

mhluongo · 2018-02-18T04:35:35Z

Wow this is rocking my machine

mhluongo · 2018-02-18T04:48:32Z

I was looking into slipping the libsnark curve into here... trying to test, I don't see the justification phase ever end?

[member:9a29e77a85a9b78db7f93bc1a113159deaed5ad37c77b132dd31dd683bb00070] Broadcasting justifications.
[member:21e03fe8711c5dc8c712d67e9799031b82ae939a6545baeca144aaf664a9bcfd] Broadcasting justifications.
[member:21e03fe8711c5dc8c712d67e9799031b82ae939a6545baeca144aaf664a9bcfd] Waiting for other justifications...
[member:f5d4acf223929d9a6e34b286431d2a69b0ffb21dc3452c9015477599a97d9cbe] Broadcasting justifications.
[member:f5d4acf223929d9a6e34b286431d2a69b0ffb21dc3452c9015477599a97d9cbe] Waiting for other justifications...
[member:6fb5c8da6b0a9f0ded310f01ec450401f4731c0e0fa4a4a5f5e80064be524455] Waiting for other justifications...
[member:0e4517ab41ac2da275ec4187a69749b1a0573c4fb64f77fa2711fdd08310f39b] Broadcasting justifications.
[member:0e4517ab41ac2da275ec4187a69749b1a0573c4fb64f77fa2711fdd08310f39b] Waiting for other justifications...
[member:9a29e77a85a9b78db7f93bc1a113159deaed5ad37c77b132dd31dd683bb00070] Waiting for other justifications...

No additional log output, it just hangs.

Shadowfiend · 2018-02-19T16:00:56Z

I'll see what I can do on the conflicts today to unblock, but as you observed there are some concurrency issues still lurking. They're particularly visible if you're trying large group sizes, which I gather is probably true given the “this is rocking my machine” remark ;) For now try sticking to the 20-30 range on group size and it should usually terminate.

Also try upping some of the block timeouts. Basically the problem is if some of the clients end up out of sync from the current phase, the channel writes + waits can get weird. Making some of the channels buffered will probably help, but will introduce its own issues (how big the buffer should be). Going to try and find some quick “this works all right enough” solutions so we can get this merged though.

Shadowfiend · 2018-02-20T20:56:17Z

Ok, I'm actually going to nuke this branch from orbit and rebase it on top of #41 to avoid more conflict noise… Sorry folks, the growing pains are real!

Rewards calculation for tBTCv2

Shadowfiend added 3 commits February 8, 2018 19:12

Add stub interface for broadcast channel.

8e03ba9

Add initial threshold group Member struct.

f0d4a6d

Includes a healthy dose of documentation, as well as a NewMember constructor function. There are still some open questions around how to handle the id, but this should be a good starting point.

Shadowfiend mentioned this pull request Feb 12, 2018

[WIP] Guard Tower: Initial interface and struct setups for distributed key generation #32

Merged

Shadowfiend added 13 commits February 12, 2018 15:13

Transition keep-network imports to github.com/keep-network/keep-core.

20e89f2

For now it's /keep-core/go/..., but we'll see if we come up with a different strategy.

Add constructor functions for broadcast and private messages

8042386

This starts the formalization of distinctions between these two. Eventually private messages will need to encrypt, for example.

Local channel sends messages in goroutines

143fc7b

This allows message sends to not block the caller, so that the caller can move on to receiving from other participants.

Make member BLS ID publicly accessible.

d9f78c7

There are a few places where we need it for filtering, though it's very likely these will be unnecessary if our underlying channels give us a way to not receive our own messages.

Set BLS id from a hex string in member instantiation

ddab772

Properly capture threshold in member constructor

871234a

We were forgetting to set it, so even though we took it into the function we never propagated it to the member. We'll be using this for better public key creation once the DKG process is done.

Provide access to member IDs excluding the current member

71365a1

OtherMemberIDs is just a convenient way to get the list of group member IDs other than a member's own ID, which we use when computing and shipping out private shares.

Add methods to know when we have all expected bits of a given type

c7d19b3

Without these methods we don't have a way externally of knowing that the member no longer needs to receive additional shares or commitments.

Don't accidentally accuse ourselves

fb3db6b

We were accidentally checking ourselves for received shares while deciding who to accuse. Of course we didn't receive shares for ourselves -- we simply have them available.

Fix combined commitment computation

fbacbbd

We were expecting to find this member's commitments in the commitments map, but they are tracked separately. We also change the combined commitments allocation to reference the threshold, since this is most directly correct.

Add two chain stubs, BeaconConfig and BlockCounter

c2f98a3

BeaconConfig is a stub for getting the current configuration of the relay beacon from the chain. BlockCounter is a stub for setting up notifications of when a certain number of blocks has been observed.

Set up main to call DKG with the configured group size and threshold

796d7ba

We use a local broadcast channel and a channel for reporting the complete members as they wrap up. No group signing yet though!

Shadowfiend force-pushed the guard-tower branch from 454ec7f to 796d7ba Compare February 12, 2018 23:09

Shadowfiend added 7 commits February 12, 2018 18:32

Move DKG into relay instead of its own package

c44a7c1

Not a huge deal, but don't want a million packages.

Merge remote-tracking branch 'origin/master' into guard-tower

88cacc2

Fix group secret key share creation

c0bf08e

We were expecting the group member's own share to be in the receivedShares map, but it was not. Instead, we have to query the member's secret shares to see the share it generated for itself. Then we add all the received shares.

Clarify group publicy key extraction

88b1d2a

No substantive change, but the explicit assignment makes it clear where the public key is coming from.

Whoops, fix reference to ExecuteDKG from dkg to relay package

070c753

Add group signature check to main

80f3aba

Notably, this doesn't do anything about thresholds, it does the whole thing.

Shadowfiend force-pushed the guard-tower branch from e2dbb37 to 80f3aba Compare February 13, 2018 00:34

Shadowfiend added 2 commits February 12, 2018 21:47

Move methods for Member to the end of member.go

832ebd4

The methods are defined in order from first used to last used during the DKG/relay process.

Switch the BLS config we're using to CurveFp254BNb

b35d30b

This is what we'll need to work with on-chain implementations, from our current understanding.

Shadowfiend changed the title ~~[WIP] Guard Tower: Initial interface and struct setups for distributed key generation~~ Guard Tower: Initial interface and struct setups for distributed key generation Feb 13, 2018

Fix Dockerfile for working with new imports

ef16b27

We're not removing anything from the Keep GitHub paths for now.

Shadowfiend force-pushed the guard-tower branch from 4bd5aea to ef16b27 Compare February 13, 2018 14:50

rargulati suggested changes Feb 13, 2018

View reviewed changes

Shadowfiend added 7 commits February 13, 2018 15:52

Add some documentation to chain.go

f77c47b

Rename localBlockCounter.heightMutex to structMutex

6ae1d0e

heightMutex implies the mutex locks only access to block height information, but in fact the mutex protects access to all struct members.

Cover edge cases more clearly when registring a new block waiter

93db326

Drop a spurious assignment

277962a

We append + assign further down in the function anyway.

Reduce scope of localBlockCounter mutex when incrementing block height

44fe4fc

We were wrapping the whole tick in the mutex, including waiter notification, but really we can read all the state we need quickly and notify waiters separately.

Notify waiters of a local block height increment in a goroutine

851d208

This most closely mimics waiting on individual clients, since each one will get its signal when it is ready for it. Without the goroutines, we find ourselves waiting for a slow waiter before notifying other waiters.

Burn some commented out code

48e6240

mhluongo approved these changes Feb 15, 2018

View reviewed changes

rargulati approved these changes Feb 16, 2018

View reviewed changes

Shadowfiend mentioned this pull request Feb 19, 2018

[WIP] Docker-based relay group initialization #39

Closed

10 tasks

Shadowfiend changed the title ~~Guard Tower: Initial interface and struct setups for distributed key generation~~ [WIP] Guard Tower: Initial interface and struct setups for distributed key generation Feb 20, 2018

Shadowfiend closed this Feb 20, 2018

Shadowfiend mentioned this pull request Feb 23, 2018

Reduxed with channel semaphore #44

Closed

Shadowfiend deleted the guard-tower branch May 17, 2018 14:50

dimpar pushed a commit that referenced this pull request Feb 10, 2023

Merge pull request #35 from threshold-network/tbtcv2-rewards

d41619d

Rewards calculation for tBTCv2

Uh oh!

Conversation

Shadowfiend commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shadowfiend commented Feb 12, 2018

Uh oh!

Shadowfiend commented Feb 13, 2018

Uh oh!

Shadowfiend commented Feb 13, 2018

Uh oh!

Shadowfiend commented Feb 13, 2018

Uh oh!

Shadowfiend commented Feb 13, 2018

Uh oh!

rargulati left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shadowfiend commented Feb 13, 2018

Uh oh!

mhluongo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rargulati left a comment

Choose a reason for hiding this comment

Shadowfiend commented Feb 12, 2018 •

edited

Loading

rargulati left a comment •

edited

Loading