Skip to content

Refactor bundle header#1496

Merged
liuchengxu merged 9 commits intomainfrom
refactor-bundle-Header
Jun 6, 2023
Merged

Refactor bundle header#1496
liuchengxu merged 9 commits intomainfrom
refactor-bundle-Header

Conversation

@liuchengxu
Copy link
Copy Markdown
Contributor

The main point in this PR is to move bundle_solution and signature in the previous SignedBundle into BundleHeader, no logical processing changes. Now we have PreliminaryBundleHeader containing everything in the corresponding BundleHeader but the signature, domain operator signs the hash of PreliminaryBundleHeader and then use the signature to create the final BundleHeader when producing a bundle.

Close #1485

Code contributor checklist:

`bundle_solution` and `signature` have been added into `BundleHeader`
since this commit, which means the proof of election can be verified
using only the header now, rendering the `SignedBundle` useless.
Since `BundleHeader` encompasses the bundle solution and signature now,
we can update the interface between the domain and consensus chain,
eliminating the usages of `SignedOpaqueBundle`.
They're useless and all the usages have been removed.
The sole difference between `PreliminaryBundleHeader` and `BundleHeader`
is the signature, similar to the typical post digests in Substrate block header.
Copy link
Copy Markdown
Contributor

@NingLin-P NingLin-P left a comment

Choose a reason for hiding this comment

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

Make sense overall!

@liuchengxu liuchengxu enabled auto-merge June 6, 2023 14:08
@liuchengxu liuchengxu merged commit 62d1095 into main Jun 6, 2023
@liuchengxu liuchengxu deleted the refactor-bundle-Header branch June 6, 2023 16:24
pub bundle_solution: BundleSolution<DomainHash>,
/// Signature of the bundle.
pub signature: ExecutorSignature,
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not nesting BundleHeader in SignedBundleHeader or SealedBundleHeader? This is a strange approach and the only reason for it I can come up with is such that some of existing code doesn't need to change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I hesitated to nest the BundleHeader because, in most cases, we primarily work with the SealBundleHeader instead of the BundleHeader. To facilitate working with the SealBundleHeader more conveniently, I opted to make the default assumption and enable direct usage of header.foo instead of sealed_header.header.foo.

The naming you suggested looks good to me though, I can rename them to BundleHeader and SealedBundleHeader.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can always do let header = sealed_header.header; and the rest of the code will be just like it was before. It is less pretty in my opinion that you need to construct a separate data structure to sign it, also during refactoring when new field is added you need to not forget to add it to the data structure that you sign. Just feels unnecessarily tricky.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The concern of missing a field during the refactoring makes sense, I'm not super satisfied with let header = sealed_header.header;, but happy to take this suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update Bundle Header Contents

4 participants