PKI PR Feedback. (#3191)

Author: winder

agreement/abstractions.go

@@ -226,9 +226,9 @@ type KeyManager interface {
 	// keysRound.
 	VotingKeys(votingRound, keysRound basics.Round) []account.Participation
 
-	// RecordAsync indicates that the given participation action has been taken.
+	// Record indicates that the given participation action has been taken.
 	// The operation needs to be asynchronous to avoid impacting agreement.
-	RecordAsync(account basics.Address, round basics.Round, participationType account.ParticipationAction)
+	Record(account basics.Address, round basics.Round, participationType account.ParticipationAction)
 }
 
 // MessageHandle is an ID referring to a specific message.

agreement/agreementtest/keyManager.go

@@ -39,6 +39,6 @@ func (m SimpleKeyManager) VotingKeys(votingRound, _ basics.Round) []account.Part
 func (m SimpleKeyManager) DeleteOldKeys(r basics.Round) {
 }
 
-// RecordAsync implements KeyManager.RecordAsync.
-func (m SimpleKeyManager) RecordAsync(account basics.Address, round basics.Round, action account.ParticipationAction) {
+// Record implements KeyManager.Record.
+func (m SimpleKeyManager) Record(account basics.Address, round basics.Round, action account.ParticipationAction) {
 }

agreement/keyManager_test.go

@@ -17,7 +17,10 @@
 package agreement
 
 import (
+	"testing"
+
 	"github.com/algorand/go-deadlock"
+	"github.com/stretchr/testify/require"
 
 	"github.com/algorand/go-algorand/data/account"
 	"github.com/algorand/go-algorand/data/basics"
@@ -53,11 +56,19 @@ func (m *recordingKeyManager) DeleteOldKeys(r basics.Round) {
 }
 
 // Record implements KeyManager.Record.
-func (m *recordingKeyManager) RecordAsync(acct basics.Address, round basics.Round, action account.ParticipationAction) {
+func (m *recordingKeyManager) Record(acct basics.Address, round basics.Round, action account.ParticipationAction) {
 	m.mutex.Lock()
 	defer m.mutex.Unlock()
 	if _, ok := m.recording[acct]; !ok {
 		m.recording[acct] = make(map[account.ParticipationAction]basics.Round)
 	}
 	m.recording[acct][action] = round
 }
+
+// ValidateVoteRound requires that the given address voted on a particular round.
+func (m *recordingKeyManager) ValidateVoteRound(t *testing.T, address basics.Address, round basics.Round) {
+	m.mutex.Lock()
+	require.Equal(t, round, m.recording[address][account.Vote])
+	require.Equal(t, round, m.recording[address][account.BlockProposal])
+	m.mutex.Unlock()
+}

agreement/pseudonode.go

@@ -441,7 +441,7 @@ func (t pseudonodeVotesTask) execute(verifier *AsyncVoteVerifier, quit chan stru
 	for _, r := range verifiedResults {
 		select {
 		case t.out <- messageEvent{T: voteVerified, Input: r.message, Err: makeSerErr(r.err)}:
-			t.node.keys.RecordAsync(r.v.R.Sender, r.v.R.Round, account.Vote)
+			t.node.keys.Record(r.v.R.Sender, r.v.R.Round, account.Vote)
 		case <-quit:
 			return
 		case <-t.context.Done():
@@ -529,7 +529,7 @@ func (t pseudonodeProposalsTask) execute(verifier *AsyncVoteVerifier, quit chan
 	for _, r := range verifiedVotes {
 		select {
 		case t.out <- messageEvent{T: voteVerified, Input: r.message, Err: makeSerErr(r.err)}:
-			t.node.keys.RecordAsync(r.v.R.Sender, r.v.R.Round, account.BlockProposal)
+			t.node.keys.Record(r.v.R.Sender, r.v.R.Round, account.BlockProposal)
 		case <-quit:
 			return
 		case <-t.context.Done():

agreement/pseudonode_test.go

@@ -223,11 +223,8 @@ func TestPseudonode(t *testing.T) {
 		messageEvent, typeOk := ev.(messageEvent)
 		assert.True(t, true, typeOk)
 		// Verify votes are recorded - everyone is voting and proposing blocks.
-		keyManager.mutex.Lock()
-		assert.Equal(t, startRound, keyManager.recording[messageEvent.Input.Vote.R.Sender][account.Vote])
-		assert.Equal(t, startRound, keyManager.recording[messageEvent.Input.Vote.R.Sender][account.BlockProposal])
+		keyManager.ValidateVoteRound(t, messageEvent.Input.Vote.R.Sender, startRound)
 		events[messageEvent.t()] = append(events[messageEvent.t()], messageEvent)
-		keyManager.mutex.Unlock()
 	}
 	assert.Subset(t, []int{5, 6, 7, 8, 9, 10}, []int{len(events[voteVerified])})
 	assert.Equal(t, 0, len(events[payloadVerified]))
@@ -395,7 +392,7 @@ func (k *KeyManagerProxy) VotingKeys(votingRound, balanceRound basics.Round) []a
 	return k.target(votingRound, balanceRound)
 }
 
-func (k *KeyManagerProxy) RecordAsync(account basics.Address, round basics.Round, action account.ParticipationAction) {
+func (k *KeyManagerProxy) Record(account basics.Address, round basics.Round, action account.ParticipationAction) {
 }
 
 func TestPseudonodeLoadingOfParticipationKeys(t *testing.T) {

daemon/algod/api/server/v2/handlers.go

@@ -68,7 +68,7 @@ type NodeInterface interface {
 	StartCatchup(catchpoint string) error
 	AbortCatchup(catchpoint string) error
 	Config() config.Local
-	InstallParticipationKey(partKeyBinary *[]byte) (account.ParticipationID, error)
+	InstallParticipationKey(partKeyBinary []byte) (account.ParticipationID, error)
 	ListParticipationKeys() ([]account.ParticipationRecord, error)
 	GetParticipationKey(account.ParticipationID) (account.ParticipationRecord, error)
 	RemoveParticipationKey(account.ParticipationID) error
@@ -102,19 +102,19 @@ func convertParticipationRecord(record account.ParticipationRecord) generated.Pa
 	}
 
 	// Optional fields.
-	participationKey.EffectiveFirstValid = roundToPtrOrNil(record.EffectiveFirst)
+	if record.EffectiveLast != 0 && record.EffectiveFirst == 0 {
+		// Special case for first valid on round 0
+		zero := uint64(0)
+		participationKey.EffectiveFirstValid = &zero
+	} else {
+		participationKey.EffectiveFirstValid = roundToPtrOrNil(record.EffectiveFirst)
+	}
 	participationKey.EffectiveLastValid = roundToPtrOrNil(record.EffectiveLast)
 	participationKey.LastVote = roundToPtrOrNil(record.LastVote)
 	participationKey.LastBlockProposal = roundToPtrOrNil(record.LastBlockProposal)
 	participationKey.LastVote = roundToPtrOrNil(record.LastVote)
 	participationKey.LastStateProof = roundToPtrOrNil(record.LastStateProof)
 
-	// Special case for first valid on round 0
-	if record.EffectiveLast != 0 && record.EffectiveFirst == 0 {
-		zero := uint64(0)
-		participationKey.EffectiveFirstValid = &zero
-	}
-
 	return participationKey
 }
 
@@ -152,7 +152,7 @@ func (v2 *Handlers) AddParticipationKey(ctx echo.Context) error {
 		return badRequest(ctx, err, err.Error(), v2.Log)
 	}
 
-	partID, err := v2.Node.InstallParticipationKey(&partKeyBinary)
+	partID, err := v2.Node.InstallParticipationKey(partKeyBinary)
 
 	if err != nil {
 		return badRequest(ctx, err, err.Error(), v2.Log)
@@ -167,7 +167,7 @@ func (v2 *Handlers) AddParticipationKey(ctx echo.Context) error {
 // (DELETE /v2/participation/{participation-id})
 func (v2 *Handlers) DeleteParticipationKeyByID(ctx echo.Context, participationID string) error {
 
-	decodedParticipationID, err := account.ParticipationIDFromString(participationID)
+	decodedParticipationID, err := account.ParseParticipationID(participationID)
 
 	if err != nil {
 		return badRequest(ctx, err, err.Error(), v2.Log)
@@ -176,12 +176,11 @@ func (v2 *Handlers) DeleteParticipationKeyByID(ctx echo.Context, participationID
 	err = v2.Node.RemoveParticipationKey(decodedParticipationID)
 
 	if err != nil {
-
 		if errors.Is(err, account.ErrParticipationIDNotFound) {
-			return ctx.JSON(http.StatusOK, generated.ErrorResponse{Message: "participation id not found"})
+			return notFound(ctx, account.ErrParticipationIDNotFound, "participation id not found", v2.Log)
 		}
 
-		return badRequest(ctx, err, err.Error(), v2.Log)
+		return internalError(ctx, err, err.Error(), v2.Log)
 	}
 
 	return ctx.NoContent(http.StatusOK)
@@ -191,7 +190,7 @@ func (v2 *Handlers) DeleteParticipationKeyByID(ctx echo.Context, participationID
 // (GET /v2/participation/{participation-id})
 func (v2 *Handlers) GetParticipationKeyByID(ctx echo.Context, participationID string) error {
 
-	decodedParticipationID, err := account.ParticipationIDFromString(participationID)
+	decodedParticipationID, err := account.ParseParticipationID(participationID)
 
 	if err != nil {
 		return badRequest(ctx, err, err.Error(), v2.Log)
@@ -200,7 +199,11 @@ func (v2 *Handlers) GetParticipationKeyByID(ctx echo.Context, participationID st
 	participationRecord, err := v2.Node.GetParticipationKey(decodedParticipationID)
 
 	if err != nil {
-		return badRequest(ctx, err, err.Error(), v2.Log)
+		return internalError(ctx, err, err.Error(), v2.Log)
+	}
+
+	if participationRecord.IsZero() {
+		return notFound(ctx, account.ErrParticipationIDNotFound, account.ErrParticipationIDNotFound.Error(), v2.Log)
 	}
 
 	response := convertParticipationRecord(participationRecord)

daemon/algod/api/server/v2/test/helpers.go

@@ -86,7 +86,7 @@ type mockNode struct {
 	err       error
 }
 
-func (m mockNode) InstallParticipationKey(partKeyBinary *[]byte) (account.ParticipationID, error) {
+func (m mockNode) InstallParticipationKey(partKeyBinary []byte) (account.ParticipationID, error) {
 	panic("implement me")
 }