Relax Trait Bounds on TransactionPool::Transaction and EthPoolTransaction

[0xForerunner]

I'm working on the World Coin Block builder and ran into a problem implementing TransactionPool which currently requires

    PoolTransaction<
        Consensus = TransactionSignedEcRecovered,
        Pooled = PooledTransactionsElementEcRecovered
    >

which is unnecessarily restrictive. I've created a super trait which looks like this:

/// Super trait for transactions that can be converted to and from Eth transactions
pub trait EthPoolTransaction:
    PoolTransaction<
        Consensus: From<TransactionSignedEcRecovered> + Into<TransactionSignedEcRecovered>,
        Pooled: From<PooledTransactionsElementEcRecovered>
                    + Into<PooledTransactionsElementEcRecovered>,
    > + IntoRecoveredTransaction
{
    /// Extracts the blob sidecar from the transaction.
    fn take_blob(&mut self) -> EthBlobTransactionSidecar;

    /// Returns the number of blobs this transaction has.
    fn blob_count(&self) -> usize;

    /// Validates the blob sidecar of the transaction with the given settings.
    fn validate_blob(
        &self,
        blob: &BlobTransactionSidecar,
        settings: &KzgSettings,
    ) -> Result<(), BlobTransactionValidationError>;

    /// Returns the number of authorizations this transaction has.
    fn authorization_count(&self) -> usize;
}

which should be far more versatile for anyone else looking to implement the trait.

I was considering removing IntoRecoveredTransaction since it seems unnecessary since now you can simply call .into_consensus().into() in every case where you would have called .to_recovered_transaction But I decided to leave it since I wasn't sure if there were any other use cases for it. If you guys want to remove it we can do that in another PR.

Additionally perhaps we should consider renaming trait TransactionPool to trait EthTransactionPool to better illustrate it's bounds.

Would love your feedback, and looking forward to getting this merged in. Thanks!

[emhane]

honestly prefer if we leave this completely without trait bounds for now, following design in #10616 and #10997

smthg like

trait PoolTransaction {
    type Error;
    type BlobSideCar;
    etc.
}

associated types for all types used in trait methods except KzgSettings probably

pls change title of pr, it mentions the wrong trait

[0xForerunner]

@emhane I've named the PR because this is specifically the trait bound on TransactionPool I am attempting to relax

[emhane]

sure, that's not incorrect, though more helpful would be clarifying it's the trait bound on the associated type TransactionPool::Transaction in the title

[0xForerunner]

@emhane thanks so much for taking a look.

I'm not 100% sure what you're suggesting though. Are you saying to remove type Consensus from PoolTransaction entirely?

Or are you suggesting removing all bounds from EthPoolTransaction in favour of requiring Into/From on methods only where required?

I had a look at the 2 PRs you linked and I'm not entirely sure how they relate. Thanks for clarification!

[emhane]

Or are you suggesting removing all bounds from EthPoolTransaction in favour of requiring Into/From on methods only where required?

this :)

[emhane]

@mattsse tried to move away from those Into/From trait bounds recently, so don't think we want to reintroduce them now

imo worth exploring if it's possible to delay those trait bounds to where you need them and not have them on the trait definition

[mattsse]

supportive in general, luckily these bounds are now possible with stable rust 1.81

only have 1 q re the additional IntoRecoveredTransaction bound

@klkvr I think ideally we replace the EthPool restrictions to something like alloy::Transaction or similar

[mattsse]

relaxing this makes sense, until we have proper trait abstraction for primitives

[mattsse]

why do we need this now?

[0xForerunner]

We don't actually need it. I kept it there because the method from this trait was used and rather than using .into_consensus.into() I just left it as is. I can for sure get rid of it though!

[mattsse]

I'd like to remove this, don't think this is used

[0xForerunner]

@emhane @mattsse Thanks again for taking the time to review.

I think ideally we replace the EthPool restrictions to something like alloy::Transaction or similar

Sounds good to me. Assuming you're referencing this trait. I'll take a look into this.

imo worth exploring if it's possible to delay those trait bounds to where you need them and not have them on the trait definition

Okay perfect I'm going to begin work on this modification! Shouldn't take long!

[0xForerunner]

Quick update, @mattsse I took a look at alloy::consensus::Transaction but I don't think that quite solves the problem we have, perhaps I'm not quite understanding your suggestion. Anyhow, going to continue adding the trait bounds directly to the methods!

[0xForerunner]

@emhane @mattsse So after trying to apply these trait bounds directly to the methods for a couple hours I've found that it just kind of explodes. You end up with

where
    T: PoolTransaction<
        Consensus: From<TransactionSignedEcRecovered> + Into<TransactionSignedEcRecovered>,
        Pooled: From<PooledTransactionsElementEcRecovered>
                    + Into<PooledTransactionsElementEcRecovered>,
    > + IntoRecoveredTransaction

on a lot of function signatures. Personally I don't think this feels like the correct approach, but if you are pretty sure this is the direction you want to head then just let me know and i'll keep going.

I think since the current code essentially requires that these conversions are implemented it feels okay to have the trait also depend on it. In the future I think this could be made even more generic of transaction types, but that seems like the pretty serious overhall.

Let me know either way, thanks!

[mattsse]

I don't think we use this on main, so idk why we need this here

[emhane]

agreed.

(note: name of the trait should be ToRecoveredTransaction, ref https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv)

[0xForerunner]

Just to clarify @mattsse, did you want me to remove all traces of IntoRecoveredTransaction from the codebase? Or just remove this impl and the new trait bounds? Either one of these is possible.

[mattsse]

I'd like to remove this, don't think this is used

[mattsse]

@ewoolsey not worth looking into alloy::transaction rn,

the only thing I'd like to change here is removing the IntoRecoveredTransaction trait bound because I couldn't find any usage on EthPooledTransaction ref #11203

[emhane]

sure, that's not incorrect, though more helpful would be clarifying it's the trait bound on the associated type TransactionPool::Transaction in the title

[emhane]

agreed.

(note: name of the trait should be ToRecoveredTransaction, ref https://rust-lang.github.io/api-guidelines/naming.html#ad-hoc-conversions-follow-as_-to_-into_-conventions-c-conv)

[emhane]

Suggested change

	impl<T: PoolTransaction<Consensus: Into<TransactionSignedEcRecovered>>> IntoRecoveredTransaction
	impl<T> IntoRecoveredTransaction where T: PoolTransaction<Consensus: Into<TransactionSignedEcRecovered>>

we default to using the where clause syntax when the trait bound is more than one name or there is more than one generic

[0xForerunner]

@emhane @mattsse I've implemented the changes you've suggested! Specifically I've:

• Removed the bound on IntoRecoveredTransaction
• Renamed IntoRecoveredTransaction to ToRecoveredTransaction
• Switched to using where clause
• Merged from main

Things should be ready for your final review, and again thanks so much for taking a look!

[0xForerunner]

@emhane @mattsse just bumping this!

[mattsse]

thanks for the bump,
scheduling this for later today.

[0xForerunner]

Thanks so much all! have a great weekend!