Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimise (dom (delegs ▷ Set.singleton hk) ◁ stake) #1857

Merged
merged 3 commits into from Sep 16, 2020

Conversation

nc6
Copy link
Contributor

@nc6 nc6 commented Sep 15, 2020

Split apart from #1851 as the tests seem to be failing.

(dom (delegs ▷ Set.singleton hk) ◁ stake), motivated by intersectDomPLeft which efficiently computes
((dom rewards' ◁ delegs) ▷ dom poolParams). Altered the Set Algebra operators so that every singleton
set ends up in Exp as (SetSingleton x), rather than (Base SetR (Sett _)) or (Base SingleR (SetSingle _)),
this means we ony have a single pattern to match. See the smart constructors dRestrict, rRestrict etc.
Also added some property tests that test that intersectDomPLeft and  intersectDomP meet their specification.
TimSheard and others added 2 commits September 16, 2020 11:22
…compute.

compute (DRestrict (Dom (RRestrict (Base MapR delegs) (SetSingleton hk))) (Base MapR stake)) = ..
line 1022 of SetAlgebraInternals.hs   There are three ways to compute this case
 1) run (compile (dom (delegs ▷ Set.singleton hk) ◁ stake)))
 2) (intersectDomPLeft (\ _k v2 -> (v2==hk)) stake delegs))
 3) materialize MapR (do { (x,y,z) <- delegs 'domEq' stake; when ((y==hk)); one(x,z) }))
Only the first case was correct. When we made the function rRestrict be RRestrict, the
(SetSingleton hk) pattern failed and we fell through to the first case, which is the default case.
Once we fixed the code so case 2 and 3 was correct, everything ran fine, even when we reverted
rRestrict to its optimizing version. We also added some property tests checking that all 3 cases
were the same.
@uroboros
Copy link
Contributor

After @TimSheard 's bug fix on this PR - we were still getting "Insufficient Utxo Balance" errors in the Tx generator - this has been occasionally happening since I increased the trace lengths for "preservation of Ada" properties. Longer traces increase the chance of the Tx generators eventually "running out of utxo". To address this I've increased the genTxRetries constant from 6 -> 10.

@uroboros uroboros merged commit 156154d into master Sep 16, 2020
@iohk-bors iohk-bors bot deleted the ts/optimiseRewards branch September 16, 2020 09:45
@JaredCorduan
Copy link
Contributor

After @TimSheard 's bug fix on this PR - we were still getting "Insufficient Utxo Balance" errors in the Tx generator - this has been occasionally happening since I increased the trace lengths for "preservation of Ada" properties. Longer traces increase the chance of the Tx generators eventually "running out of utxo". To address this I've increased the genTxRetries constant from 6 -> 10.

@TimSheard actually has a fix for the "Insufficient Utxo Balance" as a part of #1815. The fix is a better coin selection algorithm in the trace generation. I think we should separate this fix as a standalone PR.

@uroboros uroboros restored the ts/optimiseRewards branch September 18, 2020 09:42
@nc6 nc6 deleted the ts/optimiseRewards branch January 11, 2022 08:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants