Skip to content

[TierTwo] P2SH Proposal Payouts - #2830

Open
panleone wants to merge 3 commits into
PIVX-Project:masterfrom
panleone:p2sh_governance
Open

[TierTwo] P2SH Proposal Payouts#2830
panleone wants to merge 3 commits into
PIVX-Project:masterfrom
panleone:p2sh_governance

Conversation

@panleone

@panleone panleone commented Mar 29, 2023

Copy link
Copy Markdown

the idea of this PR is taken from #2265.
Having the governance system accept P2SH proposals is a nice addition since, for example, the work of a team can be paid in a single multisig address.
P2SH proposals will be enabled after v6.0 and is not backward compatible: in particular with the current implementation a v6 node will relay a p2sh proposal even to non-v6 nodes (and will lead to non v6 nodes banning the v6 node).
Since v6.0 will be a huge mandatory update I don't think it will be an issue

@panleone
panleone marked this pull request as draft March 29, 2023 07:19
@panleone

Copy link
Copy Markdown
Author

Ready for review: last commit is a bit out of scope and adds CheckString() in IsWellFormed() (It was a waste of time opening another PR just for that line)

@panleone
panleone marked this pull request as ready for review March 29, 2023 21:34
@Fuzzbawls Fuzzbawls changed the title P2SH Proposals [TierTwo] P2SH Proposal Payouts Apr 1, 2023
@Fuzzbawls Fuzzbawls added this to the 6.0.0 milestone Apr 1, 2023
@RainDeveloper69

Copy link
Copy Markdown

Thanks for working on enabling P2SH/multisig proposal payouts.

I reviewed the current implementation and found one issue that I think should be addressed before approval.

CheckStrings() can reject valid proposal URLs

After v6.0 activation, IsWellFormed() now calls CheckStrings():

if (isV6Enforced && !CheckStrings()) {
    return false;
}

CheckStrings() uses SanitizeString() with SAFE_CHARS_DEFAULT, which does not allow several standard URL characters such as &, =, #, %, and +.

This means valid proposal URLs containing query parameters or fragments may be rejected after v6.0 activation.

For example:

https://example.com/page?foo=1&bar=2

or:

https://example.com/proposal#details

would fail the current validation.

There is already a dedicated validateURL() helper in utilstrencodings, so it may be worth considering URL-specific validation here instead of using the generic SanitizeString() check.

The added functional test covering the P2SH proposal flow is useful, but I would also recommend adding regression coverage for valid URLs containing query parameters and fragments.

I don't see a consensus-level issue with the core P2SH payout change itself, but I think the URL validation behavior should be clarified or fixed before this PR is approved.

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

Projects

Status: QA

Development

Successfully merging this pull request may close these issues.

3 participants