Added skipping of invalid transactions during block production #746
Conversation
Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
|
|
||
| // Execute each transaction. | ||
| for (idx, tx) in tx_iter { |
There was a problem hiding this comment.
why do we need to change this to a while loop with manually managed index incrementing? Is it just for skipping the first tx?
I'd prefer the iterator based approach to ensure we don't infinitely loop, or open the possibility of future refactoring bugs where the index isn't managed properly.
There was a problem hiding this comment.
It was done to be able to use swap_remove below
With an iterator, it is not possible. Alternative to the while loop, I can transform transactions into into_iterator, filter them, and after collect them into vector and set to the block. Or Vec::retain
There was a problem hiding this comment.
Hmm I don't think we'd want to swap_remove, as wouldn't that change the sorted order of the transactions and potentially impact the validity of the block?
For example, let's say you have 3 transactions (tx1, tx2, tx3):
- let tx3 depend on the output of tx2
- let tx1 be a faulty tx that needs to be skipped
If tx1 is swap_removed, the vec of transactions will now be [tx3, tx2]. When another node tries to sync this block, it will be invalid as tx3 depends on utxos that won't be in the state set yet. Worse, tx3 wont even be processed at all.
I made a simple playground to demonstrate how this can lead to pretty severely incorrect results on a larger set of txs where half of the txs are invalid: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=50c1cc2edb68d931ebd99aa3e2ce58bc
I'd rather the focus of the code be on correctness and clarity, vs optimizing array access and modification.
As a remedy, I'd prefer to lean more heavily on iterator style operations like FilterMap as opposed to imperatively mucking with the memory. Generally speaking, modifying lists in place while iterating over them is a recipe for bugs.
There was a problem hiding this comment.
Rust is also specially tuned to speed up iterators vs loops. It allows the compiler to do more inlining and other optimizations that aren't possible with side-effectful imperative looping styles: https://doc.rust-lang.org/book/ch13-04-performance.html
There was a problem hiding this comment.
Also, I don't think we'd want to increment the index if the tx was skipped. As the index is used for txpointers and other indexes where we want to record the total location of a tx within a block. If there are skipped txs and we increment the idx anyways, these indexes will point to incorrect locations in the block.
There was a problem hiding this comment.
In while we don't increment it for skipped transacitons=)
There was a problem hiding this comment.
The changes look better, although since the original bug wasn't caught in the original unit tests I think we should add new cases to cover the issue I uncovered above.
Co-authored-by: Brandon Kite <brandonkite92@gmail.com>
|
|
||
| // Execute each transaction. | ||
| for (idx, tx) in tx_iter { |
…' into bugfix/skip-invalid-transactions # Conflicts: # fuel-core/src/executor.rs
Added skipping of invalid transactions during block production with unit tests.
Close #738