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

[consensus observer] add new BlockTransactionPayload variants to prepare for block gas limit override on blocks #15848

Merged
merged 3 commits into from
Jan 31, 2025
Merged

[consensus observer] add new BlockTransactionPayload variants to prepare for block gas limit override on blocks #15848

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8aa965f
[consensus observer] add new BlockTransactionPayload variants to prep…
bchocho Jan 30, 2025
350aecf
remove unnecessary OptQuorumStoreV2
bchocho Jan 30, 2025
7627a20
Move the new BlockTransactionPayload to use enum TransactionsWithProof
bchocho Jan 30, 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
121 changes: 91 additions & 30 deletions consensus/src/consensus_observer/network/observer_message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ impl Display for ConsensusObserverDirectSend {
"BlockPayload: {}. Number of transactions: {}, limit: {:?}, proofs: {:?}",
block_payload.block,
block_payload.transaction_payload.transactions().len(),
block_payload.transaction_payload.limit(),
block_payload.transaction_payload.transaction_limit(),
block_payload.transaction_payload.payload_proofs(),
)
},
Expand Down Expand Up @@ -336,6 +336,38 @@ impl PayloadWithProof {
}
}

/// The transaction payload and proof of each block with a transaction and block gas limit
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub struct PayloadWithProofAndLimits {
payload_with_proof: PayloadWithProof,
transaction_limit: Option<u64>,
gas_limit: Option<u64>,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we make this an enum, so we just need to extend this in future if necessary?

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 was thinking this, but thought we probably don't need to extend it. Probably what we thought before too :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ibalajiarun changed to an enum. There's a lot of "similar-looking and sounding" structs in here now, that we'll need to clean up later

}

impl PayloadWithProofAndLimits {
pub fn new(
payload_with_proof: PayloadWithProof,
transaction_limit: Option<u64>,
gas_limit: Option<u64>,
) -> Self {
Self {
payload_with_proof,
transaction_limit,
gas_limit,
}
}

#[cfg(test)]
/// Returns an empty payload with proof and limit (for testing)
pub fn empty() -> Self {
Self {
payload_with_proof: PayloadWithProof::empty(),
transaction_limit: None,
gas_limit: None,
}
}
}

/// The transaction payload and proof of each block with a transaction limit
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub struct PayloadWithProofAndLimit {
Expand Down Expand Up @@ -364,13 +396,19 @@ impl PayloadWithProofAndLimit {
/// The transaction payload of each block
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)]
pub enum BlockTransactionPayload {
InQuorumStore(PayloadWithProof),
InQuorumStoreWithLimit(PayloadWithProofAndLimit),
// TODO: deprecate InQuorumStore* variants
DeprecatedInQuorumStore(PayloadWithProof),
DeprecatedInQuorumStoreWithLimit(PayloadWithProofAndLimit),
QuorumStoreInlineHybrid(PayloadWithProofAndLimit, Vec<BatchInfo>),
OptQuorumStore(
PayloadWithProofAndLimit,
/* OptQS and Inline Batches */ Vec<BatchInfo>,
),
QuorumStoreInlineHybridV2(PayloadWithProofAndLimits, Vec<BatchInfo>),
OptQuorumStoreV2(
PayloadWithProofAndLimits,
/* OptQS and Inline Batches */ Vec<BatchInfo>,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, we don't need a V2 for opt quorum store because we haven't turned it on yet?

}

impl BlockTransactionPayload {
Expand All @@ -380,7 +418,7 @@ impl BlockTransactionPayload {
proofs: Vec<ProofOfStore>,
) -> Self {
let payload_with_proof = PayloadWithProof::new(transactions, proofs);
Self::InQuorumStore(payload_with_proof)
Self::DeprecatedInQuorumStore(payload_with_proof)
}

/// Creates a returns a new InQuorumStoreWithLimit transaction payload
Expand All @@ -391,7 +429,7 @@ impl BlockTransactionPayload {
) -> Self {
let payload_with_proof = PayloadWithProof::new(transactions, proofs);
let proof_with_limit = PayloadWithProofAndLimit::new(payload_with_proof, limit);
Self::InQuorumStoreWithLimit(proof_with_limit)
Self::DeprecatedInQuorumStoreWithLimit(proof_with_limit)
}

/// Creates a returns a new QuorumStoreInlineHybrid transaction payload
Expand Down Expand Up @@ -426,37 +464,54 @@ impl BlockTransactionPayload {
/// Returns the list of inline batches and optimistic batches in the transaction payload
pub fn optqs_and_inline_batches(&self) -> &[BatchInfo] {
match self {
BlockTransactionPayload::QuorumStoreInlineHybrid(_, inline_batches) => inline_batches,
BlockTransactionPayload::OptQuorumStore(_, optqs_and_inline_batches) => {
optqs_and_inline_batches
},
_ => &[],
BlockTransactionPayload::DeprecatedInQuorumStore(_)
| BlockTransactionPayload::DeprecatedInQuorumStoreWithLimit(_) => &[],
BlockTransactionPayload::QuorumStoreInlineHybrid(_, inline_batches)
| BlockTransactionPayload::OptQuorumStore(_, inline_batches) => inline_batches,
BlockTransactionPayload::QuorumStoreInlineHybridV2(_, inline_batches)
| BlockTransactionPayload::OptQuorumStoreV2(_, inline_batches) => inline_batches,
}
}

/// Returns the limit of the transaction payload
pub fn limit(&self) -> Option<u64> {
/// Returns the transaction limit of the payload
pub fn transaction_limit(&self) -> Option<u64> {
match self {
BlockTransactionPayload::InQuorumStore(_) => None,
BlockTransactionPayload::InQuorumStoreWithLimit(payload) => payload.transaction_limit,
BlockTransactionPayload::QuorumStoreInlineHybrid(payload, _) => {
BlockTransactionPayload::DeprecatedInQuorumStore(_) => None,
BlockTransactionPayload::DeprecatedInQuorumStoreWithLimit(payload) => {
payload.transaction_limit
},
BlockTransactionPayload::OptQuorumStore(payload, _) => payload.transaction_limit,
BlockTransactionPayload::QuorumStoreInlineHybrid(payload, _)
| BlockTransactionPayload::OptQuorumStore(payload, _) => payload.transaction_limit,
BlockTransactionPayload::QuorumStoreInlineHybridV2(payload, _)
| BlockTransactionPayload::OptQuorumStoreV2(payload, _) => payload.transaction_limit,
}
}

/// Returns the block gas limit of the payload
pub fn gas_limit(&self) -> Option<u64> {
match self {
BlockTransactionPayload::DeprecatedInQuorumStore(_)
| BlockTransactionPayload::DeprecatedInQuorumStoreWithLimit(_)
| BlockTransactionPayload::QuorumStoreInlineHybrid(_, _)
| BlockTransactionPayload::OptQuorumStore(_, _) => None,
BlockTransactionPayload::QuorumStoreInlineHybridV2(payload, _)
| BlockTransactionPayload::OptQuorumStoreV2(payload, _) => payload.gas_limit,
}
}

/// Returns the proofs of the transaction payload
pub fn payload_proofs(&self) -> Vec<ProofOfStore> {
match self {
BlockTransactionPayload::InQuorumStore(payload) => payload.proofs.clone(),
BlockTransactionPayload::InQuorumStoreWithLimit(payload) => {
BlockTransactionPayload::DeprecatedInQuorumStore(payload) => payload.proofs.clone(),
BlockTransactionPayload::DeprecatedInQuorumStoreWithLimit(payload) => {
payload.payload_with_proof.proofs.clone()
},
BlockTransactionPayload::QuorumStoreInlineHybrid(payload, _) => {
BlockTransactionPayload::QuorumStoreInlineHybrid(payload, _)
| BlockTransactionPayload::OptQuorumStore(payload, _) => {
payload.payload_with_proof.proofs.clone()
},
BlockTransactionPayload::OptQuorumStore(payload, _) => {
BlockTransactionPayload::QuorumStoreInlineHybridV2(payload, _)
| BlockTransactionPayload::OptQuorumStoreV2(payload, _) => {
payload.payload_with_proof.proofs.clone()
},
}
Expand All @@ -465,14 +520,18 @@ impl BlockTransactionPayload {
/// Returns the transactions in the payload
pub fn transactions(&self) -> Vec<SignedTransaction> {
match self {
BlockTransactionPayload::InQuorumStore(payload) => payload.transactions.clone(),
BlockTransactionPayload::InQuorumStoreWithLimit(payload) => {
BlockTransactionPayload::DeprecatedInQuorumStore(payload) => {
payload.transactions.clone()
},
BlockTransactionPayload::DeprecatedInQuorumStoreWithLimit(payload) => {
payload.payload_with_proof.transactions.clone()
},
BlockTransactionPayload::QuorumStoreInlineHybrid(payload, _) => {
BlockTransactionPayload::QuorumStoreInlineHybrid(payload, _)
| BlockTransactionPayload::OptQuorumStore(payload, _) => {
payload.payload_with_proof.transactions.clone()
},
BlockTransactionPayload::OptQuorumStore(payload, _) => {
BlockTransactionPayload::QuorumStoreInlineHybridV2(payload, _)
| BlockTransactionPayload::OptQuorumStoreV2(payload, _) => {
payload.payload_with_proof.transactions.clone()
},
}
Expand Down Expand Up @@ -624,16 +683,18 @@ impl BlockTransactionPayload {
) -> Result<(), Error> {
// Get the payload limit
let limit = match self {
BlockTransactionPayload::InQuorumStoreWithLimit(payload) => payload.transaction_limit,
BlockTransactionPayload::QuorumStoreInlineHybrid(payload, _) => {
payload.transaction_limit
},
BlockTransactionPayload::OptQuorumStore(payload, _) => payload.transaction_limit,
_ => {
BlockTransactionPayload::DeprecatedInQuorumStore(_) => {
return Err(Error::InvalidMessageError(
"Transaction payload does not contain a limit!".to_string(),
))
},
BlockTransactionPayload::DeprecatedInQuorumStoreWithLimit(payload) => {
payload.transaction_limit
},
BlockTransactionPayload::QuorumStoreInlineHybrid(payload, _)
| BlockTransactionPayload::OptQuorumStore(payload, _) => payload.transaction_limit,
BlockTransactionPayload::QuorumStoreInlineHybridV2(payload, _)
| BlockTransactionPayload::OptQuorumStoreV2(payload, _) => payload.transaction_limit,
};

// Compare the expected limit against the payload limit
Expand Down
4 changes: 2 additions & 2 deletions consensus/src/payload_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,7 @@ impl TPayloadManager for QuorumStorePayloadManager {

Ok((
transaction_payload.transactions(),
transaction_payload.limit(),
transaction_payload.transaction_limit(),
))
}
}
Expand Down Expand Up @@ -554,7 +554,7 @@ async fn get_transactions_for_observer(
// Return the transactions and the transaction limit
Ok((
transaction_payload.transactions(),
transaction_payload.limit(),
transaction_payload.transaction_limit(),
))
}

Expand Down
Loading