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

Implement Proposers Submitting Collations onto SMC #111

Merged
merged 19 commits into from Jun 10, 2018

Conversation

@terencechain
Copy link
Member

commented May 10, 2018

This is a PR referencing #106 to implement a proposer service/protocol that can be used to create collation, sign collation, check SMC header availability and add header to the main chain

Context

To align proposer implementation with minimal sharding protocol. Proposers will be responsible for create collation, sign collation, check collation header availability on the mainchain, add collation header to the mainchain via SMC, and broadcast collation body to the shard network it lives on.

Requirements for Merge

  • Create a Proposer struct/interface
  • Define common methods such as create collation, sign collation, check header hasn't been added in a given period and shard, add header
  • Pass all the tests for the methods below

Functions Being Implemented

func (p *Proposer) CreateCollation(node.Node, shardId *big.Int, period *big.Int, txs []*types.Transaction) (*sharding.Collation, error) {}
func (p *Proposer) CheckHeaderInSMC(shardID *big.Int, period *big.Int) (bool, error) {}
func (p *Proposer) AddHeaderToSMC(header *CollationHeader) (error) {}

@terencechain terencechain changed the title sharding: beginning the proposer design of phase1 Implement proposer propose collation onto SMC May 10, 2018

@terencechain terencechain self-assigned this May 10, 2018

@terencechain

This comment has been minimized.

Copy link
Member Author

commented May 18, 2018

The architecture of how proposer proposes collation will be based on #122

@rauljordan rauljordan changed the title Implement proposer propose collation onto SMC Implement Proposers Submitting Collations onto SMC May 23, 2018

@terencechain terencechain added this to To do in Collation Proposals via automation Jun 1, 2018

@terencechain terencechain added this to the Ruby milestone Jun 1, 2018

@@ -182,6 +183,13 @@ func (n *shardingNode) CreateTXOpts(value *big.Int) (*bind.TransactOpts, error)
}, nil
}

// Sign signs the hash of collationHeader contents by
// using default account on keystore and returns signed signature.
func (n *shardingNode) Sign(hash common.Hash) ([]byte, error) {

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 2, 2018

Member

Do we not have tests for this file?

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 4, 2018

Author Member

This will be addressed with #149

return nil
}

func (m *mockNode) Close() {

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 2, 2018

Member

Yikes, there are ~75 lines of mockNode and many of these are unused.
Is there a better way to do this?
Can we declare an interface for the proposer expected methods?

Take a look at effective go interface definition: https://golang.org/doc/effective_go.html#interfaces
Interfaces in Go provide a way to specify the behavior of an object: if something can do this, then it can be used here.

What this means to me: we could compose some interface to explain what a proposer-compatible node looks like.

type proposeNode interface {
   SMC // SMC interface that has SMCTransactor() and SMCCaller()
   Account() (Account, err)
   CreateTXOpts() (...)
}

This ~75 lines of setup with some methods that aren't used by this package make me think that we are not properly making use of interfaces. I.e. The consumer (sharding/proposer) should define the interface that it needs to properly function and the producer (sharding/node) simply provides a full struct.

Hopefully, that will reduce boilerplate on the test setup and provide clear isolation of responsibilities.

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 4, 2018

Author Member

This will be addressed with #149

t.Errorf("Create collation failed: %v", err)
}
if collation.Header().Period().Cmp(big.NewInt(5)) != 0 {
t.Errorf("Incorrect collationd period, want 5, got %v ", collation.Header().Period())

This comment has been minimized.

Copy link
@prestonvanloon
func TestCheckCollation(t *testing.T) {
backend, smc, err := setup()
if err != nil {
t.Fatalf("Failed to deploy SMC contract: %v", err)

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 2, 2018

Member

Rather than repeating this in every test, can you just pass t to setup() and check this within the setup?

t.Errorf("Can not check header availability: %v", err)
}
if a {
t.Errorf("Check header avability should return: %v", !a)

This comment has been minimized.

Copy link
@prestonvanloon
// normal test case 2: check if we can add header for period 2, should return true.
a, err = checkHeaderAvailability(node, big.NewInt(0), big.NewInt(2))
if !a {
t.Errorf("Check header avability should return: %v", !a)

This comment has been minimized.

Copy link
@prestonvanloon
if collationFromSMC.Proposer != node.Account().Address {
t.Errorf("Incorrect proposer address, got %v", *collation.ProposerAddress())
}
if common.BytesToHash(collationFromSMC.ChunkRoot[:]) != *collation.Header().ChunkRoot() {

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 2, 2018

Member

why do you need [:], what does that do?

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 2, 2018

Author Member

[:] converts byte[32] to byte[]

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 2, 2018

Member

why do you need it?

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 3, 2018

Author Member

BytesToHash takes byte[] mean while ChunkRoot in SMC is byte[32]. I need to convert byte[32] to byte[]

t.Errorf("Can not check header availability: %v", err)
}
if a {
t.Errorf("Check header avability should return: %v", !a)

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 2, 2018

Member

Also, we know that it should return a falsey value. Wouldn't it be better to report the actual value?

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 3, 2018

Author Member

Ah. Good catch!

}

var chunkRoot [32]byte
copy(chunkRoot[:], collation.Header().ChunkRoot().Bytes())

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 2, 2018

Member

Why do you need to make a copy here? Are you mutating it?

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 2, 2018

Author Member

Not mutating it. I want to convert []byte (what .Bytes() returns) to [32]byte (what SMC takes). Any better idea on how to do that?

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 2, 2018

Member

Does it work if you do this?

chunkRoot := collation.Header().ChunkRoot().Bytes()[:32]

This should reference the same array in-memory, so you aren't copying the data.

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 3, 2018

Author Member

That doesn't work. I really don't want to copy data. I haven't found a nice way to do it without copying

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 3, 2018

Member

What doesn't work about it? This should be a slice referencing the same internal array.

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 3, 2018

Author Member

It's still []byte not [32]byte. Is this what you are suggesting?

array :=[]byte{0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,31}
array32 := array[:32]
fmt.Printf("%T", array32)

I think we can either use the copy function or by assigning array elements one by one but all of these copy data...

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 4, 2018

Author Member

After more research, the Go language spec does not allow you to access "directly" the underlying, backing array of a slice. We can do that using package unsafe, but as its name hints: it's unsafe

var (
   s []byte    // slice
   a *[32]byte // pointer to array
)

s = make([]byte, 32)
a = byte32(s)
func byte32(s []byte) (a *[32]byte) {
    if len(a) <= len(s) {
        a = (*[len(a)]byte)(unsafe.Pointer(&s[0]))
    }
    return a
}
var chunkRoot [32]byte
copy(chunkRoot[:], collation.Header().ChunkRoot().Bytes())

_, err = n.SMCTransactor().AddHeader(txOps, collation.Header().ShardID(), collation.Header().Period(), chunkRoot)

This comment has been minimized.

Copy link
@prestonvanloon

prestonvanloon Jun 2, 2018

Member

Why are you throwing away the transaction? Did it succeed? Has it been mined?

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 2, 2018

Author Member

I'll add a check to make sure tx succeeds

// added to the main chain. There can only be one header per shard
// per period, proposer should check if anyone else has added the header.
// checkHeaderAvailability returns true if it's available, false if it's unavailable.
func checkHeaderAvailability(n node.Node, shardId *big.Int, period *big.Int) (bool, error) {

This comment has been minimized.

Copy link
@rauljordan

rauljordan Jun 2, 2018

Contributor

I don't like the name of this method - it seems like it has to do with the whole data availability problem so it's confusing. Let's change to something else.

This comment has been minimized.

Copy link
@terencechain

terencechain Jun 3, 2018

Author Member

You are right. Changed it to checkHeaderSubmitted

@rauljordan

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

With #149, we now have a simple entry point where we can add all the proposer goroutines. Make sure we leverage as much concurrency as we can for this within the Proposer.Start() function:

// Start the main loop for proposing collations.
func (p *Proposer) Start() error {
	log.Info("Starting proposer service")
	// TODO: Propose collations.
	return nil
}

Let's try to avoid the issue in the notary service as explained in #152.

@prestonvanloon

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

I have some issues with the copy requirement conversions from []byte to [32]byte.

Is this our code that’s enforcing one or the other?
I feel like it’s wasteful to copy, but it is literately only 32 bytes.

If it’s upstream code, let’s just be sure to comment on the slice operations to fixed size array and vice versa. Specifically, I mean to explain why we do it in a few words. Future readers (like forgetful Preston) will read this code and say “what were they thinking?” and a comment would satisfy this.

Thanks

@terencechain

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2018

Below is the code. ChunkRoot is always going to be a hash. The question is how do we convert hash to [32]byte (not []byte) without copying data. I haven't been able to find a good way. Any tips?

func (c *Collation) CalculateChunkRoot() {
	chunkRoot := common.BytesToHash(c.body)
	c.header.data.ChunkRoot = &chunkRoot
}
@prestonvanloon

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

If its easier to copy, let's just do that for now. My only ask is for a comment to explain why

@rauljordan

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

Looks good @terenc3t, just open an issue for the TODO

@rauljordan

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2018

@prestonvanloon can we get a quick look on this to approve?

@prestonvanloon
Copy link
Member

left a comment

LGTM, sorry to hold up merging this!

terencechain added 2 commits Jun 9, 2018

@terencechain terencechain merged commit 5791426 into prysmaticlabs:master Jun 10, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Collation Proposals automation moved this from To do to Done Jun 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants
You can’t perform that action at this time.