Add ExecutedBlock type #1601
Add ExecutedBlock type #1601
Conversation
chain/block.go
Outdated
| @@ -26,6 +26,8 @@ import ( | |||
| "github.com/ava-labs/hypersdk/internal/workers" | |||
| "github.com/ava-labs/hypersdk/state" | |||
| "github.com/ava-labs/hypersdk/utils" | |||
|
|
|||
| publicfees "github.com/ava-labs/hypersdk/fees" | |||
There was a problem hiding this comment.
Can we just rename the existing internal/fees to internalfees instead of calling this publicfees?
chain/block.go
Outdated
| @@ -894,6 +896,79 @@ func (b *StatefulBlock) FeeManager() *fees.Manager { | |||
| return b.feeManager | |||
| } | |||
|
|
|||
| func (b *StatefulBlock) ExecutedBlock() *ExecutedBlock { | |||
There was a problem hiding this comment.
This might be a hot take... but I feel like ExecutedBlock being built off of a StatefulBlock looks weird to me because in my mind an ExecutedBlock is literally a copy of the state-representation of the block (i.e raw block data) but we're appending execution results onto it.
I think it would make sense to actually define the ExecutedBlock + it's creation in vm as vm.Block. It might seem like a strange place to put it but I would argue that it makes more sense there because processAcceptedBlock is defined there.
Because ExecutedBlock is also supposed to be a raw data type, should we also make this a value instead of a pointer?
There was a problem hiding this comment.
I thought about the alternative ordering as well ie. when you execute a stateful block it either produces or populates the fields on an ExecutedBlock type. Will take a stab at that and see what it looks like.
As for moving to vm.Block, I think we could definitely use a re-org of the chain/vm packages, but for now I think the new block type fits more naturally in the chain/ package.
re pointers vs. values, this is following the current style, so would prefer not to start making that change in this PR.
chain/block.go
Outdated
| Results []*Result `json:"results"` | ||
| UnitPrices publicfees.Dimensions `json:"unitPrices"` | ||
|
|
||
| blockBytes []byte |
There was a problem hiding this comment.
I don't think we need this field since we already have a reference to the underlying block and can call Bytes on it
There was a problem hiding this comment.
This just saves marshalling it an extra time. Will switch to doing the extra serialization
chain/block.go
Outdated
| UnitPrices publicfees.Dimensions `json:"unitPrices"` | ||
|
|
||
| blockBytes []byte | ||
| id ids.ID |
There was a problem hiding this comment.
I think we can export this can get rid of the corresponding ID() function
There was a problem hiding this comment.
Can un-embed StatelessBlock and switch to requiring the caller to go through that type
There was a problem hiding this comment.
This has the annoying side effect of needing to handle the error for both Marshal() and ID() since it's not stored on the type
chain/block.go
Outdated
| } | ||
|
|
||
| type ExecutedBlock struct { | ||
| *StatelessBlock `json:"block"` |
There was a problem hiding this comment.
Any reason for embedding this struct here? I personally dislike embedding unless we always want all exported fields/functions to be shared... this allows weird stuff like b.Size(). I personally don't have too much of an issue by copying struct fields.
| return writer.Bytes(), writer.Err() | ||
| } | ||
|
|
||
| func UnmarshalExecutedBlock(bytes []byte, parser Parser) (*ExecutedBlock, error) { |
There was a problem hiding this comment.
nit: should Unmarshal have a receiver on the struct too (or Marshal be static)? Seems weird that it's not consistent.
There was a problem hiding this comment.
Following current style here, happy to discuss for a separate change
There was a problem hiding this comment.
This is new code introduced as part of the diff... would it be reasonable to do it as part of this PR?
There was a problem hiding this comment.
All of the block marshalling / unmarshalling code is consistent with this change, so I'd rather not introduce a new style for one function.
There was a problem hiding this comment.
I'm also not sure moving it to a function on the pointer receiver is better. Either way, I don't think it belongs in this PR.
There was a problem hiding this comment.
What are changes in this file needed for?
There was a problem hiding this comment.
This was to support JSON encoding/decoding of the ExecutedBlock type, which includes this type now.
fees/dimension.go
Outdated
| @@ -104,9 +104,11 @@ func (d Dimensions) Greater(o Dimensions) bool { | |||
| return true | |||
| } | |||
|
|
|||
| const dimensionsFormatter = "(Bandwidth=%d, Compute=%d, Storage(Read)=%d, Storage(Allocate)=%d, Storage(Write)=%d)" | |||
There was a problem hiding this comment.
nit: move to top of file w/ other consts
| return writer.Bytes(), writer.Err() | ||
| } | ||
|
|
||
| func UnmarshalExecutedBlock(bytes []byte, parser Parser) (*ExecutedBlock, error) { |
There was a problem hiding this comment.
This is new code introduced as part of the diff... would it be reasonable to do it as part of this PR?
| return b.feeManager | ||
| } | ||
|
|
||
| type ExecutedBlock struct { | ||
| Block *StatelessBlock `json:"block"` |
There was a problem hiding this comment.
I would just prefer if we manually copied out the fields in the StatelessBlock type (Parent, Timestamp, Height, Txs, StateRoot). StatelessBlock has weird signatures (e.g Size is a redundant method because you can call len on the result of Marshal, and the call to ID() returns an error but we could handle that more cleanly by just initializing the ExecutedBlock with an ID) that I want to avoid propagating to upstream code.
This PR creates an
ExecutedBlocktype that includes theStatelessBlockand the execution results including both[]*chain.Resultand the unit prices iefee.Dimensions. This replaces passing aroundchain.StatefulBlockwith the new type.Addresses #1481 and #1595