Skip to content

Remove ValidateMasternodeWinner in favour of IsTransactionValid - #58

Open
kerryoncrypto wants to merge 1 commit into
phoenixkonsole:masterfrom
kerryoncrypto:remove-validate-masternode-winner
Open

Remove ValidateMasternodeWinner in favour of IsTransactionValid#58
kerryoncrypto wants to merge 1 commit into
phoenixkonsole:masterfrom
kerryoncrypto:remove-validate-masternode-winner

Conversation

@kerryoncrypto

Copy link
Copy Markdown

Hello Telos-Team,

I know this PR will be a bit hard as you have spent much work for CMasternodePayments::ValidateMasternodeWinner() so far, but there is already such functionality on the source-code that just have to used the right way.

This PR removes CMasternodePayments::ValidateMasternodeWinner() from the code and re-arms the CMasternodePayments::IsTransactionValid()-Check with your SPORK_17_MASTERNODE_PAYMENT_CHECK-Spork.

Just like ValidateMasternodeWinner() assures IsTransactionValid() that there is a masternode-payment on the stake-transaction and has the right value spent. But in contrast to your implementation it is a bit more vague on the masternode that has to be paid. Your implementation requires that one specific masternode is paid which implies that the masternode-assets have to be the same on every node validating this transaction. As the number of masternodes grows and the synchronization-process is a bit laggy this is something that cannot be guaranteed and therefore should be avoided. IsTransactionValid adds a bit fuzz and allows a wider set of masternodes to be paid and therefore is likely to fail less. The function is taken from PIVX-upstream, used by most other masternode-coins and can be assumed to be safe - at least it's safe to assume that the payee is a valid masternode.

I'm looking forward for your feedback!

P.S.: It would be better to have this done via the already present but unused spork-key-mechanism (see SPORK_8_MASTERNODE_PAYMENT_ENFORCEMENT), but for the moment this PR is more like the update you intended first.

@DancingAxolotl

Copy link
Copy Markdown
Contributor

Thank you for bringing this to attention.

Reusing existing code certainly would be better. But in this case IsTransactionValid performs checks based on a different masternode selection process. As we added several tiers of masternodes, it is likely that validation logic from PIVX will not work correctly.

The best approach would be a combination of both checking a wide set of masternodes and enforcing masternode tiers. We will have to check if IsTransactionValid can also support validating tiers of masternodes. It probably needs some modifications for that.

@kerryoncrypto

Copy link
Copy Markdown
Author

Oh, you're right! I've missed this fact about the tiered MNs...
Whenever there is spare time, I'll look into it by myself.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants