Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DIP3 and DIP6 bugs #2815

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

panleone
Copy link

@panleone panleone commented Feb 27, 2023

The aim of this PR is fixing all the bugs (mostly are trivial) that we are finding while testing the v6.0

Bugs found:

  • help command was not working because DIP3 check was done too early
  • protx_register_fund was not working because a parameter needed to be added in rpc/client.cpp
  • Consider in the qt that rewards are coinbase

TODO (BUGS That still must be solved):
keep finding more bugs

DeanSparrow
DeanSparrow previously approved these changes Mar 3, 2023
Copy link

@DeanSparrow DeanSparrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 8b00afa

Works as intended

@Fuzzbawls
Copy link
Collaborator

The changes to src/rpc/masternode.cpp are un-necessary here. Once the chain reaches the spork 21 activation height and legacy MNs become unsupported, there is no sense in providing any help text to the startmasternode RPC command because it is functionally useless at that point.

To continue providing help output after this command is rendered moot would only serve to introduce confusion.

@Liquid369
Copy link
Member

Liquid369 commented Mar 4, 2023

@Fuzzbawls
While working on things this was needed to fix the issues with the pivx-cli help command throwing like such below:
./pivx-cli help error code: -1 error message: startmasternode is not supported when deterministic masternode list is active (DIP3)

That change renders this error from occurring moving forward.

Considering it will be depreciated, it is still in use now

@Fuzzbawls
Copy link
Collaborator

oh you mean the overall help list...

in that case, this would be more optimal at the top of the function:

Index: src/rpc/masternode.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/rpc/masternode.cpp b/src/rpc/masternode.cpp
--- a/src/rpc/masternode.cpp	(revision 59e4c1acb092abc46b825fd461a86fef0e80bd71)
+++ b/src/rpc/masternode.cpp	(date 1677928321976)
@@ -410,7 +410,11 @@
 {
     // Skip after legacy obsolete. !TODO: remove when transition to DMN is complete
     if (deterministicMNManager->LegacyMNObsolete()) {
-        throw JSONRPCError(RPC_MISC_ERROR, "startmasternode is not supported when deterministic masternode list is active (DIP3)");
+        if (request.fHelp) {
+            throw std::runtime_error("startmasternode (deprecated and no longer functional)");
+        } else {
+            throw JSONRPCError(RPC_MISC_ERROR, "startmasternode is not supported when deterministic masternode list is active (DIP3)");
+        }
     }
 
     CWallet * const pwallet = GetWalletForJSONRPCRequest(request);

@Liquid369
Copy link
Member

Okay yes I agree in this change accounting for the help request, and yes also help fixing was the intended solution.

@panleone
Copy link
Author

panleone commented Mar 4, 2023

updated to suggested change which is indeed cleaner, by the way don't tACK PR has still TODO to be done

@Fuzzbawls
Copy link
Collaborator

by the way don't tACK PR has still TODO to be done

if you intend to keep this PR open whilst further changes are pending or "in discovery", either add [WIP] as a prefix to it's title, or convert it to a draft PR. otherwise it is implied that the PR is finalized and ready for review, and any noted TODO items are intended for future PRs.

@panleone panleone marked this pull request as draft March 4, 2023 13:00
@Fuzzbawls
Copy link
Collaborator

btw, Draft or [WIP] status/notation should not discourage anyone from giving review feedback if they notice something that should be changed, but should avoid the ACK/tACK stamp of approval on something that is clearly not finalized just yet.

@Fuzzbawls Fuzzbawls added this to the 6.0.0 milestone Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants