introduce ELIGIBLE_FOR_FF flag on Spends via policy class

[arvidn]

when parsing and validating conditions. This indicates whether some of the requirements are met to be able to fast-forward a singleton spend.

This patch also introduces a customization class (ConditionPolicy) to hook into the parsing of conditions. the ELIGIBLE_FOR_DEDUP is moved out into the mempool-mode policy, and the new ELIGIBLE_FOR_FF is added to that same policy class.

The other policy is NonePolicy, which is used in consensus mode, which doesn't do any extra analysis of spends.

Given the simplicity of the two spend analyses (implemented in MempoolPolicy) it's not obvious to me that this abstraction and customization point carries its own weight. I think it's a net increase in complexity.

All the "business logic" for this patch is concentrated here: https://github.com/Chia-Network/chia_rs/pull/289/files#diff-f80b284fd6e11b143a06f56f735b48c7901525f11ff62bed4eb15fc1aa08a1ccR64-R138

The rest is the patch is plumbing and tests. All the tests are only covering the MempoolPolicy as the NonePolicy is empty.

[coveralls-official]

Pull Request Test Coverage Report for Build 6654683446

• 275 of 279 (98.57%) changed or added relevant lines in 7 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.2%) to 89.554%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
wheel/src/run_generator.rs	26	30	86.67%

Totals
Change from base Build 6638861137:	0.2%
Covered Lines:	9748
Relevant Lines:	10885

---

💛 - Coveralls

[richardkiss]

I think it can be simplified by only calling the policy thing one time, passing in all parameters including the list of conditions. Yeah, it would have to iterate over the conditions itself, but that doesn't seem like too big a deal.

As for the name Policy, I think of it more as an observer. For that reason, I think it's a bit odd to pass in the spend mutable. It also gives the observer a chance to mutate and invalidate spends (maybe you want this for the mempool though?). If you pass the spend's index, the observer can just keep a corresponding list of additional properties for each spend that it thinks relevant.

I do agree this would increase complexity further. The benefit would be further insulating the consensus. But maybe that's not necessary.

Do you plan to parameterize compilation on the policy (so compile-time changes to the execution path rather than runtime)?

[arvidn]

I think it can be simplified by only calling the policy thing one time, passing in all parameters including the list of conditions. Yeah, it would have to iterate over the conditions itself, but that doesn't seem like too big a deal.

I don't currently build a list of the conditions, they are coalesced and possibly ignored as they are parsed. Building this list just for the purpose of this customization point would, by far, outweigh the cost of the work being done to detect eligibility for dedup and fast-forward.

I can see an option where the "policy" parses the conditions itself, but it seems quite risky to do the parsing twice. There are a few risks in the parsing.

As for the name Policy, I think of it more as an observer. For that reason, I think it's a bit odd to pass in the spend mutable.

I agree that "Policy" is not a great name here. It is closer to an observer, at least if it would keep its state itself rather than updating the Spend. Doing that, though, would not only increase complexity further, but also (most likely) dwarf the cost of the work being done to detect these kinds of spends. Passing Spend as a mutable reference, to some extent, justifies calling the customization point "Policy", so does the fact that it's a template argument.

It also gives the observer a chance to mutate and invalidate spends (maybe you want this for the mempool though?). If you pass the spend's index, the observer can just keep a corresponding list of additional properties for each spend that it thinks relevant.

One possibility would be to pass in Spend as an immutable reference and have it return flags to set. It would insulate the main logic from the customization point's ability to mutate spends. I think this would be getting into the territory of trying to predict future uses of this interface at the expense of complexity. If we need to do more here in the future, I expect it to look completely different.

I do agree this would increase complexity further. The benefit would be further insulating the consensus. But maybe that's not necessary.

The insulation of the consensus-only logic from the mempool logic comes in the template argument itself. As long as the consensus-only version of this function uses the NonePolicy, it doesn't really matter what the policy is able to do, the consensus logic is still insulated. That said, the mempool logic also matters, and it's not OK for it to misbehave. This is partly why I think overall complexity probably trumps the separation of consensus-only vs. mempool logic.

Do you plan to parameterize compilation on the policy (so compile-time changes to the execution path rather than runtime)?

It is a template parameter already, that's partly why I named it "Policy", as in policy-based design.

At first I was planning to tweak the customization point as I described above, but as I was thinking about it, I'm not sure it's an improvement. I will try to come up with a better name than "Policy" and see if I can tweak it in some way to simplify it.

[richardkiss]

I think we need to pass in an instance of the thing that we're parameterizing on.

[arvidn]

I think it's not obvious what requirements this interface would have for implementing a mempool sniffer. In an ideal world it would also be possible to disable validation as well, and just parse the conditions.