<p><b>@sqrrm</b> approved this pull request.</p>
<p>utACK</p>
<p>Added some fixes to spelling mistakes and variable naming.</p>
<p>Otherwise this looks better, waiting for the end of batch parsing and checking the current phase at that height before publishing vote. I wonder if our usage of chainHeight, blocHeight, height and what not is lacking since we're getting confused. Perhaps better naming would help us understand better what we're doing.</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2360#discussion_r253307329">core/src/main/java/bisq/core/dao/node/BsqNode.java</a>:</p>
<pre style='color:#555'>> // The chain height of the latest Block we either get reported by Bitcoin Core or from the seed node
+ // This property should not be used in consensus code but only or retrieving blocks as it is not in sync with the
</pre>
⬇️ Suggested change
<pre style="color: #555">- // This property should not be used in consensus code but only or retrieving blocks as it is not in sync with the
+ // This property should not be used in consensus code but only for retrieving blocks as it is not in sync with the
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2360#discussion_r253307453">core/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java</a>:</p>
<pre style='color:#555'>> // the blind vote was created in case we have not done it already.
// The voter need to be at least once online in the reveal phase when he has a blind vote created,
- // otherwise his vote becomes invalid and his locked stake will get unlocked
- private void maybeRevealVotes() {
- // We must not use daoStateService.getChainHeight() because that gets updated with each parsed block but we
- // 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();
+ // otherwise his vote becomes invalid.
+ // In case the user miss the vote reveal phase an (invalid) vote reveal tx will be created the next time the user is
+ // online. That tx only serves the purpose to unlock the stake from teh blind vote but it will be ignored for voting.
</pre>
⬇️ Suggested change
<pre style="color: #555">- // online. That tx only serves the purpose to unlock the stake from teh blind vote but it will be ignored for voting.
+ // online. That tx only serves the purpose to unlock the stake from the blind vote but it will be ignored for voting.
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2360#discussion_r253309168">core/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java</a>:</p>
<pre style='color:#555'>> } else {
+ // We missed the vote reveal phase but publish a vote reveal tx to unlock he blind vote stake.
</pre>
⬇️ Suggested change
<pre style="color: #555">- // We missed the vote reveal phase but publish a vote reveal tx to unlock he blind vote stake.
+ // We missed the vote reveal phase but publish a vote reveal tx to unlock the blind vote stake.
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2360#discussion_r253309226">core/src/main/java/bisq/core/dao/governance/votereveal/VoteRevealService.java</a>:</p>
<pre style='color:#555'>> try {
// We collect all valid blind vote items we received via the p2p network.
// It might be that different nodes have a different collection of those items.
// To ensure we get a consensus of the data for later calculating the result we will put a hash of each
- // voters blind vote collection into the opReturn data and check for a majority at issuance time.
+ // voters blind vote collection into the opReturn data and check for a majority at the vote result phase.
</pre>
⬇️ Suggested change
<pre style="color: #555">- // voters blind vote collection into the opReturn data and check for a majority at the vote result phase.
+ // voter's blind vote collection into the opReturn data and check for a majority in the vote result phase.
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2360#discussion_r253309515">core/src/main/java/bisq/core/dao/governance/voteresult/VoteResultException.java</a>:</p>
<pre style='color:#555'>> @@ -28,17 +28,17 @@
public class VoteResultException extends Exception {
@Getter
- private final Cycle cycle;
+ private final int heightOfFirstBlock;
</pre>
<p>The name <code>heightOfFirstBlock</code> is a bit confusing without the context that it's the first block in the cycle</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/2360#pullrequestreview-199392927">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AkpZtvX1TvLDcAOYg_ydAUPB69acK33Jks5vJxWBgaJpZM4agKi1">mute the thread</a>.<img src="https://github.com/notifications/beacon/AkpZtiPuJBiBkL9Z3jgpkfydhbb8_yI3ks5vJxWBgaJpZM4agKi1.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/bisq-network/bisq","title":"bisq-network/bisq","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/bisq-network/bisq"}},"updates":{"snippets":[{"icon":"PERSON","message":"@sqrrm approved #2360"}],"action":{"name":"View Pull Request","url":"https://github.com/bisq-network/bisq/pull/2360#pullrequestreview-199392927"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/2360#pullrequestreview-199392927",
"url": "https://github.com/bisq-network/bisq/pull/2360#pullrequestreview-199392927",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>