feat: transaction filter in runtime config

[tdelabro]

No description provided.

[apoorvsadana]

I have some doubts in the design.

1. Does the current code check for BanInvokeV0TransactionRule? I couldn't see where its called.
2. It works if I call Validate::perform_pre_validation_stage::<BanInvokeV0TransactionRule>. If this is the expected behaviour, how will this work when we want to have multiple filters for Invoke transactions?

[tdelabro]

1. Does the current code check for `BanInvokeV0TransactionRule`? I couldn't see where its called.

No, because it wasn't the case before. But it could be used easily.

2. It works if I call `Validate::perform_pre_validation_stage::<BanInvokeV0TransactionRule>`. If this is the expected behaviour, how will this work when we want to have multiple filters for Invoke transactions?

pub trait TransactionFilter<T> {
    fn filter(transaction: &T) -> bool;
}

impl<T> TransactionFilter<T> for () {
    fn filter(_transaction: &T) -> bool {
        true
    }
}

pub struct BanInvokeV0TransactionRule;

impl TransactionFilter<InvokeTransaction> for BanInvokeV0TransactionRule {
    fn filter(transaction: &InvokeTransaction) -> bool {
        match transaction.tx {
            starknet_api::transaction::InvokeTransaction::V0(_) => false,
            _ => true,
        }
    }
}

pub struct BanTxFrom0x0TransactionRule;

impl TransactionFilter<InvokeTransaction> for BanTxFrom0x0TransactionRule {
    fn filter(transaction: &InvokeTransaction) -> bool {
        transaction.sender_address().0.0 != StarkFelt::ZERO
    }
}

pub struct MyFilterForInvokeTransaction;

impl TransactionFilter<InvokeTransaction> for MyFilterForInvokeTransaction {
    fn filter(transaction: &InvokeTransaction) -> bool {
        BanInvokeV0TransactionRule::filter(transaction) && BanTxFrom0x0TransactionRule::filter(transaction)
    }
}

[apoorvsadana]

Understood, this way we'll have one master filter for all transaction types. Do you think it makes sense to embed this master filter into each transaction (like explicitly state in code that there's ONE master filter)? Like have a trait called FilterableTransaction and implement it for each transaction type. The trait also has one method only called filter which calls all the TransactionFilters needed.

[tdelabro]

Understood, this way we'll have one master filter for all transaction types. Do you think it makes sense to embed this master filter into each transaction (like explicitly state in code that there's ONE master filter)?

I don't understand what you mean here

The way we execute tx require having a Filter type by TxType (Declare/Deploy/Invoke) if we want to use the trait.
Can you a quick snippet ?

[apoorvsadana]

Ya sure,

trait FilterableTransaction {
    fn filter(&self) -> bool;
}

impl FilterableTransaction for InvokeTransaction {
    fn filter(&self) -> bool {
        BanInvokeV0TransactionRule::filter(self)
    }
}

and inside perform_pre_validation_stage

assert!(self.filter(), "custom chain provided tx filter failed"); // calls the master filter

[tdelabro]

The problem is that the pallet config receives a type.
So we will need to do the following

#[pallet::config]
pub trait Config: frame_system::Config {
    type InvokeTransactionFilterType: TransactionFilter<InvokeTransaction>;

    #[pallet::constant]
    type InvokeTransactionFilter: Get<Self::InvokeTransactionFilter>;
}

And now you can do:

pub struct BlacklistedAddresses(HashSet<ContractAddress>)

impl TransactionFilter<InvokeTransaction> for BlacklistedAddresses {
    fn filter(transaction: &InvokeTransaction) -> bool {
        !self.0.contains(transaction.sender_address())
    }
}

Once again, in order to change the list of blacklisted addresses, you will have to run a runtime upgrade

[apoorvsadana]

I didn't understand this part

#[pallet::constant]
type InvokeTransactionFilter: Get<Self::InvokeTransactionFilter>;

should this be

type InvokeTransactionFilter: Get<BlacklistedAddresses>;

Also, is this contrary to the FilterableTransaction point I was saying or something else?

[tdelabro]

type InvokeTransactionFilter: Get;

Mean that the pallet (a lib) only accept one type of InvokeTransactionFilter, the BlacklistedAddresses. Because it is a lib we want it to accept anything that implement TransactionFilter<InvokeTransaction>, so that any user of the pallet can configure it's own filter.
The configuration is to be done in runtime crate.
The pallet define an interface, the runtime inject the dependency in it

[tdelabro]

@apoorvsadana @EvolveArt do you agree on this design?

[apoorvsadana]

Sorry for the late reply. Ya overall the design looks good to me and I think we should proceed. I still have a few questions

1. I agree we don't need to change the pallet code but what exactly is the advantage of that given all nodes need to upgrade their code, rebuild and restart. I am not against it, I am just trying to understand the mental model.
2. Should also have an AccountTransaction filter to filter account transactions in general.

[tdelabro]

@apoorvsadana

1. In terms of code management, it is simpler to just change a config file than the whole pallet. It also make sense in term of semantic versioning and responsibilities: the pallet still has the same exact features it doesn't need an upgrade, but the protocol is now different (while using the same exact pallet) so we do a runtime upgrade and upgrade it's version. The protocol one, not the pallet one.

2. It does not make sens given the way the execution is implemented. It exists at the TransactionType level. We don't call execute on AccoutTransaction

[apoorvsadana]

1. Understood
2. What about use cases like filtering out specific transaction versions, calldata lenght etc. which can be done at the AccountTransaction level in validate_unsigned?

[apoorvsadana]

so we don't add the rules here by default?

[tdelabro]

By default we want our Madara to be as extensive as possible: support all tx versions, all contract classes, etc.

Then other chains can use it to "do less", but we should still cover as much surface area as possible ourselves

[apoorvsadana]

Got it, although, by default do we not want it to replicate Starknet Mainent (no V0 txs)

[tdelabro]

Good question. I would say no on this specific point. But that's debatable.
In any case we made clear that Madara should not be used as it is and require a bit of customization