What does this implement/fix? Explain your changes.
Optimist's blockProposedEventHandler writes transactions to dB without any check. There is at least one example why this is incorrect. If a bad client sends a duplicated transaction, this transaction may overwrite a previous transaction from a correct block, and a duplicated commitment will not be found anymore.
An example of this issue can be seen in optimist resync test (attached some log file with a summary of these events)
sync-test.txt
, where in the last subtest, a bad client/proposer submits a block including a duplicated transaction from the previous block. It can be seen that the optimist detects an incorrect block due to an incorrect frontier (i imagine that the bad proposer did not bother to make one correct). However, optimist overwrites the previously proposed transaction in the db, it proposes it again later in a different block with a correct frontier, and optimist cannot detect duplicated commitment because the original transaction was overwritten and therefore there is no duplicate found.
The idea is to store only those transactions that have not been successfully proposed earlier. For that, storeTransaction should check that successfully transactions are not overwritten. I am not too happy with the implementation suggested in this PR, as I hope it can be done in a single operation. However, i could only work out a two step operation:
Only insert new transactions.
Only update transactions with blockNumberL2 = -1
Alternatively, a different solution could be to have the optimist check for duplicated commitments from blocks rather than from transactions in dB, but this change is too advanced for me and i may be missing something in my analysis. For now, i am happy to show that there is a problem and the root cause is understood. Correct solution can come later
What does this implement/fix? Explain your changes.
Optimist's
blockProposedEventHandlerwrites transactions to dB without any check. There is at least one example why this is incorrect. If a bad client sends a duplicated transaction, this transaction may overwrite a previous transaction from a correct block, and a duplicated commitment will not be found anymore.An example of this issue can be seen in optimist resync test (attached some log file with a summary of these events) sync-test.txt , where in the last subtest, a bad client/proposer submits a block including a duplicated transaction from the previous block. It can be seen that the optimist detects an incorrect block due to an incorrect frontier (i imagine that the bad proposer did not bother to make one correct). However, optimist overwrites the previously proposed transaction in the db, it proposes it again later in a different block with a correct frontier, and optimist cannot detect duplicated commitment because the original transaction was overwritten and therefore there is no duplicate found.
The idea is to store only those transactions that have not been successfully proposed earlier. For that,
storeTransactionshould check that successfully transactions are not overwritten. I am not too happy with the implementation suggested in this PR, as I hope it can be done in a single operation. However, i could only work out a two step operation:Alternatively, a different solution could be to have the optimist check for duplicated commitments from blocks rather than from transactions in dB, but this change is too advanced for me and i may be missing something in my analysis. For now, i am happy to show that there is a problem and the root cause is understood. Correct solution can come later
Does this close any currently open issues?
What commands can I run to test the change?
Any other comments?