Validate proposals on prepare and submit #1488

tgflynn · 2017-06-06T13:37:10Z

There have been some issues with the submission of invalid proposals. Currently these are detected only by sentinel and can thus result in the loss of the collateral fee for the submitter.

This PR replicates sentinel's proposal validation in dashcore and will reject invalid proposals from being prepared or submitted.

It also provides a new RPC command: "gobject check" which allows proposal data to be validated without attempting to create a proposal.

Includes commits: Implemented CProposalValidator Use CProposalValidator to check proposals at prepare and submit stages Modify proposal validator to support numerical data in string format Multiple bug fixes in governance-validators.cpp Fixed bug in CheckURL Fixed stream state check Increase strictness of payment address validation for compatibility with sentinel Improved error reporting Implemented "check" rpc command to validate proposals Fixes to RPC check command Fix error message Unit test and data files for proposal validator Added test cases Removed debugging code

UdjinM6

Nice, thanks 👍
Looks mostly ok for me, see inline comments for few issues.

UdjinM6 · 2017-06-06T16:26:04Z

src/governance-validators.cpp

+bool CProposalValidator::Validate()
+{
+    if(!ValidateJSON()) {
+        return false;


Maybe add error msg like "Invalid JSON" or "Invalid format" here to avoid silent failure

Those error messages are added by ParseJSONData. The only case not covered is if the data string is empty. I will add a message for that.

UdjinM6 · 2017-06-06T16:26:40Z

src/governance-validators.cpp

+    }
+
+    if(nEndEpoch <= nStartEpoch) {
+        strErrorMessages += "end_epoch <= start_epoch field not found;";


This message is confusing

Yes, that's a bug, will fix.

UdjinM6 · 2017-06-06T16:29:15Z

src/governance-validators.cpp

+    }
+
+    if(dValue <= 0.0) {
+        strErrorMessages += "payment_amount invalid;";


Could be more specific here i.e. "payment_amount is negative". Also shouldn't there also be a max payment_amount validation (i.e. if (dValue > total_budget_at_end_epoch) ...)?

My goal was to replicate the sentinel Proposal.is_valid() method.

However though that method doesn't check for too large amounts, such amounts will be rejected at superblock trigger creation time so I guess it would make sense to add that check here.

Also Validate() concatenates "Invalid payment amount"

The problem is that the total budget depends on the block height of the superblock and that isn't well defined in general for proposals because the start/end epoch can span multiple superblocks.

So I'm not sure how to implement this.

So I'm not sure how to implement this.

Estimate blocks height at end_epoch using consensus.nPowTargetSpacing, current timestamp and current block height;

Find next sb for that height, same way as in getgovernanceinfo - probably can move calculation to a standalone function e.g.

void CSuperblock::GetNearestSuperblocksHeights(int nBlockHeight, int &nLastSuperblock, int &nNextSuperblock) { const Consensus::Params& consensusParams = Params().GetConsensus(); int nSuperblockStartBlock = consensusParams.nSuperblockStartBlock; int nSuperblockCycle = consensusParams.nSuperblockCycle; // Get first superblock int nFirstSuperblockOffset = (nSuperblockCycle - nSuperblockStartBlock % nSuperblockCycle) % nSuperblockCycle; int nFirstSuperblock = nSuperblockStartBlock + nFirstSuperblockOffset; if(nBlockHeight < nFirstSuperblock) { nLastSuperblock = 0; nNextSuperblock = nFirstSuperblock; } else { nLastSuperblock = nBlockHeight - nBlockHeight % nSuperblockCycle; nNextSuperblock = nLastSuperblock + nSuperblockCycle; } }

and then just reuse it in both cases;
3. Use CAmount CSuperblock::GetPaymentsLimit(int nBlockHeight) to get max total budget at nNextSuperblock.

Yes, that would work because the budget should be a monotonically decreasing function of block height so you only need to check the block height corresponding to end_epoch.

But is nPowTargetSpacing = 2.5 minutes the most accurate way to estimate block height from time ? It appears that sentinel uses 2.62 minutes. Accuracy could be quite important because theoretically the the end_epoch could be a number of months in the future.

That's another reason why we should use block heights and estimate epochs and not the otherwise ;) OK, let's maybe write a TODO here for now.

willwray · 2017-06-07T12:29:31Z

src/governance-validators.cpp

+    ParseJSONData();
+}
+
+bool CProposalValidator::Validate()


Just checking that this is intended behaviour:
do the validations in the given order with 'early-out' returns
rather than
do all the validations with a single return concatenating any/all error messages

Yes, that's what was intended.

willwray · 2017-06-07T12:51:23Z

src/governance-validators.cpp

+        return false;
+    }
+
+    static const std::string strAllowedChars = "-_abcdefghijklmnopqrstuvwxyz012345789";


Use const char* for this named sting literal (or put the string literal directly at the single point of use):
Avoids the need for static qualification as string literals are magically static.
Saves unnecessary std::string initialization and allocation.

Maybe put the capital letters in too, to save on the transform tolower.

I disagree, the safety advantages of standardizing on std::string far outweigh the negligible cost of the small extra memory allocation.

String literals, constants, are a special case and have some safety benefits
(as we are now on C++11, C-style mutable access to string literals is illegal closing that security hole).
Zero-termination is guaranteed for string literals.
They are magically static and may be held in protected memory with some compiler checks on bounds.

The suggestion to move the literal directly to its single point of use is good for safety, no?

if(strName.find_first_not_of( "-_abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ012345789") != std::string::npos)

The name currently self-documents that these are allowed characters but that's clear in the code.
Apart from the documentation value there's no need for a separate variable, especially one that allocates.
The string literal is there anyway - adding more moving parts is only adding potential exploit vectors.

If there is a need for a name and const char* feels unsafe then const char[] captures the size and so can be used with iterators, for instance:

char hi[] = "hello"; std::copy(std::begin(hi),std::end(hi),std::ostream_iterator<char>(std::cout));

(free function std begin,end are C++11 and constexpr since C++14, std::size is C++17)

In this case we have a single use string literal, used in a std call that accepts char* so const char* is fine.

Allocation is another discussion, more related to my comments on string concatentation.

willwray · 2017-06-07T12:57:25Z

src/governance-validators.cpp

+        return false;
+    }
+
+    static const std::string base58chars = "123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz";


const char* as above

willwray · 2017-06-07T13:24:04Z

src/governance-validators.cpp

+
+    std::string strURLStripped = StripWhitespace(strURL);
+
+    if(int(strURLStripped.size()) < 4) {


Kill the cast
If the cast is there to quieten the warning of 'comparing signed and unsigned'
then use an unsigned literal instead
if(strURLStripped.size() < 4u)

willwray · 2017-06-07T13:55:43Z

src/governance-validators.cpp

+        fJSONValid = true;
+    }
+    catch(std::exception& e) {
+        strErrorMessages += std::string(e.what()) + std::string(";");


I'd drop the second std::string and append ";" directly.
Others below.

More explanation on my comment.
If the error message type is changed from string to stringstream then it is more clearly redundant to convert output char* parameters to strings before streaming them out:

strstreamErrorMessages << e.what() << ";";

Apart from type and syntax, the calling pattern is the same so there is no reason to treat the parameters differently in string vs stringstream. Sticking with string, the two string conversions on the output parameters can be dropped, with no safety repercussions:

strErrorMessages += e.what(); strErrorMessages += ';';

Compare generated code to estimate the cost in terms of code size.
The links below use Compiler Explorer https://godbolt.org,
(x86-64 gcc 6.3 with -Os option to optimise for size.)

For the baseline string concatenation with redundant string conversions:

strErrorMessages += std::string(e.what()) + std::string(";");

Baseline >130 lines, 13 library calls

For the suggested refactor:

strErrorMessages += e.what(); strErrorMessages += ';';

Suggestion <10 lines 2 library calls

The cost of allocation is harder to quantify, Many small allocations add up.

willwray · 2017-06-07T14:00:30Z

src/governance-validators.cpp

+        {
+            std::istringstream istr(uValue.get_str());
+            istr >> dValue;
+            fOK = ! istr.fail();


nitpick, inconsistent spacing.
Then again, ! does look like i and !i does my eyes in!
Feel free to ignore

willwray · 2017-06-07T14:06:36Z

src/governance-validators.cpp

+
+std::string CProposalValidator::StripWhitespace(const std::string& strIn)
+{
+    static const std::string strWhitespace = " \f\n\r\t\v";


const char* as above.

willwray · 2017-06-07T14:19:44Z

src/governance-validators.cpp

+ */
+bool CProposalValidator::CheckURL(const std::string& strURLIn)
+{
+    std::string strRest(strURLIn);


As this is copying the function argument and is its only use, this std::string could be made the function argument.
'value semantics' also allows an r-value string argument to be moved in, avoiding the copy allocation.

willwray · 2017-06-07T14:21:40Z

src/governance-validators.cpp

+
+    // Process netloc
+    if((strRest.size() > 2) && (strRest.substr(0,2) == "//")) {
+        static const std::string strNetlocDelimiters = "/?#";


const char*

willwray · 2017-06-07T14:30:47Z

src/governance-validators.cpp

+    }
+
+    if(dValue <= 0.0) {
+        strErrorMessages += "payment_amount invalid;";


Also Validate() concatenates "Invalid payment amount"

UdjinM6

Since this is not a performance or consensus critical part and this code is going to be rarely used at all I don't see a need to micro- or over-optimization here.

utACK

willwray · 2017-06-19T14:35:34Z

utACK

Updating string handling patterns in dash or bitcoin code calls for wider discussion.

Tim Flynn added 2 commits June 5, 2017 21:35

Fix name validation

e574650

UdjinM6 reviewed Jun 6, 2017

View reviewed changes

UdjinM6 added this to the 12.2 milestone Jun 7, 2017

willwray reviewed Jun 7, 2017

View reviewed changes

Changes to address code review comments

3d11409

UdjinM6 approved these changes Jun 19, 2017

View reviewed changes

UdjinM6 added the enhancement label Jun 21, 2017

UdjinM6 merged commit a109a61 into dashpay:v0.12.2.x Jun 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate proposals on prepare and submit #1488

Validate proposals on prepare and submit #1488

tgflynn commented Jun 6, 2017

UdjinM6 left a comment

UdjinM6 Jun 6, 2017

tgflynn Jun 7, 2017

UdjinM6 Jun 6, 2017

tgflynn Jun 7, 2017

UdjinM6 Jun 6, 2017

tgflynn Jun 7, 2017

willwray Jun 7, 2017

tgflynn Jun 7, 2017

UdjinM6 Jun 7, 2017

tgflynn Jun 8, 2017

UdjinM6 Jun 8, 2017

willwray Jun 7, 2017

tgflynn Jun 8, 2017

willwray Jun 7, 2017

tgflynn Jun 8, 2017

willwray Jun 8, 2017 •

edited

Loading

willwray Jun 7, 2017

willwray Jun 7, 2017

willwray Jun 7, 2017

willwray Jun 8, 2017 •

edited

Loading

willwray Jun 13, 2017

willwray Jun 7, 2017

willwray Jun 7, 2017

willwray Jun 7, 2017

willwray Jun 7, 2017

willwray Jun 7, 2017

UdjinM6 left a comment

willwray commented Jun 19, 2017


		std::string strURLStripped = StripWhitespace(strURL);

		if(int(strURLStripped.size()) < 4) {

Validate proposals on prepare and submit #1488

Validate proposals on prepare and submit #1488

Conversation

tgflynn commented Jun 6, 2017

UdjinM6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willwray Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willwray Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

willwray commented Jun 19, 2017

willwray Jun 8, 2017 •

edited

Loading

willwray Jun 8, 2017 •

edited

Loading