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

[Malleability] Refactor BlockDigest and ChannelList types #6980

Merged
merged 3 commits into from
Feb 6, 2025
Merged

[Malleability] Refactor BlockDigest and ChannelList types #6980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b51c719
Removed unused ID method from ChannelList
UlyanaAndrukhiv Feb 4, 2025
4e8ab87
Refactor BlockDigest: rename id to BlockID, remove ID() method, and u…
UlyanaAndrukhiv Feb 4, 2025
128d875
Updated godoc for BlockDigest
UlyanaAndrukhiv Feb 6, 2025
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
2 changes: 1 addition & 1 deletion access/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,7 +1366,7 @@ func (h *Handler) SubscribeBlockDigestsFromLatest(request *access.SubscribeBlock
func (h *Handler) handleBlockDigestsResponse(send sendSubscribeBlockDigestsResponseFunc) func(*flow.BlockDigest) error {
return func(blockDigest *flow.BlockDigest) error {
err := send(&access.SubscribeBlockDigestsResponse{
BlockId: convert.IdentifierToMessage(blockDigest.ID()),
BlockId: convert.IdentifierToMessage(blockDigest.BlockID),
BlockHeight: blockDigest.Height,
BlockTimestamp: timestamppb.New(blockDigest.Timestamp),
})
Expand Down
2 changes: 1 addition & 1 deletion engine/access/rest/websockets/models/block_digest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (

// Build creates a BlockDigest instance with data from the provided flow.BlockDigest.
func (b *BlockDigest) Build(block *flow.BlockDigest) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this pattern is a bit of a bad practice. Specifically it assumes that we first create an object which is in "invalid" state and then call Build to initialize it. I am a big fan of programming idiom where we construct objects that hold their invariant. This means that we create objects in valid state at initialization.

Current usage of Build:

var block models.BlockDigest
block.Build(blockDigest)

proposed solution:

block := ConvertFromBlockDigest(blockDigest)

Additionally, I would consider getting rid of Build all together. It has several usages that seem to be redundant.

Copy link
Member

Choose a reason for hiding this comment

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

Fully agree with Yurii's guidance for the protocol-level: we highly value correctness and therefore should be embracing design patterns that are "safe by default" - in other words, patterns that are very hard to accidentally use them the wrong way.

Though, this "builder pattern" seems to be very broadly used only in the Access API, to convert flow-internal data types to json-encodable types that can be sent over REST, for example:

flow-go/engine/access/rest/common/models/block.go

Lines 30 to 31 in e027b13

var payload BlockPayload
err := payload.Build(block.Payload)

Given how prevalent this pattern is, I would be inclined to not make changes as part of the malleability work-stream. Within the malleability work-stream, I don't think we are going to be able to notably reduce the prevalence of this pattern ... but by partially "fixing" this pattern here and there, we would be making the code less consistent, which also has its downsides.

Nevertheless, please lets make sure we are not introducing this pattern into business logic of the lower-level protocol. I can live with it within the Access API, but not within the code guaranteeing correctness of the protocol overall. If we need a builder within the lower-level protocol, please make sure the builder is a distinct data structure that is different from the struct which the builder is producing. Then, there is no risk of accidentally conflating a builder in an interim incomplete state with a properly constructed output of the build process.

b.BlockId = block.ID().String()
b.BlockId = block.BlockID.String()
b.Height = util.FromUint(block.Height)
b.Timestamp = block.Timestamp
}
2 changes: 1 addition & 1 deletion engine/access/rpc/backend/backend_stream_block_digests_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func (s *BackendBlockDigestSuite) requireBlockDigests(v interface{}, expectedBlo
actualBlock, ok := v.(*flow.BlockDigest)
require.True(s.T(), ok, "unexpected response type: %T", v)

s.Require().Equal(expectedBlock.Header.ID(), actualBlock.ID())
s.Require().Equal(expectedBlock.Header.ID(), actualBlock.BlockID)
s.Require().Equal(expectedBlock.Header.Height, actualBlock.Height)
s.Require().Equal(expectedBlock.Header.Timestamp, actualBlock.Timestamp)
}
Expand Down
13 changes: 4 additions & 9 deletions model/flow/block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,27 +113,22 @@ func (b *CertifiedBlock) Height() uint64 {
return b.Block.Header.Height
}

// BlockDigest holds lightweight block information which includes only block id, block height and block timestamp
// BlockDigest holds lightweight block information which includes only the block's id, height and timestamp
type BlockDigest struct {
id Identifier
BlockID Identifier
Height uint64
Timestamp time.Time
}

// NewBlockDigest constructs a new block digest.
func NewBlockDigest(
id Identifier,
blockID Identifier,
height uint64,
timestamp time.Time,
) *BlockDigest {
return &BlockDigest{
id: id,
BlockID: blockID,
Height: height,
Timestamp: timestamp,
}
}

// ID returns the id of the BlockDigest.
func (b *BlockDigest) ID() Identifier {
return b.id
}
10 changes: 0 additions & 10 deletions network/channels/channel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@ package channels

import (
"regexp"
"sort"

"github.com/onflow/flow-go/model/flow"
)

// Channel specifies a virtual and isolated communication medium.
Expand Down Expand Up @@ -36,13 +33,6 @@ func (cl ChannelList) Swap(i, j int) {
cl[i], cl[j] = cl[j], cl[i]
}

// ID returns hash of the content of ChannelList. It first sorts the ChannelList and then takes its
// hash value.
func (cl ChannelList) ID() flow.Identifier {
sort.Sort(cl)
return flow.MakeID(cl)
}

// Contains returns true if the ChannelList contains the given channel.
func (cl ChannelList) Contains(channel Channel) bool {
for _, c := range cl {
Expand Down
Loading