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.

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

multi fees per block (versioned) #1659

Closed
wants to merge 48 commits into from
Closed

Conversation

tbro
Copy link
Contributor

@tbro tbro commented Jul 1, 2024

Re-working multi-fees-per block to account for versioning (#1637).

types/src/v0/impls/state.rs Outdated Show resolved Hide resolved
@@ -592,6 +604,7 @@ impl ExplorerHeader<SeqTypes> for Header {
type ProposerId = FeeAccount;
type NamespaceId = NamespaceId;

// TODO what are these expected values w/ multiple Fees
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we determine proposer_id w/ multiple Fees? Can this be resolved in block explorer by propagating the Vec upward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ayiga for your reference.

Copy link
Member

Choose a reason for hiding this comment

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

What is the proposer id currently? Is it the HotShot leader id or the builder id?

Copy link
Contributor Author

@tbro tbro Jul 2, 2024

Choose a reason for hiding this comment

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

Here they are taking the account field of FeeInfo (builder account) for use as proposer id. Which I think makes sense in the case of the single fee (since that is the account paying the fee). With multiple fees not sure how this would work.

types/src/v0/impls/state.rs Outdated Show resolved Hide resolved
@tbro tbro requested a review from elliedavidson July 2, 2024 16:36
Comment on lines 79 to 276
fee_info: vec![fee_info],
builder_signature: vec![builder_signature],
Copy link
Member

Choose a reason for hiding this comment

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

Since we now have multiple builders, would it make sense to make the builder signature part of the fee info type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +361 to +393
pub fn fee_info(&self) -> Vec<FeeInfo> {
match self {
Self::V1(fields) => vec![fields.fee_info],
Self::V2(fields) => vec![fields.fee_info],
Self::V3(fields) => fields.fee_info.clone(),
}
Copy link
Member

Choose a reason for hiding this comment

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

We need some way to distinguish bid fees from sequencing fees, likely using an enum. I'm not sure where the best place to put this information is, but it needs to go somewhere :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this discussion addresses this.

@@ -592,6 +604,7 @@ impl ExplorerHeader<SeqTypes> for Header {
type ProposerId = FeeAccount;
type NamespaceId = NamespaceId;

// TODO what are these expected values w/ multiple Fees
Copy link
Member

Choose a reason for hiding this comment

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

What is the proposer id currently? Is it the HotShot leader id or the builder id?

types/src/v0/v0_1/state.rs Outdated Show resolved Hide resolved
tbro added 12 commits July 3, 2024 15:20
still need to update logic where FeeInfo is used in validation
revert the iterable `FeeInfo`. Currently the compiler is unaware of
header version in `validate_builder_fee` causing compile errors.
Note that upstream changes to BuilderSignature trait are required in
order for this to compile. Or, we *could* simply bypass the trait.
@tbro tbro force-pushed the tb/ver/multi-fees-per-block branch from ec7f564 to 0867660 Compare July 3, 2024 12:20
`Vec<FeeInfo>::amount()` will return `None` if any FeeAmount is out of
range or if the sum total of the amounts is out of range. So this
commit updates varient name and `Display` impl to better reflect that.
tbro added 7 commits July 8, 2024 18:23
Except arguments passed to builder signature (waiting on
hotshot). Also move `IterableFeeInfo` trait to types.
The absence of a builder signature can be represented as an
empty array.
Base automatically changed from ab/ver to main July 10, 2024 17:43
tbro added a commit that referenced this pull request Jul 11, 2024
@tbro tbro mentioned this pull request Jul 11, 2024
tbro added a commit that referenced this pull request Jul 11, 2024
@tbro
Copy link
Contributor Author

tbro commented Jul 11, 2024

Superseded by #1696

@tbro tbro closed this Jul 11, 2024
tbro added a commit that referenced this pull request Jul 17, 2024
@Ancient123 Ancient123 deleted the tb/ver/multi-fees-per-block branch August 29, 2024 15:39
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.

4 participants