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

Startpost rpc #1312

Merged
merged 16 commits into from
Aug 15, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 58 additions & 10 deletions activation/activation.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/spacemeshos/go-spacemesh/common"
"github.com/spacemeshos/go-spacemesh/log"
"github.com/spacemeshos/go-spacemesh/types"
"sync"
"sync/atomic"
)

Expand Down Expand Up @@ -36,7 +37,7 @@ func (provider *PoETNumberOfTickProvider) NumOfTicks() uint64 {
type NipstBuilder interface {
BuildNIPST(challenge *common.Hash) (*types.NIPST, error)
IsPostInitialized() bool
InitializePost() (*types.PostProof, error)
InitializePost(logicalDrive string, commitmentSize uint64) (*types.PostProof, error)
}

type IdStore interface {
Expand All @@ -60,6 +61,12 @@ type BytesStore interface {
Get(key []byte) ([]byte, error)
}

const (
Idle = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

1 + iota ?

InProgress = 2
Done = 3
)

type Builder struct {
nodeId types.NodeId
coinbaseAccount address.Address
Expand All @@ -80,6 +87,9 @@ type Builder struct {
started uint32
store BytesStore
isSynced func() bool
accountLock sync.RWMutex
postInitLock sync.RWMutex
initStatus int
log log.Log
}

Expand All @@ -99,6 +109,7 @@ func NewBuilder(nodeId types.NodeId, coinbaseAccount address.Address, db ATXDBPr
finished: make(chan struct{}),
isSynced: isSyncedFunc,
store: store,
initStatus: Idle,
log: log,
}
}
Expand All @@ -119,14 +130,6 @@ func (b *Builder) Stop() {

// loop is the main loop that tries to create an atx per tick received from the global clock
func (b *Builder) loop() {
// post is initialized here, consider moving it to another location.
if !b.nipstBuilder.IsPostInitialized() {
_, err := b.nipstBuilder.InitializePost() // TODO: add proof to first ATX
if err != nil {
b.log.Error("PoST initialization failed: %v", err)
return
}
}
err := b.loadChallenge()
if err != nil {
log.Info("challenge not loaded: %s", err)
Expand All @@ -140,6 +143,13 @@ func (b *Builder) loop() {
b.log.Info("cannot create atx : not synced")
break
}
b.postInitLock.RLock()
initStat := b.initStatus
b.postInitLock.RUnlock()
if initStat != Done || !b.nipstBuilder.IsPostInitialized() {
moshababo marked this conversation as resolved.
Show resolved Hide resolved
b.log.Info("nipst is not initialized yet, not building nipst")
moshababo marked this conversation as resolved.
Show resolved Hide resolved
break
}
if b.working {
break
}
Expand Down Expand Up @@ -222,6 +232,44 @@ func (b *Builder) buildNipstChallenge(epoch types.EpochId) error {
return nil
}

func (b *Builder) StartPost(rewardAddress address.Address, logicalDrive string, commitmentSize uint64) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

logicalDrive is a bit misleading, because it's not merely the drive. What about datadir?

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of commitmentSize, perhaps space, since it's already what's used.

b.SetCoinbaseAccount(rewardAddress)

if !b.nipstBuilder.IsPostInitialized() {
Copy link
Contributor

Choose a reason for hiding this comment

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

isPostInitialized() implementation should be refactored. The state can be inspected via initializer.State().

Copy link
Contributor

Choose a reason for hiding this comment

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

If post is already initialized, it should return an error. There's no reason for the endpoint to continue swallowing these requests without reporting anything back.

b.postInitLock.Lock()
if b.initStatus != Idle {
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worth to check for InProgress status directly, instead of assuming that's the case if we're not on Idle && IsPostInitialized() returns false. Whatever you think.

b.postInitLock.Unlock()
return fmt.Errorf("attempted to start post when post already inited")
Copy link
Contributor

Choose a reason for hiding this comment

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

"inited" -> "initiated" ?

}
b.initStatus = InProgress
go func() {
_, err := b.nipstBuilder.InitializePost(logicalDrive, commitmentSize) // TODO: add proof to first ATX
b.postInitLock.Lock()
b.initStatus = Done
b.postInitLock.Unlock()
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're checking the err after already setting up the status to Done, but the err indicates that it didn't start/finish.

Also, I don't think we should settle on just logging the error, because the call is intended to be async.
Some errors can be returned immediately, so we should propagate and not merely log them.

b.log.Error("PoST initialization failed: %v", err)
return
}
}()
b.postInitLock.Unlock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for not doing the unlock before launching the goroutine?

}
return nil
}

func (b *Builder) SetCoinbaseAccount(rewardAddress address.Address) {
b.accountLock.Lock()
b.coinbaseAccount = rewardAddress
b.accountLock.Unlock()
}

func (b *Builder) getCoinbaseAccount() address.Address {
b.accountLock.RLock()
acc := b.coinbaseAccount
b.accountLock.RUnlock()
return acc
}

func (b Builder) getNipstKey() []byte {
return []byte("nipst")
}
Expand Down Expand Up @@ -326,7 +374,7 @@ func (b *Builder) PublishActivationTx(epoch types.EpochId) error {
pubEpoch, len(view), view)
}

atx := types.NewActivationTxWithChallenge(*b.challenge, b.coinbaseAccount, activeSetSize, view, b.nipst)
atx := types.NewActivationTxWithChallenge(*b.challenge, b.getCoinbaseAccount(), activeSetSize, view, b.nipst)

b.log.With().Info("active ids seen for epoch", log.Uint64("pos_atx_epoch", uint64(posEpoch)),
log.Uint32("view_cnt", activeSetSize))
Expand Down
43 changes: 37 additions & 6 deletions activation/activation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/stretchr/testify/require"
"sort"
"testing"
"time"
)

// ========== Vars / Consts ==========
Expand Down Expand Up @@ -71,15 +72,20 @@ func (n *NetMock) Broadcast(id string, d []byte) error {
}

type NipstBuilderMock struct {
poetRef []byte
buildNipstFunc func(challenge *common.Hash) (*types.NIPST, error)
poetRef []byte
buildNipstFunc func(challenge *common.Hash) (*types.NIPST, error)
SleepTime int
PostInitialized bool
}

func (np *NipstBuilderMock) IsPostInitialized() bool {
return true
return np.PostInitialized
}

func (np *NipstBuilderMock) InitializePost() (*types.PostProof, error) {
func (np *NipstBuilderMock) InitializePost(logicalDrive string, commitmentSize uint64) (*types.PostProof, error) {
if np.SleepTime != 0 {
time.Sleep(time.Duration(np.SleepTime) * time.Millisecond)
}
return nil, nil
}

Expand All @@ -97,7 +103,7 @@ func (np *NipstErrBuilderMock) IsPostInitialized() bool {
return true
}

func (np *NipstErrBuilderMock) InitializePost() (*types.PostProof, error) {
func (np *NipstErrBuilderMock) InitializePost(logicalDrive string, commitmentSize uint64) (*types.PostProof, error) {
return nil, nil
}

Expand Down Expand Up @@ -432,7 +438,7 @@ func TestBuilder_NipstPublishRecovery(t *testing.T) {
coinbase := address.HexToAddress("0xaaa")
net := &NetMock{}
layers := &MeshProviderMock{}
nipstBuilder := &NipstBuilderMock{}
nipstBuilder := &NipstBuilderMock{PostInitialized: true}
layersPerEpoch := uint16(10)
lg := log.NewDefault(id.Key[:5])
db := NewMockDB()
Expand Down Expand Up @@ -492,3 +498,28 @@ func TestBuilder_NipstPublishRecovery(t *testing.T) {
err = b.PublishActivationTx(3)
assert.True(t, db.hadNone)
}

func TestStartPost(t *testing.T) {
id := types.NodeId{"aaaaaa", []byte("bbbbb")}
coinbase := address.HexToAddress("0xaaa")
//net := &NetMock{}
layers := &MeshProviderMock{}
nipstBuilder := &NipstBuilderMock{PostInitialized: false}
layersPerEpoch := uint16(10)
lg := log.NewDefault(id.Key[:5])

drive := "/tmp/anton"
coinbase2 := address.HexToAddress("0xabb")
db := NewMockDB()
activationDb := NewActivationDb(database.NewMemDatabase(), &MockIdStore{}, mesh.NewMemMeshDB(lg.WithName("meshDB")), layersPerEpoch, &ValidatorMock{}, lg.WithName("atxDB1"))
b := NewBuilder(id, coinbase, activationDb, &FaultyNetMock{}, layers, layersPerEpoch, nipstBuilder, nil, func() bool { return true }, db, lg.WithName("atxBuilder"))

nipstBuilder.SleepTime = 10000
err := b.StartPost(coinbase2, drive, 1000)
assert.NoError(t, err)

// negative test to run
err = b.StartPost(coinbase2, drive, 1000)
assert.Error(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

It worth asserting the err msg, because it's not clear what failure you are testing here - an "InProgress" state or "Done" state.


}
8 changes: 8 additions & 0 deletions address/address.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ func BigToAddress(b *big.Int) Address { return BytesToAddress(b.Bytes()) }
// If s is larger than len(h), s will be cropped from the left.
func HexToAddress(s string) Address { return BytesToAddress(common.FromHex(s)) }

func StringToAddress(s string) (Address, error) {
bt, err := hex.DecodeString(s)
if err != nil {
return Address{}, err
}
return BytesToAddress(bt), nil
}

// Bytes gets the string representation of the underlying address.
func (a Address) Bytes() []byte { return a[:] }

Expand Down