Make transaction tracker oracle methods harder to misuse.#4465
Make transaction tracker oracle methods harder to misuse.#4465afck wants to merge 2 commits intolinera-io:mainfrom
Conversation
| OracleResponse::Http(response) => response, | ||
| _ => return Err(ExecutionError::OracleResponseMismatch), | ||
| } | ||
| let maybe_response = if self.txn_tracker.is_replaying() { |
There was a problem hiding this comment.
shouldn't this be a negation?
| let maybe_response = if self.txn_tracker.is_replaying() { | |
| let maybe_response = if !self.txn_tracker.is_replaying() { |
Previously, the next_replayed_oracle_response would return:
If not in replay mode,
Noneis returned,
There was a problem hiding this comment.
In the None case, i.e. in non-replay mode, we queried the actual oracle.
That's still the same: In non-replay mode we query the oracle (the else clause), but in replay mode we set this to None because we will obtain the actual value from the replaying responses below.
This PR just merges both calls so you can't forget one, i.e. we don't obtain the Some value in replay mode yet.
There was a problem hiding this comment.
This is more confusing than before IMO.
Previously, when next_replayed_oracle_response returned Some(Http(response)) we'd simply add that to add_oracle_response.
Now the add_oracle_response has new logic included that interprets None as "we should use a next entry from the oracle responses.
Maybe it'd be bit clearer if you didn't try to have single let response here and special case in the if { ... } and else { ... } clauses? Maybe
if self.txn_tracker.is_replaying() {
self.txn_tracker.replay(callback)
}There was a problem hiding this comment.
Yes, maybe it's not much of an improvement after all. Let's do the removal part first: #4467
It's not in all call sites as simple as putting the response in the callback, so I think what you're suggesting would basically be the approach from before.
The case I'm worried about is that in one of the many places where we use oracles we forget to add the response in one of the two cases (or both).
There was a problem hiding this comment.
Maybe if we mark the callback as must_use and then make the code so that the only thing you can do with it is push via self.transaction_tracker?
There was a problem hiding this comment.
Right, so replay either returns an oracle response and adds it to the outcomes, or it returns a must_use value that can only be used by later adding the response from the actual oracle to the outcomes.
Maybe I'll try that approach tomorrow.
| self.oracle_responses.push(recorded_response); | ||
| true | ||
| } else { | ||
| self.oracle_responses.push(oracle_response); |
There was a problem hiding this comment.
Why this split? Wasn't the previous version correct already?
|
Closing in favor of #4471. |
2 participants
Motivation
In the execution code, oracles must always be used as follows:
It's currently easy to forget one of those steps.
Proposal
Refactor the transaction tracker's methods to make it a bit harder to use them the wrong way.
Also remove a few unused execution requests. (We could make these crate-private, but in a separate PR.)
Test Plan
CI
Release Plan
Links
next_replayed_oracle_responsesafer to use. #3519.