feat: support period vote tx #305

troggy · 2019-08-01T12:01:10Z

Validators emit PeriodVote tx when it is time to submit period.
Validators collect period votes in a state

Resolves #282

codecov · 2019-08-01T12:08:22Z

Codecov Report

Merging #305 into master will decrease coverage by 0.22%.
The diff coverage is 92.18%.

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage   94.88%   94.66%   -0.23%     
==========================================
  Files          66       70       +4     
  Lines         958     1012      +54     
  Branches      177      189      +12     
==========================================
+ Hits          909      958      +49     
- Misses         44       48       +4     
- Partials        5        6       +1

Impacted Files	Coverage Δ
src/abis/operator.js	`100% <ø> (ø)`	⬆️
src/period/utils/checkEnoughVotes.js	`0% <0%> (ø)`
src/tx/applyTx/checkPeriodVote.js	`100% <100%> (ø)`
src/txHelpers/submitPeriod.js	`94.11% <100%> (+0.17%)`	⬆️
src/utils/buildCas.js	`100% <100%> (ø)`
src/period/submitPeriodVote.js	`100% <100%> (ø)`
src/period/index.js	`100% <100%> (ø)`	⬆️
src/tx/applyTx/utils.js	`98.55% <100%> (+0.06%)`	⬆️
src/block/updatePeriod.js	`95.65% <100%> (+0.41%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07f94b6...9644584. Read the comment docs.

johannbarbie · 2019-08-08T10:33:39Z

src/period/submitPeriodVote.js

+    bridgeState.account.privateKey
+  );
+
+  await sendDelayed(periodVoteTx);


why does this one need to be delayed? isn't tendermint deterministic? so if we got the period full, we are sure it will not go back?

I have no answer to this yet, just used the existing approach: https://github.com/leapdao/leap-node/blob/master/src/eventsRelay.js#L65

johannbarbie · 2019-08-08T11:02:48Z

src/tx/applyTx/checkPeriodVote.js

+
+  // validators are not in consensus on period roots atm
+  // disabling this until resolved
+  // https://github.com/leapdao/leap-node/issues/282#issuecomment-519160276


issue solved, should this be reenabled?

it is still not working right. I'm fixing it as part of #309 here: https://github.com/leapdao/leap-node/tree/feat/submit-with-cas

TLDR: bad idea to submit period in tx handler. I'm submitting period in block middleware now, PR to follow once done

johannbarbie · 2019-08-08T11:43:11Z

src/period/index.js

    setTimeout(async () => {
+      await submitPeriodVote(


why did you decide to call submitPeriodVote() here?

this is called when tendermint is trying to propose a new block (right?), and submitPeriod() is called as a fallback, in case the selected proposer is offline.

this handler is called every period (checkPeriod middleware). Right, I'm submitting the vote here as a fallback as well (submitPeriodVote has a protection against double votes).

updatePeriod is being called every block and that's where I submit a vote first: https://github.com/leapdao/leap-node/pull/305/files/15924f654aabe89a4492c03fa3c51f7f67b02545#diff-786c9ee7cb87a989994653f4a23150a6R25

johannbarbie

i would fix a few small things:

remove sendDelayed for sending periodVotes
validate that output index == 0 on all received votes
remove the code comments

johannbarbie · 2019-08-08T14:13:48Z

src/txHelpers/submitPeriod.test.js

  });
+
+  // describe('period vote', () => {


no code comments

src/tx/applyTx/checkPeriodVote.js

johannbarbie · 2019-08-08T14:18:05Z

src/tx/applyTx/checkPeriodVote.js

+  // validators are not in consensus on period roots atm
+  // disabling this until resolved
+  // https://github.com/leapdao/leap-node/issues/282#issuecomment-519160276
+  // const { result } = checkEnoughVotes(periodRoot, state);


remove code comments

troggy · 2019-08-08T15:02:54Z

addressed everything.

plus refactored to keep periodVotes in a tendermint state, not in a bridgeState. This is the right way to do (see)

collect votes in a state to use later for period submission

johannbarbie

looks great.
like the tests, CAS bitmap building looks good as well.

johannbarbie · 2019-08-09T10:25:08Z

src/tx/applyTx/checkPeriodVote.test.js

+  ...attrs,
+});
+
+describe('checkPeriodVote', () => {


johannbarbie · 2019-08-09T10:25:40Z

src/period/submitPeriodVote.test.js

+  prevHash: '0x5678',
+};
+
+describe('submit period vote', () => {


johannbarbie

why votes in the state? wouldn't that make the state grow infinitely?

i feel we should keep votes in bridgeState (with some pruning), and/or in some db, but not in memory forever.

johannbarbie

ok, given we prune the votes in the state later (out of scope), accepted

johannbarbie

classic

troggy marked this pull request as ready for review August 2, 2019 19:41

troggy mentioned this pull request Aug 2, 2019

Create basic CAS test leapdao/leap-sandbox#39

Open

troggy force-pushed the feat/support-period-vote branch 2 times, most recently from 984340e to 15924f6 Compare August 8, 2019 10:06

troggy mentioned this pull request Aug 8, 2019

Submit period only once got 2/3 of period votes #309

Closed

johannbarbie reviewed Aug 8, 2019

View reviewed changes

johannbarbie requested changes Aug 8, 2019

View reviewed changes

troggy requested a review from johannbarbie August 8, 2019 15:03

troggy added 12 commits August 9, 2019 12:08

feat: support period vote tx

ec42437

collect votes in a state to use later for period submission

feat: submit PeriodVote tx in checkBridge

3ec01d1

feat: propose period only if 2/3 of period votes collected

3e1973f

fix: alreadyVotedForPeriod

f95ac9b

feat: calculate CAS and submit to operator

9d7cc97

more tests

1d1fda8

submit period in checkPeriodVote tx handler

cd1dfe3

reduce the scope. period submission to be commited in a separate PR

685c30a

remove commented code

4d77b47

check prevout index for PeriodVote

6f9c89e

no delayed send for period vote tx

44c3bc6

keep periodVotes in state, not bridgeState

33bc998

johannbarbie force-pushed the feat/support-period-vote branch from 2716184 to 33bc998 Compare August 9, 2019 10:09

johannbarbie approved these changes Aug 9, 2019

View reviewed changes

johannbarbie requested changes Aug 9, 2019

View reviewed changes

johannbarbie approved these changes Aug 9, 2019

View reviewed changes

fix: initialize periodVotes in app state

9644584

johannbarbie approved these changes Aug 9, 2019

View reviewed changes

troggy merged commit c7ac7c1 into master Aug 9, 2019

troggy deleted the feat/support-period-vote branch August 9, 2019 14:36

troggy restored the feat/support-period-vote branch August 9, 2019 14:36

troggy deleted the feat/support-period-vote branch August 9, 2019 14:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support period vote tx #305

feat: support period vote tx #305

troggy commented Aug 1, 2019 •

edited

Loading

codecov bot commented Aug 1, 2019 •

edited

Loading

johannbarbie Aug 8, 2019

troggy Aug 8, 2019

johannbarbie Aug 8, 2019

troggy Aug 8, 2019 •

edited

Loading

johannbarbie Aug 8, 2019

troggy Aug 8, 2019

johannbarbie left a comment

johannbarbie Aug 8, 2019

johannbarbie Aug 8, 2019

troggy commented Aug 8, 2019 •

edited

Loading

johannbarbie left a comment

johannbarbie Aug 9, 2019

johannbarbie Aug 9, 2019

johannbarbie left a comment •

edited

Loading

johannbarbie left a comment •

edited

Loading

johannbarbie left a comment

feat: support period vote tx #305

feat: support period vote tx #305

Conversation

troggy commented Aug 1, 2019 • edited Loading

codecov bot commented Aug 1, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troggy Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannbarbie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

troggy commented Aug 8, 2019 • edited Loading

johannbarbie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannbarbie left a comment • edited Loading

Choose a reason for hiding this comment

johannbarbie left a comment • edited Loading

Choose a reason for hiding this comment

johannbarbie left a comment

Choose a reason for hiding this comment

troggy commented Aug 1, 2019 •

edited

Loading

codecov bot commented Aug 1, 2019 •

edited

Loading

troggy Aug 8, 2019 •

edited

Loading

troggy commented Aug 8, 2019 •

edited

Loading

johannbarbie left a comment •

edited

Loading

johannbarbie left a comment •

edited

Loading