smite-ir: split determinism out of has_side_effects - #198
Conversation
ef5f24c to
9b4dcf2
Compare
| | Self::LookupShortChannelId => true, | ||
| Self::LoadAmount(_) | ||
| | Self::BroadcastTransaction => true, | ||
| Self::LookupShortChannelId |
There was a problem hiding this comment.
Let's keep the match arms in the same order as the operator definitions. Applies throughout this file.
BTW, the LookupShortChannelId definition is currently under the "Act" header comment -- we should move it to its own new section.
There was a problem hiding this comment.
Done -- moved LookupShortChannelId under a new // -- Query: read state from outside the program -- section, and fixed the ordering throughout the file.
| /// CSE even when it is free of side effects, and its position in the | ||
| /// program is meaningful. | ||
| #[must_use] | ||
| pub fn is_deterministic(&self) -> bool { |
There was a problem hiding this comment.
I think deterministic actually isn't the right word. What we're checking is that the operation depends only on its inputs (not some external state).
Maybe depends_only_on_inputs would be better.
There was a problem hiding this comment.
Agreed, renamed to depends_only_on_inputs
| | Self::RecvChannelReady => false, | ||
| Self::SendMessage | ||
| | Self::SendOpenChannel | ||
| | Self::SendFundingCreated |
There was a problem hiding this comment.
SendFundingCreated actually depends on executor state, so it should be in the false category.
9b4dcf2 to
29cd0c2
Compare
LookupShortChannelId is a read-only operation, but it was marked as having side effects to stop CSE from merging two lookups whose results differ. Add depends_only_on_inputs and is_pure so CSE and the reorder mutator can ask about purity while DCE keeps asking about side effects.
29cd0c2 to
4dcce5e
Compare
2 participants
Follow-up to #155, where we agreed
LookupShortChannelIddoesn't really have side effects and that CSE wants a separate question answered.has_side_effectswas being asked three different questions by three callers: DCE wants to know whether an instruction can be dropped, CSE wants to know whether two instructions can be merged, andInstructionReorderMutatorwants to know whether an instruction's position matters. Those coincide for every operation we had untilLookupShortChannelId, which is read-only but reads chain state, so it was marked as side-effecting to keep CSE away from it.Add
depends_only_on_inputs, andis_pureon top of it, so each caller asks what it actually means. DCE keeps usinghas_side_effects, while CSE and the reorder mutator switch tois_pure.LookupShortChannelIdbecomes side-effect free, and it joinsCreateFundingTransaction,SendFundingCreated, and theRecvoperations in reading state beyond its inputs.The only behavior change is that DCE can now drop a
LookupShortChannelIdwhose result nothing consumes. I checked every operation, and it is the only one where!is_pure()differs fromhas_side_effects(), so CSE's skip set and the reorder mutator's candidate set are unchanged.Keeping the lookup reorderable matters: moving it across a
MineBlocksis what turns a confirmedshort_channel_idinto the unconfirmed sentinel, and it's the only way to reach the sentinel in a program whose scid comes from a lookup. Marking it pure would have silently removed that, so there's a test covering it along with tests for the CSE and DCE behavior.Ref: #155