go/runtime/client: Recheck failed blocks and implement max tx age #3443
Conversation
ptrus
force-pushed
the
ptrus/fix/runtime-client-txs
branch
from
c714768
to
a42e208
Compare
Yeah probably.
Good idea, open an issue. |
Codecov Report
@@ Coverage Diff @@
## master #3443 +/- ##
==========================================
+ Coverage 66.34% 66.54% +0.20%
==========================================
Files 371 371
Lines 33656 33710 +54
==========================================
+ Hits 22328 22432 +104
+ Misses 8067 8031 -36
+ Partials 3261 3247 -14
Continue to review full report at Codecov.
|
ptrus
force-pushed
the
ptrus/fix/runtime-client-txs
branch
16 times, most recently
from
168b924
to
c06666b
Compare
ptrus
requested review from
kostko,
peterjgilbert,
pro-wh and
Yawning
as code owners
| } | ||
| w.toBeChecked = failedBlocks |
There was a problem hiding this comment.
Should there be a (configurable) limit on the size of this list? The question is what should happen once that limit is reached.
There was a problem hiding this comment.
I'm not sure, if anything i think it might make more sense to have a limit on number of transactions being watched. I guess it wouldn't hurt at least logging a warning in case the failedBlocks is non-zero though.
There was a problem hiding this comment.
But this doesn't have anything to do with the number of watched transactions, right? There could be a single transaction and 1000 blocks waiting to be re-checked? And yeah logging sounds like a good idea.
There was a problem hiding this comment.
Yeah it's not related. I just think that it's more realistic that the number of transaction will grow big, rather than number of blocks needing to be rechecked, since in case there's actually problem with the storage nodes (the only reason checking blocks can currently fail), there probably won't be any new roothash blocks anyway. Unless the storage nodes are specifically blocking client requests.
There was a problem hiding this comment.
Oh one thing that would be useful is to clear this list in case there are no more pending transactions? Because it doesn't make sense to re-check blocks if all transactions have either completed or expired.
There was a problem hiding this comment.
agreed. Hm wondering if instead should just make checkBlock first check if there's any pending transactions and in case there aren't any just successfully return. Since it also doesn't make sense checking new blocks in case there aren't any pending transactions.
There was a problem hiding this comment.
I instead made checkBlock check if there's any pending transactions.
ptrus
force-pushed
the
ptrus/fix/runtime-client-txs
branch
2 times, most recently
from
082e5b5
to
62a3456
Compare
go/runtime/client/client.go
Outdated
| @@ -25,9 +29,20 @@ import ( | |||
| executor "github.com/oasisprotocol/oasis-core/go/worker/compute/executor/api" | |||
| ) | |||
|
|
|||
| const ( | |||
| // CfgMaxTransactionAge is the number of consensus blocks after witch | |||
There was a problem hiding this comment.
While I appreciate the seasonal spirit, which.
ptrus
force-pushed
the
ptrus/fix/runtime-client-txs
branch
from
62a3456
to
ea002b5
Compare
ptrus
force-pushed
the
ptrus/fix/runtime-client-txs
branch
from
ea002b5
to
c6caac5
Compare
ptrus
force-pushed
the
ptrus/fix/runtime-client-txs
branch
from
c6caac5
to
8a38e35
Compare
Fixes: #3412
SubmitTxNoWaitoperation (opened Add non-blocking runtime SubmitTx variant #3444)