Interrupt pending transaction processing on block creation timeout #7673
Conversation
fab-10
changed the title
fab-10
changed the title
fab-10
force-pushed
the
interrupt-tx-execution-on-block-creation
branch
7 times, most recently
from
d47f1a1 to
9d190a2
Compare
| @@ -422,14 +466,16 @@ private TransactionSelectionResult handleTransactionNotSelected( | |||
| final var pendingTransaction = evaluationContext.getPendingTransaction(); | |||
|
|
|||
| // check if this tx took too much to evaluate, and in case it was invalid remove it from the | |||
| // pool, otherwise penalize it. | |||
| // pool, otherwise penalize it. Not synchronized since there is no state change here. | |||
| final TransactionSelectionResult actualResult = | |||
There was a problem hiding this comment.
This assignment becomes quite complex to read. Can we put this into a method with a clear name?
There was a problem hiding this comment.
yes will rework to make it easier to read
There was a problem hiding this comment.
I have rework it a bit, and added javadoc, hope it is clearer now
...n/java/org/hyperledger/besu/ethereum/blockcreation/txselection/BlockTransactionSelector.java
Outdated
Show resolved
Hide resolved
|
|
||
| internalBlockSelectionTimeoutSimulationInvalidTxs( | ||
| isPoa, | ||
| preProcessingTooLate, | ||
| processingTooLate, | ||
| postProcessingTooLate, | ||
| 900, | ||
| INVALID_TX_EVALUATION_TOO_LONG, |
There was a problem hiding this comment.
I think INVALID_TX_EVALUATION_TOO_LONG becomes missing from the tests. In which scenario we can receive this error?
There was a problem hiding this comment.
this is a very edge case, that could happen when the tx is invalid and complete the processing while at the same time the interrupt occurs, and it is not possible to cover without first making this test deterministic.
There was a problem hiding this comment.
From my examination of the code with the same config used to limit block building and transaction execution, I believe it can't happen at all. But it might make sense to separate these two in the future
|
|
||
| import org.apache.tuweni.bytes.Bytes; | ||
|
|
||
| public class InterruptibleOperationTracer implements BlockAwareOperationTracer { |
There was a problem hiding this comment.
Just curious, why some methods aren't interruptable still?
There was a problem hiding this comment.
I only check for interruption in the methods that are called during the execution of opcodes, since is there where the tx could take too much time, other methods should be more or less constant in time.
| @@ -1,7 +1,7 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
| <Configuration status="WARN"> | |||
| <Properties> | |||
| <Property name="root.log.level">INFO</Property> | |||
| <Property name="root.log.level">DEBUG</Property> | |||
There was a problem hiding this comment.
no, but could be useful to enable more logging for the unit tests in another PR
fab-10
force-pushed
the
interrupt-tx-execution-on-block-creation
branch
2 times, most recently
from
560ac8f to
5aabcb2
Compare
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
fab-10
force-pushed
the
interrupt-tx-execution-on-block-creation
branch
from
5aabcb2 to
c8bdf92
Compare
PR description
This PR continues on the track of making the block production more robust, in case some txs takes too much time to process, specifically it introduce a new feature, that forces the interruption of the tx that is still processing after the block creation timeout occurred.
To trigger the interruption, a special operation tracer is used, that wraps and delegate all the calls to the original operator tracer, but in case the thread that is processing the tx is interrupted, just stop the processing of the tx.
To avoid penalizing perfectly valid and timely txs, a grace time, equals to the block creation max time, is given to the tx that was processing when a block creation timeout occurs, before forcing the interrupt, so that we correctly penalize only txs that are taking more that time.
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests