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

NewPooledTransactionHashes uses incorrect size for blob transactions #4571

Closed
Rjected opened this issue Sep 12, 2023 · 1 comment · Fixed by #4577
Closed

NewPooledTransactionHashes uses incorrect size for blob transactions #4571

Rjected opened this issue Sep 12, 2023 · 1 comment · Fixed by #4577
Labels
A-devp2p Related to the Ethereum P2P protocol A-networking Related to networking in general A-tx-pool Related to the transaction mempool C-bug An unexpected or incorrect behavior

Comments

@Rjected
Copy link
Member

Rjected commented Sep 12, 2023

The NewPooledTransactionHashes message we broadcast uses the following method to populate the size associated with a transaction:

// transaction lists, before deciding whether or not to send full transactions to the
// peer.
for tx in to_propagate.iter() {
if peer.transactions.insert(tx.hash()) {
hashes.push(tx);
// Do not send full 4844 transaction hashes to peers.

with implementation:

fn push(&mut self, tx: &PropagateTransaction) {
match self {
PooledTransactionsHashesBuilder::Eth66(msg) => msg.0.push(tx.hash()),
PooledTransactionsHashesBuilder::Eth68(msg) => {
msg.hashes.push(tx.hash());
msg.sizes.push(tx.size);
msg.types.push(tx.transaction.tx_type().into());
}
}
}

This uses PropagateTransaction which only computes the size based on the TransactionSigned::length. This is not accurate for blob transactions, which must account for the length of the BlobSidecar as well, i.e. the PooledTransactionsElement length is more appropriate.

@Rjected Rjected added C-bug An unexpected or incorrect behavior A-devp2p Related to the Ethereum P2P protocol A-tx-pool Related to the transaction mempool A-networking Related to networking in general labels Sep 12, 2023
@mattsse
Copy link
Collaborator

mattsse commented Sep 12, 2023

nice find, this requires two fixes,

set accordingly when the tx is created with blob, and update length by fetching from blobstore during validation (re-injected transactions)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devp2p Related to the Ethereum P2P protocol A-networking Related to networking in general A-tx-pool Related to the transaction mempool C-bug An unexpected or incorrect behavior
Projects
Archived in project
2 participants