Repository navigation
smite-scenarios: panic on unreachable initial commitment errors - #293
Conversation
erickcestari
left a comment
erickcestari
left a comment
There was a problem hiding this comment.
Should we be able to opt out of an oracle? If a target accepts invalid params, this panic stops us from following it to funding_created, where it could per example crash the target or any other odd thing could happen.
| /// This should be unreachable since `AcceptChannelOracle` validates the | ||
| /// parameters before any negotiation is recorded. |
There was a problem hiding this comment.
nit: This isn't entirely true. A negotiation is recorded as soon as the unvalidated open_channel is sent.
3d73771 to
c253fe3
Compare
c253fe3 to
adbf2f4
Compare
Maybe? Ideally the known violations would get fixed relatively quickly and then we'd be able to fuzz past the oracle checks. But some violations may be low-priority for maintainers. If we do want to selectively disable oracles, I think that's a whole other challenge in itself and would require some design work. Currently we already rely on the oracles catching certain invalid states before we progress further, so we'd have to undo that. For this particular PR, it makes no difference whether the oracle is disabled or not -- if it is disabled then we hit the |
NishantBansal2003
left a comment
NishantBansal2003
left a comment
There was a problem hiding this comment.
Makes sense to remove this given the current design!
Needs rebase.
Should we be able to opt out of an oracle? If a target accepts invalid params, this panic stops us from following it to
funding_created, where it could per example crash the target or any other odd thing could happen.
I agree that the oracle might shadow some crash sites, though fixing these oracle violations might fix the underlying issues as well. Unless there's another way past the oracle that we would catch directly anyway, we don't lose anything on the crash/bug side. What we do lose is the ability to report some crashes.
If we want to go this route, one option could be to make oracle error checks apply only to the target, not smite. We could collect violations instead of returning immediately, handle errors explicitly in each state, and log the violations at the end, preferably after check_alive or ping_pong
Now that AcceptChannelOracle validates the channel parameters prior to saving the negotiation entry, it is impossible for new_initial_commitments() to fail. push_msat is always less than the funding amount, which is always less than MAX_MONEY. This also enables us to remove the final two tests that still use negotiation injection.
adbf2f4 to
a3713c9
Compare
Now that
AcceptChannelOraclevalidates the channel parameters prior to saving the negotiation entry, it is impossible fornew_initial_commitments()to fail.push_msatis always less than the funding amount, which is always less thanMAX_MONEY.This also enables us to remove the final two tests that still use negotiation injection.
Depends on #292.