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.

Sign up for GitHub

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

agreement: split ValidatedBlock and AssembledBlock interfaces #5967

Closed
wants to merge 10 commits into from
Closed

agreement: split ValidatedBlock and AssembledBlock interfaces #5967

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
80b2721
change AssembleBlock to return a bookkeeping.Block
cce Mar 26, 2024
8ffc5cc
split ValidatedBlock and AssembledBlock interfaces
cce Mar 27, 2024
2b97b66
tweak comments
cce Mar 27, 2024
569a5c8
make linter happy
cce Mar 27, 2024
af2412d
fix comment
cce Mar 27, 2024
bdda2a5
split AssembledBlock/ProposableBlock
cce Apr 1, 2024
421fa47
make tests compile
cce Apr 1, 2024
09a8364
rename makeProposalFromAssembledBlock
cce Apr 2, 2024
d2745e5
update data/datatest BlockFactory impl
cce Apr 2, 2024
5a19b26
update BlockFactory comment
cce Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 25 additions & 11 deletions agreement/abstractions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,6 @@ type BlockValidator interface {
// and can now be recorded in the ledger. This is an optimized version of
// calling EnsureBlock() on the Ledger.
type ValidatedBlock interface {
// WithSeed creates a copy of this ValidatedBlock with its
// cryptographically random seed set to the given value.
//
// Calls to Seed() or to Digest() on the copy's Block must
// reflect the value of the new seed.
WithSeed(committee.Seed) ValidatedBlock

// Block returns the underlying block that has been validated.
Block() bookkeeping.Block
}
Expand All @@ -72,19 +65,40 @@ var ErrAssembleBlockRoundStale = errors.New("requested round for AssembleBlock i
// An BlockFactory produces an Block which is suitable for proposal for a given
// Round.
type BlockFactory interface {
// AssembleBlock produces a new ValidatedBlock which is suitable for proposal
// AssembleBlock produces a new AssembledBlock which is suitable for proposal
cce marked this conversation as resolved.
Show resolved Hide resolved
// at a given Round.
//
// AssembleBlock should produce a ValidatedBlock for which the corresponding
// AssembleBlock should produce a block for which the corresponding
// BlockValidator validates (i.e. for which BlockValidator.Validate
// returns true). If an insufficient number of nodes can assemble valid
// entries, the agreement protocol may lose liveness.
//
// AssembleBlock may return an error if the BlockFactory is unable to
// produce a ValidatedBlock for the given round. If an insufficient number of
// produce a AssembledBlock 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) (ValidatedBlock, error)
AssembleBlock(rnd basics.Round, partAddresses []basics.Address) (AssembledBlock, error)
}

// An AssembledBlock represents a Block produced by a BlockFactory
// and must be finalized before being proposed by agreement.
type AssembledBlock interface {
// WithSeed creates a copy of this AssembledBlock with its
// cryptographically random seed set to the given value.
//
// Calls to Seed() or to Digest() on the copy's Block must
// reflect the value of the new seed.
FinalizeBlock(seed committee.Seed, proposer basics.Address) ProposableBlock

Round() basics.Round
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need Round() now?

AssembledBlocks can't give out their Block()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is not to allow access to the AssembledBlock, it is an opaque type that should not be used for serializing or access until Finalize is called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making this more opaque also helped me catch some agreement tests & test helpers that were using the AssembleBlock() value without calling WithSeed before — those tests were ignoring and not validating the seed anyway, but it was good to find where they were.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does seem good that an assembledBlock will no longer implement ValidatedBlock.

}

// An ProposableBlock represents a Block produced by a BlockFactory,
// that was later finalized by providing the seed and the proposer,
// and can now be proposed by agreement.
type ProposableBlock interface {
// Block returns the underlying block that has been assembled.
Block() bookkeeping.Block
}

// A Ledger represents the sequence of Entries agreed upon by the protocol.
Expand Down
8 changes: 6 additions & 2 deletions agreement/agreementtest/simulate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,11 @@ func (b testValidatedBlock) Block() bookkeeping.Block {
return b.Inside
}

func (b testValidatedBlock) WithSeed(s committee.Seed) agreement.ValidatedBlock {
func (b testValidatedBlock) Round() basics.Round {
return b.Inside.Round()
}

func (b testValidatedBlock) FinalizeBlock(s committee.Seed, proposer basics.Address) agreement.ProposableBlock {
b.Inside.BlockHeader.Seed = s
return b
}
Expand All @@ -94,7 +98,7 @@ type testBlockFactory struct {
Owner int
}

func (f testBlockFactory) AssembleBlock(r basics.Round) (agreement.ValidatedBlock, error) {
func (f testBlockFactory) AssembleBlock(r basics.Round, _ []basics.Address) (agreement.AssembledBlock, error) {
return testValidatedBlock{Inside: bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: r}}}, nil
}

Expand Down
13 changes: 9 additions & 4 deletions agreement/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,11 @@ func (b testValidatedBlock) Block() bookkeeping.Block {
return b.Inside
}

func (b testValidatedBlock) WithSeed(s committee.Seed) ValidatedBlock {
func (b testValidatedBlock) Round() basics.Round {
return b.Inside.Round()
}

func (b testValidatedBlock) FinalizeBlock(s committee.Seed, _ basics.Address) ProposableBlock {
b.Inside.BlockHeader.Seed = s
return b
}
Expand All @@ -180,7 +184,7 @@ type testBlockFactory struct {
Owner int
}

func (f testBlockFactory) AssembleBlock(r basics.Round) (ValidatedBlock, error) {
func (f testBlockFactory) AssembleBlock(r basics.Round, _ []basics.Address) (AssembledBlock, error) {
return testValidatedBlock{Inside: bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: r}}}, nil
}

Expand Down Expand Up @@ -413,7 +417,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)
ve, err := factory.AssembleBlock(round, accs.addresses)
Copy link
Contributor

Choose a reason for hiding this comment

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

So the idea is that the underlying type is not going to be a Block, it's going to be something that holds onto extra information about those addresses (balances, presumably) so that a later call to FinalizeBlock can erase the payout if it won't leave the account above min balance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and whatever else you need to do (like updating the seed, assigning proposer etc) to finish/finalize the block

if err != nil {
logging.Base().Errorf("Could not generate a proposal for round %d: %v", round, err)
return nil, nil
Expand Down Expand Up @@ -525,8 +529,9 @@ 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)
ae, err := f.AssembleBlock(r, nil)
require.NoError(t, err)
ve := ae.FinalizeBlock(committee.Seed{}, basics.Address{})

var payload unauthenticatedProposal
payload.Block = ve.Block()
Expand Down
8 changes: 6 additions & 2 deletions agreement/fuzzer/ledger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ func (b testValidatedBlock) Block() bookkeeping.Block {
return b.Inside
}

func (b testValidatedBlock) WithSeed(s committee.Seed) agreement.ValidatedBlock {
func (b testValidatedBlock) Round() basics.Round {
return b.Inside.Round()
}

func (b testValidatedBlock) FinalizeBlock(s committee.Seed, _ basics.Address) agreement.ProposableBlock {
b.Inside.BlockHeader.Seed = s
return b
}
Expand All @@ -108,7 +112,7 @@ type testBlockFactory struct {
Owner int
}

func (f testBlockFactory) AssembleBlock(r basics.Round) (agreement.ValidatedBlock, error) {
func (f testBlockFactory) AssembleBlock(r basics.Round, _ []basics.Address) (agreement.AssembledBlock, error) {
return testValidatedBlock{Inside: bookkeeping.Block{BlockHeader: bookkeeping.BlockHeader{Round: r}}}, nil
}

Expand Down
4 changes: 3 additions & 1 deletion agreement/player_permutation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@ import (
"github.com/algorand/go-algorand/config"
"github.com/algorand/go-algorand/crypto"
"github.com/algorand/go-algorand/data/basics"
"github.com/algorand/go-algorand/data/committee"
"github.com/algorand/go-algorand/protocol"
"github.com/algorand/go-algorand/test/partitiontest"
)

func makeRandomProposalPayload(r round) *proposal {
f := testBlockFactory{Owner: 1}
ve, _ := f.AssembleBlock(r)
ae, _ := f.AssembleBlock(r, nil)
ve := ae.FinalizeBlock(committee.Seed{}, basics.Address{})

var payload unauthenticatedProposal
payload.Block = ve.Block()
Expand Down
34 changes: 24 additions & 10 deletions agreement/proposal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,14 +101,28 @@ type proposal struct {
validatedAt time.Duration
}

func makeProposal(ve ValidatedBlock, pf crypto.VrfProof, origPer period, origProp basics.Address) proposal {
// makeProposalFromAssembledBlock is called when making a new proposal message,
// using the output of AssembleBlock (from makeProposals -> proposalForBlock)
func makeProposalFromAssembledBlock(blk ProposableBlock, pf crypto.VrfProof, origPer period, origProp basics.Address) proposal {
e := blk.Block()
var payload unauthenticatedProposal
payload.Block = e
payload.SeedProof = pf
payload.OriginalPeriod = origPer
payload.OriginalProposer = origProp
return proposal{unauthenticatedProposal: payload}
}

// makeProposalFromValidatedBlock is called after successfully validating a proposal message,
// using the output of BlockValidator.Validate (from unauthenticatedProposal.validate)
func makeProposalFromValidatedBlock(ve ValidatedBlock, pf crypto.VrfProof, origPer period, origProp basics.Address) proposal {
e := ve.Block()
var payload unauthenticatedProposal
payload.Block = e
payload.SeedProof = pf
payload.OriginalPeriod = origPer
payload.OriginalProposer = origProp
return proposal{unauthenticatedProposal: payload, ve: ve}
return proposal{unauthenticatedProposal: payload, ve: ve} // store ve to use when calling Ledger.EnsureValidatedBlock
}

func (p proposal) u() unauthenticatedProposal {
Expand Down Expand Up @@ -244,22 +258,22 @@ func verifyNewSeed(p unauthenticatedProposal, ledger LedgerReader) error {
return nil
}

func proposalForBlock(address basics.Address, vrf *crypto.VRFSecrets, ve ValidatedBlock, period period, ledger LedgerReader) (proposal, proposalValue, error) {
rnd := ve.Block().Round()
func proposalForBlock(address basics.Address, vrf *crypto.VRFSecrets, blk AssembledBlock, period period, ledger LedgerReader) (proposal, proposalValue, error) {
rnd := blk.Round()
newSeed, seedProof, err := deriveNewSeed(address, vrf, rnd, period, ledger)
if err != nil {
return proposal{}, proposalValue{}, fmt.Errorf("proposalForBlock: could not derive new seed: %v", err)
}

ve = ve.WithSeed(newSeed)
proposal := makeProposal(ve, seedProof, period, address)
proposableBlock := blk.FinalizeBlock(newSeed, address)
prop := makeProposalFromAssembledBlock(proposableBlock, seedProof, period, address)
cce marked this conversation as resolved.
Show resolved Hide resolved
value := proposalValue{
OriginalPeriod: period,
OriginalProposer: address,
BlockDigest: proposal.Block.Digest(),
EncodingDigest: crypto.HashObj(proposal),
BlockDigest: prop.Block.Digest(),
EncodingDigest: crypto.HashObj(prop),
}
return proposal, value, nil
return prop, value, nil
}

// validate returns true if the proposal is valid.
Expand All @@ -282,5 +296,5 @@ func (p unauthenticatedProposal) validate(ctx context.Context, current round, le
return invalid, fmt.Errorf("EntryValidator rejected entry: %v", err)
}

return makeProposal(ve, p.SeedProof, p.OriginalPeriod, p.OriginalProposer), nil
return makeProposalFromValidatedBlock(ve, p.SeedProof, p.OriginalPeriod, p.OriginalProposer), nil
}
16 changes: 8 additions & 8 deletions agreement/proposalStore_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func TestBlockAssemblerPipeline(t *testing.T) {

round := player.Round
period := player.Period
testBlockFactory, err := factory.AssembleBlock(player.Round)
testBlockFactory, err := factory.AssembleBlock(player.Round, accounts.addresses)
require.NoError(t, err, "Could not generate a proposal for round %d: %v", round, err)

accountIndex := 0
Expand Down Expand Up @@ -132,7 +132,7 @@ func TestBlockAssemblerBind(t *testing.T) {

player, _, accounts, factory, ledger := testSetup(0)

testBlockFactory, err := factory.AssembleBlock(player.Round)
testBlockFactory, err := factory.AssembleBlock(player.Round, accounts.addresses)
require.NoError(t, err, "Could not generate a proposal for round %d: %v", player.Round, err)

accountIndex := 0
Expand Down Expand Up @@ -200,7 +200,7 @@ func TestBlockAssemblerAuthenticator(t *testing.T) {

player, _, accounts, factory, ledger := testSetup(0)

testBlockFactory, err := factory.AssembleBlock(player.Round)
testBlockFactory, err := factory.AssembleBlock(player.Round, accounts.addresses)
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)
Expand Down Expand Up @@ -266,7 +266,7 @@ func TestBlockAssemblerTrim(t *testing.T) {

player, _, accounts, factory, ledger := testSetup(0)

testBlockFactory, err := factory.AssembleBlock(player.Round)
testBlockFactory, err := factory.AssembleBlock(player.Round, accounts.addresses)
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)
Expand Down Expand Up @@ -339,7 +339,7 @@ func TestProposalStoreT(t *testing.T) {

player, _, accounts, factory, ledger := testSetup(0)

testBlockFactory, err := factory.AssembleBlock(player.Round)
testBlockFactory, err := factory.AssembleBlock(player.Round, accounts.addresses)
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)
Expand Down Expand Up @@ -413,7 +413,7 @@ func TestProposalStoreUnderlying(t *testing.T) {

player, _, accounts, factory, ledger := testSetup(0)

testBlockFactory, err := factory.AssembleBlock(player.Round)
testBlockFactory, err := factory.AssembleBlock(player.Round, accounts.addresses)
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)
Expand Down Expand Up @@ -477,7 +477,7 @@ func TestProposalStoreHandle(t *testing.T) {

proposalVoteEventBatch, proposalPayloadEventBatch, _ := generateProposalEvents(t, player, accounts, factory, ledger)

testBlockFactory, err := factory.AssembleBlock(player.Round)
testBlockFactory, err := factory.AssembleBlock(player.Round, accounts.addresses)
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)
Expand Down Expand Up @@ -661,7 +661,7 @@ func TestProposalStoreGetPinnedValue(t *testing.T) {

// create proposal Store
player, router, accounts, factory, ledger := testPlayerSetup()
testBlockFactory, err := factory.AssembleBlock(player.Round)
testBlockFactory, err := factory.AssembleBlock(player.Round, accounts.addresses)
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
Expand Down
6 changes: 3 additions & 3 deletions agreement/proposal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,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)
ve, err := factory.AssembleBlock(round, accs.addresses)
if err != nil {
logging.Base().Errorf("Could not generate a proposal for round %d: %v", round, err)
return nil, nil
Expand Down Expand Up @@ -122,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)
ve, err := factory.AssembleBlock(player.Round, accs.addresses)
require.NoError(t, err, "Could not generate a proposal for round %d: %v", round, err)

validator := testBlockValidator{}
Expand Down Expand Up @@ -162,7 +162,7 @@ func TestProposalUnauthenticated(t *testing.T) {

round := player.Round
period := player.Period
testBlockFactory, err := factory.AssembleBlock(player.Round)
testBlockFactory, err := factory.AssembleBlock(player.Round, accounts.addresses)
require.NoError(t, err, "Could not generate a proposal for round %d: %v", round, err)

validator := testBlockValidator{}
Expand Down
6 changes: 5 additions & 1 deletion agreement/pseudonode.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,11 @@ 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.ParticipationRecordForRound) ([]proposal, []unauthenticatedVote) {
ve, err := n.factory.AssembleBlock(round)
acctAddresses := make([]basics.Address, len(accounts))
for i := range accounts {
acctAddresses[i] = accounts[i].Account
}
ve, err := n.factory.AssembleBlock(round, acctAddresses)
if err != nil {
if err != ErrAssembleBlockRoundStale {
n.log.Errorf("pseudonode.makeProposals: could not generate a proposal for round %d: %v", round, err)
Expand Down
6 changes: 3 additions & 3 deletions data/datatest/impls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
}

// AssembleBlock implements Ledger.AssembleBlock.
func (i entryFactoryImpl) AssembleBlock(round basics.Round) (agreement.ValidatedBlock, error) {
func (i entryFactoryImpl) AssembleBlock(round basics.Round) (agreement.AssembledBlock, error) {
cce marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand All @@ -61,13 +61,13 @@

b := bookkeeping.MakeBlock(prev)
b.RewardsState = prev.RewardsState
return validatedBlock{blk: &b}, nil

Check failure on line 64 in data/datatest/impls.go

View workflow job for this annotation

GitHub Actions / Performance regression check 

cannot use validatedBlock{…} (value of type validatedBlock) as agreement.AssembledBlock value in return statement: validatedBlock does not implement agreement.AssembledBlock (missing method FinalizeBlock)

Check failure on line 64 in data/datatest/impls.go

View workflow job for this annotation

GitHub Actions / reviewdog-errors 

[Lint Errors] reported by reviewdog 🐶
cannot use validatedBlock{…} (value of type validatedBlock) as agreement.AssembledBlock value in return statement: validatedBlock does not implement agreement.AssembledBlock (missing method FinalizeBlock)

Raw Output:
data/datatest/impls.go:64:9: cannot use validatedBlock{…} (value of type validatedBlock) as agreement.AssembledBlock value in return statement: validatedBlock does not implement agreement.AssembledBlock (missing method FinalizeBlock)
}

// WithSeed implements the agreement.ValidatedBlock interface.
func (ve validatedBlock) WithSeed(s committee.Seed) agreement.ValidatedBlock {
// WithSeed implements the agreement.AssembledBlock interface.
func (ve validatedBlock) WithSeed(s committee.Seed) agreement.AssembledBlock {
cce marked this conversation as resolved.
Show resolved Hide resolved
newblock := ve.blk.WithSeed(s)
return validatedBlock{blk: &newblock}

Check failure on line 70 in data/datatest/impls.go

View workflow job for this annotation

GitHub Actions / Performance regression check 

cannot use validatedBlock{…} (value of type validatedBlock) as agreement.AssembledBlock value in return statement: validatedBlock does not implement agreement.AssembledBlock (missing method FinalizeBlock)

Check failure on line 70 in data/datatest/impls.go

View workflow job for this annotation

GitHub Actions / reviewdog-errors 

[Lint Errors] reported by reviewdog 🐶
cannot use validatedBlock{…} (value of type validatedBlock) as agreement.AssembledBlock value in return statement: validatedBlock does not implement agreement.AssembledBlock (missing method FinalizeBlock) (typecheck)

Raw Output:
data/datatest/impls.go:70:9: cannot use validatedBlock{…} (value of type validatedBlock) as agreement.AssembledBlock value in return statement: validatedBlock does not implement agreement.AssembledBlock (missing method FinalizeBlock) (typecheck)
// Copyright (C) 2019-2024 Algorand, Inc.
cce marked this conversation as resolved.
Show resolved Hide resolved
}

// Block implements the agreement.ValidatedBlock interface.
Expand Down
Loading
Loading