SignatureCheckCondition can block transaction queue

[mversic]

At the moment, SignatureCheckCondition can evaluate to true or false.

Ideally, SignatureCheckCondition should evaluate to one of three states:

• passed
• failed (and waiting for additional requirements to be met on a transaction)
• impossible (no matter how long the peer waits this transaction can never pass)

Considering that we don't have the impossible state in the current implementation of condition check, it is possible for a malicious user to write such a condition check that can never be fulfilled and flood the Iroha queue with transactions consequently blocking the queue.

@Erigara in #3043 was aware of this so he implemented a two queue solution. There is one queue for transaction that have passed signature check (are valid and can be added to a block). The other queue is for transactions that have not passed signature check. 1st queue cannot be affected by this problem, but the 2nd can.

Even if SignatureCheckCondition was such that it had 3 states, the problem is that the malicious user can still write an incorrect condition and we're back to square one

Some of possible solutions would be:

1. formally verify the user given signature check is correct (I don't believe this is possible?)
2. simulate transaction execution to verify that it signature check condition can actually be fulfilled
3. don't give user the possibility to write any sort of signature check, rather offer a set of predefined signature checks that they can choose from and mint for their account

I believe 3rd is the most easy that we can offer in the short term and I don't believe that we need the full flexibility of custom defined signature check conditions. A set of predefined might be sufficient? How do we use the signature check except for supporting MSTs?

Also, if this problem is fixed, we can go back to using just one queue for transactions in Iroha. Two queues were the temporary preferred solution because of this problem

[mversic]

related to #2595

[Erigara]

To me problem is that not only malicious user can create incorrect signature condition, but that user can take createde by us multisignature (solution 3.) and leave it in failed (waiting for more) state on purpose which would result in flooding in any case.
So imo if we have any kind of transaction that is not immediately valid when arrive there is possibility to exploit this.

[mversic]

To me problem is that not only malicious user can create incorrect signature condition, but that user can take created by us multisignature (solution 3.) and leave it in failed (waiting for more) state on purpose which would result in flooding in any case. So imo if we have any kind of transaction that is not immediately valid when arrive there is possibility to exploit this.

I'm not sure I follow. If we have predefined signature checks, then I don't think user has influence over the verification process anymore. In any case the what 3rd solution is about is to make sure that user is removed from the verification process by only allowing him to choose from a set of predefined condition checks. If there are some additional requirements then they should be ensured at the time of setting the condition check, otherwise the Mint<SignatureConditionCheck> should fail.

[Erigara]

Suppose we have predefined M of N (N >= M > 1) multisignture signature check condition.
And some user intentionally send transaction with only 1 signature out of M, such transaction would stay in the queue until expired.

[mversic]

ok, I get you, but I am still confused. Maybe we should differentiate between account signatures and peer signatures? Account signatures should be satisfied at the time of initial submission, peer signatures can be added in due time during transaction gossiping. 1st case is either valid or invalid immediately, and the 2nd is no different than BFT

[Erigara]

This is all about account signatures. In current implementation account can have more than one signature and signature check condition compares set of account signatures with transaction's signatures.

[mversic]

ah, I see now. The important code is in the Queue::push

[mversic]

after some discussion in the chat I believe that we can agree the best course of action is to replace the current architecture of two queues with only one queue for all transactions (for both SST and MST) and a HashMap<AccountId, usize> that stores the number of MST in the queue per account? Alternatively, we can store number of all transactions per account. This is basically a primitive throttling mechanism

@Erigara you've been working on this previously, can you take this up when you are free to do so?

[Erigara]

I would suggest to store all transactions, because it will make code more simple without corner cases.
Also SST shouldn't be in the queue for a long time anyway.

[mversic]

I would suggest to store all transactions, because it will make code more simple without corner cases.
Also SST shouldn't be in the queue for a long time anyway.

if you are going to work on that, decision is up to you. But I agree

[Erigara]

if you are going to work on that

yes, i will

[appetrosyan]

Questions should be closed when consensus on the right approach is reached. This is an action item now, so I'm keeping it open for the time being.