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

Open
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)

@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? 🤔

ogabrielides
ogabrielides previously approved these changes Dec 27, 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

knst
knst previously approved these changes Dec 27, 2023
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
@UdjinM6 UdjinM6 removed this from the 21 milestone Jul 26, 2024
@UdjinM6 UdjinM6 dismissed stale reviews from knst and ogabrielides via db09b6b August 8, 2024 21:34
@UdjinM6 UdjinM6 added this to the 22 milestone Aug 8, 2024
@UdjinM6 UdjinM6 force-pushed the require_no_protx branch 2 times, most recently from f635cd7 to 00a176f Compare August 13, 2024 15:18
@UdjinM6 UdjinM6 marked this pull request as ready for review September 22, 2024 17:17
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.

ACK f18b4e8

The new test fails without changes in rpc/quorums.cpp indeed as expected

@PastaPastaPasta PastaPastaPasta removed this from the 22 milestone Nov 14, 2024
@PastaPastaPasta PastaPastaPasta added this to the 22.1 milestone Nov 14, 2024
@PastaPastaPasta
Copy link
Member

Anything blocking this? or should we merge it?

@UdjinM6 UdjinM6 modified the milestones: 22.1, 23 Nov 17, 2024
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.

5 participants