Fix request amount bounds in ReimbursementValidator #5077
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Use ReimbursementConsensus.get[Min|Max]ReimbursementRequestAmount in place of CompensationConsensus.get[Min|Max]CompensationRequestAmount, which was erroneously copied (it appears) from the very similar CompensationValidator class. This ensures the correct upper bound (currently 80,000 BSQ, starting at 10,000 BSQ and doubled in cycles 12, 13 & 14) is placed on reimbursement requests, instead of the erroneous 100,000 BSQ upper bound for compensation requests.
|
As by previous agreement this needs an ACK by @ManfredKarrer |
ManfredKarrer
approved these changes
Jan 15, 2021
There was a problem hiding this comment.
utACK
Yes seems that was a copy&paste bug. Thanks for spotting it!
To ensure it does not break anything, could you build from genesis and compare final Dao state hashes? A breakpoint in the catches should also spot any case when it would have been triggered (don't assume so).
|
I've just finished a rebuild from genesis (running as a DAO full node) and the final DAO state hashes still agree with those of the seed nodes. I put a breakpoint in each of the two catch blocks in |
ripcurlx
approved these changes
Jan 19, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use
ReimbursementConsensus.get[Min|Max]ReimbursementRequestAmountin place ofCompensationConsensus.get[Min|Max]CompensationRequestAmount, which was erroneously copied (it appears) from the very similarCompensationValidatorclass.This ensures the correct upper bound (currently 80,000 BSQ, starting at 10,000 BSQ and doubled in cycles 12, 13 & 14 as part of bisq-network/proposals#203) is placed on reimbursement requests, instead of the erroneous 100,000 BSQ upper bound for compensation requests.
--
This touches the DAO packages and potentially affects consensus. I don't believe any reimbursement requests exceeding the intended limit for each given cycle were actually made, so this shouldn't invalidate past vote cycles or BSQ issuance.
Converting DaoStateStore to text, filtering and manually copying out the relevant values (so there could be mistakes), I found 30 reimbursement requests in total, as follows:
The first eight requests are before the parameter changes in Cycle 14 activated (height 637267), so every reimbursement request after that should have had an 80,000 BSQ limit anyway.