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

Dao: fix vote reveal tx publishing #2195

Merged
merged 13 commits into from Jan 5, 2019

Conversation

Projects
None yet
2 participants
@ManfredKarrer
Copy link
Member

ManfredKarrer commented Jan 3, 2019

@sqrrm Opened the PR for review but need more testing before I merge it.

ManfredKarrer added some commits Jan 3, 2019

Add chainTipHeight property
- We want to know for the vote reveal service what is the latest block.
Currently that is not exposed in the DAO only in the BitcoinJ classes,
but we don't want to access it from there.
Use chain tip height for vote reveal tx publishing
- Use the chain tip height and not the current chain height in the
parsing to check if we are in the correct phase and cycle. Only
publish the tx if we are in the correct cycle.
Handle late vote reveal txs
If the phase and cycle for the vote reveal tx was missed we still
publish it but it is considered invalid. We do not throw an exception
but filter such txs away from the vote result evaluation.

We cannot use the strategy to unlock the BSQ from the vote tx in such a
case because the blind vote tx is already in the past and is not parsed
again (snapshot).
Alternatively we could have used a different tx type for the unlock
purpose but we prefer to keep such an exceptional case simple.

@ManfredKarrer ManfredKarrer requested a review from sqrrm Jan 3, 2019

@ManfredKarrer

This comment has been minimized.

Copy link
Member

ManfredKarrer commented Jan 3, 2019

@sqrrm It is built on top of #2191 . You can ignore those commits....

@ManfredKarrer ManfredKarrer changed the title WIP Dao fix vote reveal tx publishing WIP Dao: fix vote reveal tx publishing Jan 3, 2019

ManfredKarrer added some commits Jan 3, 2019

Refactor maybeRevealVotes method.
Add exception handling to revealVote
Remove phase and cycle check for vote reveal txs.
We do not check phase or cycle as a late voteReveal tx is considered
a valid BSQ tx. The vote result though will ignore such votes.

Add more log info.
Ignore items if isBlindVoteInCorrectPhaseAndCycle is false.
If we get a voteReveal tx which was published too late we ignore it.
- Refactorings
- Improve logs

@ripcurlx ripcurlx changed the title WIP Dao: fix vote reveal tx publishing [WIP] Dao: fix vote reveal tx publishing Jan 4, 2019

@ManfredKarrer ManfredKarrer changed the title [WIP] Dao: fix vote reveal tx publishing Dao: fix vote reveal tx publishing Jan 4, 2019

@sqrrm

sqrrm approved these changes Jan 5, 2019

Copy link
Member

sqrrm left a comment

utACK

It might be worth simplifying the vote reveal publishing but not necessary.

I think this PR will also make sure to not burn any BSQ associated with a vote reveal, even if it's published before the vote reveal phase, but the vote calculation will still be correct.

// only want to publish the vote reveal tx if our current real chain height is matching the cycle and phase and
// not at any intermediate height during parsing all blocks. The bsqNode knows the latest height from either
// Bitcoin Core or from the seed node.
int chainHeight = bsqNode.getChainTipHeight();

This comment has been minimized.

@sqrrm

sqrrm Jan 5, 2019

Member

I have a feeling that we might want to wait to publish until we've parsed the chain up until chainTipHeight instead of publishing before we're done parsing. I don't have a concrete example, but publishing before knowing the actual current state seems like it could lead to more trouble. Maybe it would invalidate some BSQ, or maybe it would just burn some to fees.

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Jan 5, 2019

Member

The onNewBlockHeight is called before the parsing of the block. So at the first block of the reveal phase we create and publish the tx. In the next block it will be validated and added to to the DaoState. So we could not wait for parsing until it is not published.

// If we missed the cycle we don't care about the phase anymore.
boolean isBlindVoteTxInPastCycle = periodService.isTxInPastCycle(myVote.getTxId(), chainHeight);

if (missedPhaseSameCycle || isBlindVoteTxInPastCycle) {

This comment has been minimized.

@sqrrm

sqrrm Jan 5, 2019

Member

It seems all paths lead here except for when the time between the vote is cast and the reveal phase. Wouldn't it be easier to check at which block height the vote reveal should be sent and do a simple check if the block height is more than the reveal block height to send the vote reveal?

This comment has been minimized.

@ManfredKarrer

ManfredKarrer Jan 5, 2019

Member

Yes the current checks are a bit verbose. I preferred to make it very explicit which cases are handled and not optimize on logical code flow (which could be optimized). But feel free to make a suggestion for a change.

@ManfredKarrer ManfredKarrer merged commit a3287ab into bisq-network:master Jan 5, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ManfredKarrer ManfredKarrer deleted the ManfredKarrer:dao-fix-vote-reveal-tx-publishing branch Jan 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment