V0.12.1.x governance pr - part 1 - base functionality by tgflynn · Pull Request #944 · dashpay/dash

tgflynn · 2016-08-16T15:14:34Z

No description provided.

This macro allows debugging statements (typically printf's or cout's) to be activated or deactivated with a single comment. Uncomment the line: //#define ENABLE_DASH_DEBUG in util.h to enable debugging statements. When commented any code wrapped with the DBG() macro will simply be removed by the preprocessor. When not commented all such wrapped statements will be present. For maximum effectiveness it is best that util.h be the first effective include in all source files. It is also possible to enable the macro for a single file by temporarily adding #define ENABLE_DASH_DEBUG to the top of the file. Code committed to non-development branches should always have the define commented.

…stnet phase II This commit contains the core governance system changes for 0.12.1. Any unrelated changes have either been removed or moved to separate commits.

Changed mode 0755->0644 on several source files.

- Added reminder to revert temporary reduction of number of votes required to trigger superblock to 1 for testing

This is for efficiency since this function is called often and is only 1 line of code.

…Valid

…rblockTriggered. Since calls to the later function are always protected by the former there's no reason to keep these separate and this simplifies the code in masternode-payments.cpp.

…eValid

Note this has an effect on testing because we can now only create 1 superblock per day. Devs may need to temporarily change testnet params for easier testing.

- We assume that payment values in JSON are in units of DASH for consistency with other RPC functions, such as createrawtransaction.

- Also ensure that number of votes required is never smaller than 1

Exceptions are now thrown consistently rather than using a mix of exceptions and return code checking. Exceptions are now caught only in AddNewTrigger when the CSuperblock constructor is called. Unnecessary object status members have been removed.

This appears to help with travis tests, for unknown reasons.

schinzelh · 2016-08-16T19:27:48Z

@@ -0,0 +1,261 @@
+


nit: newline

schinzelh · 2016-08-16T19:32:48Z

utACK d7c8a6b

UdjinM6 · 2016-08-16T19:40:41Z

+ LOCK(governance.cs);
+ // GET ALL ACTIVE TRIGGERS
+ std::vector<CSuperblock_sptr> vecTriggers = triggerman.GetActiveTriggers();
+ //int nYesCount = 0;


nit: commented unused? could probably remove it completely

I'd prefer to leave it for now. Uncommenting line 310 is the patch I apply for dev testing. (Although next time I'll use a small payment amount to avoid knocking out any more testnet nodes).

UdjinM6 · 2016-08-17T00:52:38Z


 {
-LOCK2(cs_main, mempool.cs);
+LOCK(cs_main);


I don't see how any of changes here (miner.cpp) are related to this PR. Should be reverted imo.

I thought I had reverted that but it looks like I missed it somehow.

UdjinM6 · 2016-08-17T01:14:08Z

 #ifndef MASTERNODE_PAYMENTS_H
 #define MASTERNODE_PAYMENTS_H

+#include "util.h"


no need to include it here

Making util.h the first include ensures that the DBG macro can be enabled on a per file basis.

#include "util.h" is already there in masternode-payments.cpp, if you need DBG somewhere else where masternode-payments.h is included imo it's better to include util.h there explicitly rather than having chaining dependencies through masternode-payments.h.

eduffield222 · 2016-08-17T03:23:39Z

ACK, merge when ready 👍

tgflynn · 2016-08-17T03:24:50Z

What about the miner reward (ie. my question on the core-dev channel) ?

This should have been done in the original PR but was overlooked.

- Replaced redundantly defined function with inclusion of governance-vote.h - Replaced magic numbers with their corresponding constant symbols

tgflynn · 2016-08-17T04:26:36Z

I pushed commits that fix the omitted locking reversion and most of the other issues raised.

Not sure what to do about miner rewards. Maybe we leave that for another PR, that I think should also better resolve the issues with block validation for nodes that are not in sync ?

UdjinM6 · 2016-08-17T06:44:48Z

I'm leaning towards merging this as it is and fixing CSuperblock::IsValid and other mining reward related issues (if any) in another PR. All other major issues seems to be fixed, so should be helpful to have new base and solve issues/refactor pieces individually later.
utACK 0a37966

schinzelh · 2016-08-17T06:53:18Z

utACK 0a37966 as well - will merge when Travis is ready :)

schinzelh · 2016-08-17T07:08:48Z

Squashed and merged de7b2b6

tgflynn added 16 commits August 12, 2016 16:27

V0.12.1.x -- merging trigger/generic object/superblock changes for te…
d125d9b
…stnet phase II This commit contains the core governance system changes for 0.12.1. Any unrelated changes have either been removed or moved to separate commits.

File mode fixes
120724c
Changed mode 0755->0644 on several source files.

Updated todo reminders
c7f9e11
- Added reminder to revert temporary reduction of number of votes required to trigger superblock to 1 for testing

Made CSuperblockManager::IsValidSuperblockHeight an inline function
92adc98
This is for efficiency since this function is called often and is only 1 line of code.

Added comment explaining rationale for no LOCK(cs) in CSuperblock::Is…
c050ed7
…Valid

Removed unused CSuperblockManager::IsBlockValid function
dc933fe

Moved calls to SuperblockManager::IsValidSuperblockHeight into IsSupe…
decec88
…rblockTriggered. Since calls to the later function are always protected by the former there's no reason to keep these separate and this simplifies the code in masternode-payments.cpp.

Reestablished expected value check for non-superblocks in IsBlockValu…
8672885
…eValid

Changes to IsBlockValueValid to fix rpc test failure
b01cbe0

Changed include order to allow per file activation of the DBG macro
a937c76

Fixed IsValidSuperblockHeight logic
d116aa5
Note this has an effect on testing because we can now only create 1 superblock per day. Devs may need to temporarily change testnet params for easier testing.

Convert superblock payments to CAmount
2d0c2de
- We assume that payment values in JSON are in units of DASH for consistency with other RPC functions, such as createrawtransaction.

Revert temporary testing value for nAbsVoteReq
376b833
- Also ensure that number of votes required is never smaller than 1

Cleaned up CSuperblock error handling
8c89f4b
Exceptions are now thrown consistently rather than using a mix of exceptions and return code checking. Exceptions are now caught only in AddNewTrigger when the CSuperblock constructor is called. Unnecessary object status members have been removed.

Removed utilstrencodings header
d7c8a6b
This appears to help with travis tests, for unknown reasons.

schinzelh reviewed Aug 16, 2016
View reviewed changes

Comment threadsrc/governance-classes.h Outdated
@@ -0,0 +1,261 @@

schinzelhAug 16, 2016
Copy link
Copy Markdown
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: newline

UdjinM6 reviewed Aug 16, 2016
View reviewed changes

Fixed some minor code review issues
c4dfc7a

UdjinM6 reviewed Aug 17, 2016
View reviewed changes

eduffield222 mentioned this pull request Aug 17, 2016
V0.12.1.x -- merging trigger/generic object/superblock changes for testnet phase II #932
Closed

tgflynn added 4 commits August 16, 2016 23:37

Reverted locking change in miner.
63c3580
This should have been done in the original PR but was overlooked.

Fixed variable name to match common practice and bracket formatting
4ab72de

Improvements to vote conversion code
886a678
- Replaced redundantly defined function with inclusion of governance-vote.h - Replaced magic numbers with their corresponding constant symbols

Reordered governance message handling
0a37966

UdjinM6 changed the title ~~V0.12.1.x governance pr~~V0.12.1.x governance pr - part 1 - base functionalityAug 17, 2016

UdjinM6 added the enhancement label Aug 17, 2016

schinzelh merged commit de7b2b6 into dashpay:v0.12.1.xAug 17, 2016

tgflynn deleted the v0.12.1.x-governance-PR branch September 7, 2016 13:06

Uh oh!

Conversation

tgflynn commented Aug 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schinzelh commented Aug 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduffield222 commented Aug 17, 2016

Uh oh!

tgflynn commented Aug 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgflynn commented Aug 17, 2016

Uh oh!

UdjinM6 commented Aug 17, 2016

Uh oh!

schinzelh commented Aug 17, 2016

Uh oh!

schinzelh commented Aug 17, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tgflynn commented Aug 17, 2016•
edited
Loading