go/runtime/client: Recheck failed blocks and implement max tx age

[ptrus]

Fixes: #3412

• runtime client remembers failed roothash blocks and rechecks them on every new received block
• additionally transactions that do not succeed in 1500 blocks are considered failed and client drops them
  • as a future improvement should the client also support SubmitTxNoWait operation (opened Add non-blocking runtime SubmitTx variant #3444)

[kostko]

should this be configurable?

Yeah probably.

as a future improvement should the client also support SubmitTxNoWait operation (similar to the one in consensus layer)?

Good idea, open an issue.

[codecov]

Codecov Report

Merging #3443 into master will increase coverage by 0.20%.
The diff coverage is 71.25%.

@@            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     

Impacted Files	Coverage Δ
go/runtime/client/watcher.go	72.59% <68.65%> (+4.81%)	⬆️
go/runtime/client/client.go	66.66% <83.33%> (+0.31%)	⬆️
go/oasis-node/cmd/node/node.go	53.65% <100.00%> (+0.11%)	⬆️
go/storage/mkvs/checkpoint/checkpointer.go	64.75% <0.00%> (-15.58%)	⬇️
.../consensus/tendermint/apps/epochtime_mock/state.go	75.00% <0.00%> (-5.00%)	⬇️
go/storage/mkvs/lookup.go	72.63% <0.00%> (-2.11%)	⬇️
go/oasis-node/cmd/node/control.go	61.11% <0.00%> (-1.39%)	⬇️
go/runtime/registry/registry.go	71.97% <0.00%> (-1.28%)	⬇️
go/consensus/tendermint/full/full.go	62.29% <0.00%> (-0.94%)	⬇️
go/worker/common/committee/group.go	75.45% <0.00%> (-0.91%)	⬇️
... and 20 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b12fefb...8a38e35. Read the comment docs.

[kostko]

Should there be a (configurable) limit on the size of this list? The question is what should happen once that limit is reached.

[ptrus]

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.

[kostko]

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.

[ptrus]

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.

[kostko]

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.

[ptrus]

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.

[ptrus]

I instead made checkBlock check if there's any pending transactions.

[Yawning]

While I appreciate the seasonal spirit, which.