Skip to content

rpc|test: Introduce "mnauth" RPC command to override masternode authentications #3930

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.

Sign up for GitHub

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

Jump to bottom
Merged
PastaPastaPasta merged 5 commits into from
Jan 25, 2021
Merged

rpc|test: Introduce "mnauth" RPC command to override masternode authentications #3930

PastaPastaPasta merged 5 commits into from
Jan 25, 2021

Conversation

@xdustinface
Copy link

@xdustinface xdustinface commented Jan 12, 2021

Allows to fake verified MN P2P connections. Something i need for an other test. If someone has a better idea how to archive this, let me know :)

Based on #3929

@xdustinface xdustinface added the RPC Some notable changes to RPC params/behaviour/descriptions label Jan 12, 2021
@xdustinface xdustinface added this to the 17 milestone Jan 12, 2021
@xdustinface xdustinface marked this pull request as draft January 12, 2021 14:34
@xdustinface xdustinface force-pushed the pr-rpc-mnauth branch 2 times, most recently from 8e734be to db1bb93 Compare January 12, 2021 15:23
PastaPastaPasta
PastaPastaPasta reviewed Jan 12, 2021
View reviewed changes
@UdjinM6
Copy link

UdjinM6 commented Jan 12, 2021

Code looks good but... why?

Something i need for another test.

Can you expand on this a bit pls?

@xdustinface
Copy link
Author

xdustinface commented Jan 13, 2021 •
edited
Loading

Code looks good but... why?

Something i need for another test.

Can you expand on this a bit pls?

Im working on the QGETDATA/QDATA stuff which should be restricted to be used by masternodes only. At least that was the initial proposed idea from the document and normal nodes should anyway not care about it. And while i was writing tests i realised its not possible to write direct P2P tests if there is such a restriction. Soo, i see/saw the following options:

  1. Not restrict it to masternodes only at all
  2. Not restrict it to masternodes only for -regtest
  3. Not restrict it to masternodes only if some command line parameter is set
  4. Just fake MNAUTH for the testing P2P connection

I mean, actually i like having such a restriction but we could go with 1. if thats not a real concern (im not sure, just felt better to have it restricted..)? I guess the data should be cached then but it would make testing easier. Anyway, i don't like 2. and 3. because it wouldn't be a "full" test thats why i then took 4.

@UdjinM6
Copy link

UdjinM6 commented Jan 13, 2021 •
edited
Loading

Ok, so you want to use mininode to send/receive these new messages but to do this you want dashd you are connected to to think that this mininode is a masternode, right? Can't you just reuse qwatch instead?

@xdustinface
Copy link
Author

xdustinface commented Jan 13, 2021

Can't you just reuse qwatch instead?

Hm, not really. Maybe i just don't get how you think that would help but what i basically need is that the connected node has a verified protx hash assigned. This branch is still WIP but See xdustinface@aea5dd7, xdustinface@03e9edb. Its already implemented with the fake mnauth in xdustinface@c287a46 but im totally open to drop the implementation and this PR if you come up with something else which makes more sense, let me know your thoughts :)

@UdjinM6
Copy link

UdjinM6 commented Jan 14, 2021

How about xdustinface@aea5dd7#r45968519 ?

@xdustinface xdustinface mentioned this pull request Jan 19, 2021
Merged
@xdustinface
Copy link
Author

xdustinface commented Jan 19, 2021

@UdjinM6 Got it now with qwatch! I like the idea, see a55ac9a where i implemented it :) Buut, what do you think about adding both? I mean, the mnauth RPC command is basically useful for tests in general (depending on what we might do in the future with the MNAUTH for sure 🙈), no? And it at least helps in #3953 to somewhat test independent connections like here https://github.com/dashpay/dash/pull/3953/files#diff-2829e166b218f8d2acb7af7fd4e966be5cd8a21c9cc97f13fc29a8fa4a940dc6R274-R303. It's obviously not super important thing to test but it's one more 😄

@xdustinface xdustinface marked this pull request as ready for review January 22, 2021 22:21
@xdustinface xdustinface mentioned this pull request Jan 22, 2021
Merged
UdjinM6
UdjinM6 reviewed Jan 23, 2021
View reviewed changes
UdjinM6
UdjinM6 previously approved these changes Jan 23, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

PastaPastaPasta
PastaPastaPasta previously approved these changes Jan 23, 2021
View reviewed changes
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.

one nit, otherwise utACK

@xdustinface xdustinface dismissed stale reviews from PastaPastaPasta and UdjinM6 via 514cc80 January 23, 2021 19:56
@UdjinM6 UdjinM6 changed the title rpc|test: Introduce "mnauth" RPC command to fake masternode authentications rpc|test: Introduce "mnauth" RPC command to override masternode authentications Jan 23, 2021
UdjinM6
UdjinM6 approved these changes Jan 23, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 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

(tweaked PR title a bit)

PastaPastaPasta
PastaPastaPasta approved these changes Jan 25, 2021
View reviewed changes
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.

utACK

@PastaPastaPasta PastaPastaPasta merged commit ce3bcab into dashpay:develop Jan 25, 2021
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Mar 12, 2022
@xdustinface @UdjinM6
@PastaPastaPasta @gades
rpc|test: Introduce "mnauth" RPC command to override masternode authe…
9ba508c 
…ntications (dashpay#3930)

* rpc: Introduce "mnauth" RPC command to fake masternode authentications

* test: Test "mnauth" RPC command

* test: Add rpc_mnauth.py to BASE_SCRIPTS

* Update src/rpc/misc.cpp

Co-authored-by: UdjinM6 <[email protected]>

* Update test/functional/rpc_mnauth.py

Co-authored-by: PastaPastaPasta <[email protected]>

Co-authored-by: UdjinM6 <[email protected]>
Co-authored-by: PastaPastaPasta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees

No one assigned

Labels

RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Milestone

17

Development

Successfully merging this pull request may close these issues.

3 participants

@xdustinface @UdjinM6 @PastaPastaPasta