make proposal assembly time configurable (#3165)

tsachiherman · web-flow · commit 168cfe58f1d9 · 2021-11-01T12:07:58.000-04:00
## Summary

This PR is doing the following:
1. Moves the proposal deadline computation outside of the agreement package and onto the node package. The agreement wasn't doing anything with the deadline anyway.
2. Make the proposal deadline be configured using a configuration parameter rather then a global hard-coded value in the config package.

## Test Plan

Existing unit test provides sufficient coverage for this change.
diff --git a/agreement/abstractions.go b/agreement/abstractions.go
@@ -19,7 +19,6 @@ package agreement
 import (
 	"context"
 	"errors"
-	"time"
 
 	"github.com/algorand/go-algorand/config"
 	"github.com/algorand/go-algorand/crypto"
@@ -74,10 +73,7 @@ var ErrAssembleBlockRoundStale = errors.New("requested round for AssembleBlock i
 // Round.
 type BlockFactory interface {
 	// AssembleBlock produces a new ValidatedBlock which is suitable for proposal
-	// at a given Round.  The time argument specifies a target deadline by
-	// which the block should be produced.  Specifically, the deadline can
-	// cause the factory to add fewer transactions to the block in question
-	// than might otherwise be possible.
+	// at a given Round.
 	//
 	// AssembleBlock should produce a ValidatedBlock for which the corresponding
 	// BlockValidator validates (i.e. for which BlockValidator.Validate
@@ -88,7 +84,7 @@ type BlockFactory interface {
 	// produce a ValidatedBlock for the given round. If an insufficient number of
 	// nodes on the network can assemble entries, the agreement protocol may
 	// lose liveness.
-	AssembleBlock(basics.Round, time.Time) (ValidatedBlock, error)
+	AssembleBlock(basics.Round) (ValidatedBlock, error)
 }
 
 // A Ledger represents the sequence of Entries agreed upon by the protocol.
diff --git a/agreement/agreementtest/simulate_test.go b/agreement/agreementtest/simulate_test.go
@@ -92,7 +92,7 @@ type testBlockFactory struct {
 	Owner int
 }
 
-func (f testBlockFactory) AssembleBlock(r basics.Round, deadline time.Time) (agreement.ValidatedBlock, error) {
+func (f testBlockFactory) AssembleBlock(r basics.Round) (agreement.ValidatedBlock, error) {
 	return testValidatedBlock{Inside: bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: r}}}, nil
 }
 
diff --git a/agreement/common_test.go b/agreement/common_test.go
@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"math/rand"
 	"testing"
-	"time"
 
 	"github.com/algorand/go-deadlock"
 	"github.com/stretchr/testify/require"
@@ -180,7 +179,7 @@ type testBlockFactory struct {
 	Owner int
 }
 
-func (f testBlockFactory) AssembleBlock(r basics.Round, deadline time.Time) (ValidatedBlock, error) {
+func (f testBlockFactory) AssembleBlock(r basics.Round) (ValidatedBlock, error) {
 	return testValidatedBlock{Inside: bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: r}}}, nil
 }
 
@@ -422,7 +421,7 @@ type testAccountData struct {
 }
 
 func makeProposalsTesting(accs testAccountData, round basics.Round, period period, factory BlockFactory, ledger Ledger) (ps []proposal, vs []vote) {
-	ve, err := factory.AssembleBlock(round, time.Now().Add(time.Minute))
+	ve, err := factory.AssembleBlock(round)
 	if err != nil {
 		logging.Base().Errorf("Could not generate a proposal for round %d: %v", round, err)
 		return nil, nil
@@ -534,7 +533,7 @@ func (v *voteMakerHelper) MakeRandomProposalValue() *proposalValue {
 
 func (v *voteMakerHelper) MakeRandomProposalPayload(t *testing.T, r round) (*proposal, *proposalValue) {
 	f := testBlockFactory{Owner: 1}
-	ve, err := f.AssembleBlock(r, time.Now().Add(time.Minute))
+	ve, err := f.AssembleBlock(r)
 	require.NoError(t, err)
 
 	var payload unauthenticatedProposal
diff --git a/agreement/fuzzer/ledger_test.go b/agreement/fuzzer/ledger_test.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"fmt"
 	"math/rand"
-	"time"
 
 	"github.com/algorand/go-algorand/agreement"
 	"github.com/algorand/go-algorand/config"
@@ -109,7 +108,7 @@ type testBlockFactory struct {
 	Owner int
 }
 
-func (f testBlockFactory) AssembleBlock(r basics.Round, deadline time.Time) (agreement.ValidatedBlock, error) {
+func (f testBlockFactory) AssembleBlock(r basics.Round) (agreement.ValidatedBlock, error) {
 	return testValidatedBlock{Inside: bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: r}}}, nil
 }
 
diff --git a/agreement/player_permutation_test.go b/agreement/player_permutation_test.go
@@ -19,7 +19,6 @@ package agreement
 import (
 	"fmt"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/require"
 
@@ -32,7 +31,7 @@ import (
 
 func makeRandomProposalPayload(r round) *proposal {
 	f := testBlockFactory{Owner: 1}
-	ve, _ := f.AssembleBlock(r, time.Time{})
+	ve, _ := f.AssembleBlock(r)
 
 	var payload unauthenticatedProposal
 	payload.Block = ve.Block()
diff --git a/agreement/proposalStore_test.go b/agreement/proposalStore_test.go
@@ -20,7 +20,6 @@ import (
 	"os"
 	"reflect"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/require"
 
@@ -65,7 +64,7 @@ func TestBlockAssemblerPipeline(t *testing.T) {
 
 	round := player.Round
 	period := player.Period
-	testBlockFactory, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	testBlockFactory, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", round, err)
 
 	accountIndex := 0
@@ -133,7 +132,7 @@ func TestBlockAssemblerBind(t *testing.T) {
 
 	player, _, accounts, factory, ledger := testSetup(0)
 
-	testBlockFactory, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	testBlockFactory, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", player.Round, err)
 
 	accountIndex := 0
@@ -201,7 +200,7 @@ func TestBlockAssemblerAuthenticator(t *testing.T) {
 
 	player, _, accounts, factory, ledger := testSetup(0)
 
-	testBlockFactory, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	testBlockFactory, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", player.Round, err)
 	accountIndex := 0
 	proposalPayload, _, _ := proposalForBlock(accounts.addresses[accountIndex], accounts.vrfs[accountIndex], testBlockFactory, player.Period, ledger)
@@ -267,7 +266,7 @@ func TestBlockAssemblerTrim(t *testing.T) {
 
 	player, _, accounts, factory, ledger := testSetup(0)
 
-	testBlockFactory, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	testBlockFactory, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", player.Round, err)
 	accountIndex := 0
 	proposalPayload, _, _ := proposalForBlock(accounts.addresses[accountIndex], accounts.vrfs[accountIndex], testBlockFactory, player.Period, ledger)
@@ -340,7 +339,7 @@ func TestProposalStoreT(t *testing.T) {
 
 	player, _, accounts, factory, ledger := testSetup(0)
 
-	testBlockFactory, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	testBlockFactory, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", player.Round, err)
 	accountIndex := 0
 	proposalPayload, proposalV, _ := proposalForBlock(accounts.addresses[accountIndex], accounts.vrfs[accountIndex], testBlockFactory, player.Period, ledger)
@@ -414,7 +413,7 @@ func TestProposalStoreUnderlying(t *testing.T) {
 
 	player, _, accounts, factory, ledger := testSetup(0)
 
-	testBlockFactory, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	testBlockFactory, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", player.Round, err)
 	accountIndex := 0
 	proposalPayload, proposalV, _ := proposalForBlock(accounts.addresses[accountIndex], accounts.vrfs[accountIndex], testBlockFactory, player.Period, ledger)
@@ -478,7 +477,7 @@ func TestProposalStoreHandle(t *testing.T) {
 
 	proposalVoteEventBatch, proposalPayloadEventBatch, _ := generateProposalEvents(t, player, accounts, factory, ledger)
 
-	testBlockFactory, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	testBlockFactory, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", player.Round, err)
 	accountIndex := 0
 	_, proposalV0, _ := proposalForBlock(accounts.addresses[accountIndex], accounts.vrfs[accountIndex], testBlockFactory, player.Period, ledger)
@@ -662,7 +661,7 @@ func TestProposalStoreGetPinnedValue(t *testing.T) {
 
 	// create proposal Store
 	player, router, accounts, factory, ledger := testPlayerSetup()
-	testBlockFactory, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	testBlockFactory, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", player.Round, err)
 	accountIndex := 0
 	// create a route handler for the proposal store
diff --git a/agreement/proposal_test.go b/agreement/proposal_test.go
@@ -20,7 +20,6 @@ import (
 	"context"
 	"os"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/require"
 
@@ -47,7 +46,7 @@ func testSetup(periodCount uint64) (player, rootRouter, testAccountData, testBlo
 }
 
 func createProposalsTesting(accs testAccountData, round basics.Round, period period, factory BlockFactory, ledger Ledger) (ps []proposal, vs []vote) {
-	ve, err := factory.AssembleBlock(round, time.Now().Add(time.Minute))
+	ve, err := factory.AssembleBlock(round)
 	if err != nil {
 		logging.Base().Errorf("Could not generate a proposal for round %d: %v", round, err)
 		return nil, nil
@@ -123,7 +122,7 @@ func TestProposalFunctions(t *testing.T) {
 	player, _, accs, factory, ledger := testSetup(0)
 	round := player.Round
 	period := player.Period
-	ve, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	ve, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", round, err)
 
 	validator := testBlockValidator{}
@@ -163,7 +162,7 @@ func TestProposalUnauthenticated(t *testing.T) {
 
 	round := player.Round
 	period := player.Period
-	testBlockFactory, err := factory.AssembleBlock(player.Round, time.Now().Add(time.Minute))
+	testBlockFactory, err := factory.AssembleBlock(player.Round)
 	require.NoError(t, err, "Could not generate a proposal for round %d: %v", round, err)
 
 	validator := testBlockValidator{}
diff --git a/agreement/pseudonode.go b/agreement/pseudonode.go
@@ -20,9 +20,7 @@ import (
 	"context"
 	"fmt"
 	"sync"
-	"time"
 
-	"github.com/algorand/go-algorand/config"
 	"github.com/algorand/go-algorand/data/account"
 	"github.com/algorand/go-algorand/data/basics"
 	"github.com/algorand/go-algorand/logging"
@@ -267,8 +265,7 @@ func (n asyncPseudonode) makePseudonodeVerifier(voteVerifier *AsyncVoteVerifier)
 
 // makeProposals creates a slice of block proposals for the given round and period.
 func (n asyncPseudonode) makeProposals(round basics.Round, period period, accounts []account.Participation) ([]proposal, []unauthenticatedVote) {
-	deadline := time.Now().Add(config.ProposalAssemblyTime)
-	ve, err := n.factory.AssembleBlock(round, deadline)
+	ve, err := n.factory.AssembleBlock(round)
 	if err != nil {
 		if err != ErrAssembleBlockRoundStale {
 			n.log.Errorf("pseudonode.makeProposals: could not generate a proposal for round %d: %v", round, err)
diff --git a/config/config.go b/config/config.go
@@ -23,7 +23,6 @@ import (
 	"os"
 	"os/user"
 	"path/filepath"
-	"time"
 
 	"github.com/algorand/go-algorand/protocol"
 	"github.com/algorand/go-algorand/util/codecs"
@@ -234,9 +233,6 @@ const (
 	dnssecTelemetryAddr
 )
 
-// ProposalAssemblyTime is the max amount of time to spend on generating a proposal block. This should eventually have it's own configurable value.
-const ProposalAssemblyTime time.Duration = 250 * time.Millisecond
-
 const (
 	catchupValidationModeCertificate                 = 1
 	catchupValidationModePaysetHash                  = 2
diff --git a/config/localTemplate.go b/config/localTemplate.go
@@ -41,7 +41,7 @@ type Local struct {
 	// Version tracks the current version of the defaults so we can migrate old -> new
 	// This is specifically important whenever we decide to change the default value
 	// for an existing parameter. This field tag must be updated any time we add a new version.
-	Version uint32 `version[0]:"0" version[1]:"1" version[2]:"2" version[3]:"3" version[4]:"4" version[5]:"5" version[6]:"6" version[7]:"7" version[8]:"8" version[9]:"9" version[10]:"10" version[11]:"11" version[12]:"12" version[13]:"13" version[14]:"14" version[15]:"15" version[16]:"16" version[17]:"17" version[18]:"18"`
+	Version uint32 `version[0]:"0" version[1]:"1" version[2]:"2" version[3]:"3" version[4]:"4" version[5]:"5" version[6]:"6" version[7]:"7" version[8]:"8" version[9]:"9" version[10]:"10" version[11]:"11" version[12]:"12" version[13]:"13" version[14]:"14" version[15]:"15" version[16]:"16" version[17]:"17" version[18]:"18" version[19]:"19"`
 
 	// environmental (may be overridden)
 	// When enabled, stores blocks indefinitally, otherwise, only the most recents blocks
@@ -417,6 +417,9 @@ type Local struct {
 	// message before it can be used for calculating the data exchange rate. Setting this to zero
 	// would use the default values. The threshold is defined in units of bytes.
 	TransactionSyncSignificantMessageThreshold uint64 `version[17]:"0"`
+
+	// ProposalAssemblyTime is the max amount of time to spend on generating a proposal block.
+	ProposalAssemblyTime time.Duration `version[19]:"250000000"`
 }
 
 // DNSBootstrapArray returns an array of one or more DNS Bootstrap identifiers
diff --git a/config/local_defaults.go b/config/local_defaults.go
@@ -20,7 +20,7 @@
 package config
 
 var defaultLocal = Local{
-	Version:                                    18,
+	Version:                                    19,
 	AccountUpdatesStatsInterval:                5000000000,
 	AccountsRebuildSynchronousMode:             1,
 	AnnounceParticipationKey:                   true,
@@ -94,6 +94,7 @@ var defaultLocal = Local{
 	PeerConnectionsUpdateInterval:              3600,
 	PeerPingPeriodSeconds:                      10,
 	PriorityPeers:                              map[string]bool{},
+	ProposalAssemblyTime:                       250000000,
 	PublicAddress:                              "",
 	ReconnectTime:                              60000000000,
 	ReservedFDs:                                256,
diff --git a/daemon/algod/api/server/v2/test/helpers.go b/daemon/algod/api/server/v2/test/helpers.go
@@ -21,7 +21,6 @@ import (
 	"math/rand"
 	"strconv"
 	"testing"
-	"time"
 
 	"github.com/algorand/go-algorand/agreement"
 	"github.com/algorand/go-algorand/config"
@@ -171,7 +170,7 @@ func (m mockNode) GetTransactionByID(txid transactions.Txid, rnd basics.Round) (
 	return node.TxnWithStatus{}, fmt.Errorf("get transaction by id not implemented")
 }
 
-func (m mockNode) AssembleBlock(round basics.Round, deadline time.Time) (agreement.ValidatedBlock, error) {
+func (m mockNode) AssembleBlock(round basics.Round) (agreement.ValidatedBlock, error) {
 	return nil, fmt.Errorf("assemble block not implemented")
 }
 
diff --git a/data/datatest/impls.go b/data/datatest/impls.go
@@ -19,7 +19,6 @@ package datatest
 import (
 	"context"
 	"fmt"
-	"time"
 
 	"github.com/algorand/go-algorand/agreement"
 	"github.com/algorand/go-algorand/config"
@@ -54,7 +53,7 @@ type entryFactoryImpl struct {
 }
 
 // AssembleBlock implements Ledger.AssembleBlock.
-func (i entryFactoryImpl) AssembleBlock(round basics.Round, deadline time.Time) (agreement.ValidatedBlock, error) {
+func (i entryFactoryImpl) AssembleBlock(round basics.Round) (agreement.ValidatedBlock, error) {
 	prev, err := i.l.BlockHdr(round - 1)
 	if err != nil {
 		return nil, fmt.Errorf("could not make proposals: could not read block from ledger at round %v: %v", round, err)
diff --git a/data/pools/transactionPool.go b/data/pools/transactionPool.go
@@ -106,6 +106,9 @@ type TransactionPool struct {
 	rememberedLatestLocal uint64
 
 	log logging.Logger
+
+	// proposalAssemblyTime is the ProposalAssemblyTime configured for this node.
+	proposalAssemblyTime time.Duration
 }
 
 // BlockEvaluator defines the block evaluator interface exposed by the ledger package.
@@ -134,6 +137,7 @@ func MakeTransactionPool(ledger *ledger.Ledger, cfg config.Local, log logging.Lo
 		logAssembleStats:     cfg.EnableAssembleStats,
 		expFeeFactor:         cfg.TxPoolExponentialIncreaseFactor,
 		txPoolMaxSize:        cfg.TxPoolSize,
+		proposalAssemblyTime: cfg.ProposalAssemblyTime,
 		log:                  log,
 	}
 	pool.cond.L = &pool.mu
@@ -824,7 +828,7 @@ func (pool *TransactionPool) recomputeBlockEvaluator(committedTxIds map[transact
 		// assembly. We want to figure out how long have we spent before trying to evaluate the first transaction.
 		// ( ideally it's near zero. The goal here is to see if we get to a near time-out situation before processing the
 		// first transaction group )
-		asmStats.TransactionsLoopStartTime = int64(firstTxnGrpTime.Sub(pool.assemblyDeadline.Add(-config.ProposalAssemblyTime)))
+		asmStats.TransactionsLoopStartTime = int64(firstTxnGrpTime.Sub(pool.assemblyDeadline.Add(-pool.proposalAssemblyTime)))
 	}
 
 	if !pool.assemblyResults.ok && pool.assemblyRound <= pool.pendingBlockEvaluator.Round() {
@@ -1064,6 +1068,6 @@ func (pool *TransactionPool) AssembleDevModeBlock() (assembled *ledgercore.Valid
 
 	// The above was already pregenerating the entire block,
 	// so there won't be any waiting on this call.
-	assembled, err = pool.AssembleBlock(pool.pendingBlockEvaluator.Round(), time.Now().Add(config.ProposalAssemblyTime))
+	assembled, err = pool.AssembleBlock(pool.pendingBlockEvaluator.Round(), time.Now().Add(pool.proposalAssemblyTime))
 	return
 }
diff --git a/installer/config.json.example b/installer/config.json.example
@@ -1,5 +1,5 @@
 {
-    "Version": 18,
+    "Version": 19,
     "AccountUpdatesStatsInterval": 5000000000,
     "AccountsRebuildSynchronousMode": 1,
     "AnnounceParticipationKey": true,
@@ -73,6 +73,7 @@
     "PeerConnectionsUpdateInterval": 3600,
     "PeerPingPeriodSeconds": 10,
     "PriorityPeers": {},
+    "ProposalAssemblyTime": 250000000,
     "PublicAddress": "",
     "ReconnectTime": 60000000000,
     "ReservedFDs": 256,
diff --git a/node/node.go b/node/node.go
@@ -1113,7 +1113,8 @@ func (vb validatedBlock) Block() bookkeeping.Block {
 }
 
 // AssembleBlock implements Ledger.AssembleBlock.
-func (node *AlgorandFullNode) AssembleBlock(round basics.Round, deadline time.Time) (agreement.ValidatedBlock, error) {
+func (node *AlgorandFullNode) AssembleBlock(round basics.Round) (agreement.ValidatedBlock, error) {
+	deadline := time.Now().Add(node.config.ProposalAssemblyTime)
 	lvb, err := node.transactionPool.AssembleBlock(round, deadline)
 	if err != nil {
 		if errors.Is(err, pools.ErrStaleBlockAssemblyRequest) {
diff --git a/test/testdata/configs/config-v19.json b/test/testdata/configs/config-v19.json

Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ type testBlockFactory struct {`
`92`	`92`	`Owner int`
`93`	`93`	`}`
`94`	`94`
`95`		`-func (f testBlockFactory) AssembleBlock(r basics.Round, deadline time.Time) (agreement.ValidatedBlock, error) {`
	`95`	`+func (f testBlockFactory) AssembleBlock(r basics.Round) (agreement.ValidatedBlock, error) {`
`96`	`96`	`return testValidatedBlock{Inside: bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: r}}}, nil`
`97`	`97`	`}`
`98`	`98`
Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,6 @@ import (`
`20`	`20`	`"context"`
`21`	`21`	`"fmt"`
`22`	`22`	`"math/rand"`
`23`		`- "time"`
`24`	`23`
`25`	`24`	`"github.com/algorand/go-algorand/agreement"`
`26`	`25`	`"github.com/algorand/go-algorand/config"`
`@@ -109,7 +108,7 @@ type testBlockFactory struct {`
`109`	`108`	`Owner int`
`110`	`109`	`}`
`111`	`110`
`112`		`-func (f testBlockFactory) AssembleBlock(r basics.Round, deadline time.Time) (agreement.ValidatedBlock, error) {`
	`111`	`+func (f testBlockFactory) AssembleBlock(r basics.Round) (agreement.ValidatedBlock, error) {`
`113`	`112`	`return testValidatedBlock{Inside: bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: r}}}, nil`
`114`	`113`	`}`
`115`	`114`