zoneconcierge/epoching: proof that a header is in an epoch #248
Conversation
| epoch := k.GetEpoch(ctx) | ||
| if !epoch.IsLastBlock(ctx) { | ||
| panic("RecordLastBlockHeader can only be invoked at the last block of an epoch") | ||
| return sdkerrors.Wrapf(types.ErrInvalidHeight, "RecordLastBlockHeader can only be invoked at the last block of an epoch") | ||
| } |
There was a problem hiding this comment.
As the name implies, the caller should ensure that it is this is the last block of the epoch. So maybe we don't need to check it here again. We can add a note about calling this function tho.
There was a problem hiding this comment.
Hmm yeah such a contract can do the job as well, but I feel checking it here again can be a better precaution for misuses. Now constructing the full epoch metadata becomes quite cryptic with the following steps:
- Upon the first block of epoch
i, create the epoch metadata with emptyLastBlockheader,AppHashRoot, andSealerHeader. - Upon the last block of epoch
i, fillLastBlockHeaderandAppHashRoot - Upon the 2nd block of epoch
i+1, fillSealerHeaderfor epochi
So I feel it might be better to impose as many restrictions to these functions as possible.
x/epoching/abci.go
Outdated
| if err := k.RecordLastBlockHeader(ctx); err != nil { | ||
| panic(err) | ||
| } | ||
| // record the Merkle root of all AppHashs in this epoch | ||
| if err := k.RecordAppHashRoot(ctx); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
It seems that the epoch info is stored twice in the two functions. Is it possible to combine the two into one like RecordEpochInfo?
There was a problem hiding this comment.
Yep that's a good idea. Merged them to RecordLastHeaderAndAppHashRoot so that we only write to DB once 👍
x/epoching/keeper/apphash_chain.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if height < epoch.FirstBlockHeight || uint64(epoch.LastBlockHeader.Height) < height { |
There was a problem hiding this comment.
| if height < epoch.FirstBlockHeight || uint64(epoch.LastBlockHeader.Height) < height { | |
| if height < epoch.FirstBlockHeight || height > uint64(epoch.LastBlockHeader.Height) { |
Same thing but seems more intuitive. To be cleaner, maybe we can add a method to Epoch like WithinBoundary(height uint64) bool
There was a problem hiding this comment.
The WithinBoundary is really a good idea! Done 👍
x/epoching/keeper/apphash_chain.go
Outdated
|
|
||
| unwrappedProof, err := merkle.ProofFromProto(proof) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
| return err | |
| return fmt.Errorf("failed to unwrap proof: %w", err) |
Just to be more informative
Fixes BM-302
This PR implements
ProofHeaderInEpoch, the proof that a header is in an epoch. The proof is constructed by a Merkle proof that the header'sAppHashis committed to the Merkle root of allAppHashs in the epoch. Along the way, this PR implements:AppHashat every height.AppHashs in an epoch, and stores the Merkle root to the epoch metadataAppHashis committed to the Merkle rootProofHeaderInEpochIn addition, since this PR modifies many Protobuf schemas, this PR performed
make proto-swagger-gento regenerate Swagger yml files.