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

fix(rpc)!: require proTxHash to be unspecified when not asking for ENCRYPTED_CONTRIBUTIONS in quorum getdata rpc #5772

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Dec 17, 2023

Issue being fixed or feature implemented

Because when we ask for a quorum wide data only (QUORUM_VERIFICATION_VECTOR) and not for a data about one specific MN proTxHash is not used in any way in this case and should not be provided. If it still was provided then maybe user doesn't quite understand what he is doing exactly or maybe he made a typo in dataMask.

What was done?

How Has This Been Tested?

Breaking Changes

quorum getdata RPC will no longer allow proTxHash to be specified when dataMask is set to 1.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

…PTED_CONTRIBUTIONS` in `quorum getdata` rpc
@UdjinM6 UdjinM6 added this to the 21 milestone Dec 17, 2023
@UdjinM6 UdjinM6 changed the title rpc: require proTxHash to be unspecified when not asking for ENCRYPTED_CONTRIBUTIONS in quorum getdata rpc fix(rpc): require proTxHash to be unspecified when not asking for ENCRYPTED_CONTRIBUTIONS in quorum getdata rpc Dec 17, 2023
@UdjinM6 UdjinM6 changed the title fix(rpc): require proTxHash to be unspecified when not asking for ENCRYPTED_CONTRIBUTIONS in quorum getdata rpc fix(rpc)!: require proTxHash to be unspecified when not asking for ENCRYPTED_CONTRIBUTIONS in quorum getdata rpc Dec 17, 2023
@PastaPastaPasta
Copy link
Member

Release notes + add text to "Breaking changes" section of pr?

ogabrielides
ogabrielides previously approved these changes Dec 17, 2023
Copy link
Collaborator

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

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

utACK

@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Dec 18, 2023
ogabrielides
ogabrielides previously approved these changes Dec 18, 2023
Copy link
Collaborator

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

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

re-utACK

@@ -730,6 +730,9 @@ static UniValue quorum_getdata(const JSONRPCRequest& request, const LLMQContext&
} else {
throw JSONRPCError(RPC_INVALID_PARAMETER, "proTxHash missing");
}
} else if (!request.params[4].isNull()) {
// Require no proTxHash otherwise
throw JSONRPCError(RPC_INVALID_PARAMETER, "Should not specify proTxHash");
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it easy to make functional test for this case? 🤔

Copy link
Collaborator

@ogabrielides ogabrielides left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

utACK

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

will review when we branch of v21

@UdjinM6 UdjinM6 marked this pull request as draft February 13, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants