llmq|rpc|test|version: Implement P2P messages QGETDATA <-> QDATA #3953

xdustinface · 2021-01-19T05:51:38Z

This PR introduces two new P2P messages and with that it bumps the protocol version to 70219. They will in a follow up PR be used for DKG data sharing between masternodes. Means this PR only contains the implementation of the messages and their tests.

The messages:

QGETDATA - Ask a masternode for DKG data. Can be both or either of the following:
- The quorum verification vector a specific quorum
- The encrypted DKG contributions from member of the quorum
QDATA - Respond with the requested DKG data.

5e858cf, 269ce38 and 38bde3f Contain the most part of the messages implementation

6b55815 Implements tests

285c9bd Introduces a RPC wrapper around QGETDATA called quorum getdata (I actually only did that to be able to test everything related to the new messages independently in this PR)

The other commits are mostly helper around the implementation.

This PR is based on #3929 #3930 #3937 #3943 #3948

xdustinface · 2021-01-25T10:33:18Z

Okay so now it should be ready here.. d8466b4 and 1b7519e are needed after #3943.
d8466b4 reverts changing the cache value there from non-const to const. Want to rather see it in its own PR? Let me know.
1b7519e Just adjusts cache usage

UdjinM6

👍

Pls see https://github.com/UdjinM6/dash/commits/pr3953 for some suggestions.

(4c6729e is #3969)

xdustinface · 2021-01-25T17:43:26Z

Pls see https://github.com/UdjinM6/dash/commits/pr3953 for some suggestions.

Looks good! Will push them with the rebase after #3969 :)

In the meantime, back to draft here one more time 😄

PastaPastaPasta · 2021-01-26T18:23:59Z

PEP8 format, which we mostly follow, wants all lines wrapped at 80 chars. That's not practical in a lot of situations / is annoying to do for EVERY line, but here it is

PastaPastaPasta · 2021-01-26T18:28:31Z

test/functional/p2p_quorum_data.py

+        p2p_mn1.test_qgetdata(qgetdata_contributions, 0, 0, self.llmq_size)
+        wait_for_banscore(mn1.node, id_p2p_mn1, 25)
+        # Request both
+        p2p_mn1.test_qgetdata(qgetdata_all, 0, self.llmq_threshold, self.llmq_size)
+        wait_for_banscore(mn1.node, id_p2p_mn1, 50)


Why does this bump ban score?

I requests more than once within EXPIRATION_TIMEOUT window

dash/src/llmq/quorums.cpp

Line 500 in 0b0a7a0

errorHandler("Request limit exceeded", 25);

Signed-off-by: pasta <pasta@dashboost.org>

xdustinface · 2021-01-26T18:40:25Z

PEP8 format, which we mostly follow, wants all lines wrapped at 80 chars. That's not practical in a lot of situations / is annoying to do for EVERY line, but here it is

I got the general idea behind its just that the execution of the idea looks arbitrary because you picked just two random lines out the many which are above 80 chars. But okay, picked it..🙈

UdjinM6

LGTM, utACK

PastaPastaPasta · 2021-01-27T23:49:23Z

Please see 07a906d

I think I'm emotionally done with it for now

Signed-off-by: pasta <pasta@dashboost.org>

Just takes a lot time and isn't required imo.

This was introduced in 9e224ec

"test_mnauth_restriction", "test_invalid_messages" and "test_invalid_unexpected_qdata" with the resulting name "test_basics" because i don't feel like DKG recovery thing should be part of a test called "test_invalid_messages" and giving it an own test probably wouldn't make a lot sense because it would still depend on "test_invalid_messages". I also think there is no need for a separated "test_invalid_unexpected_qdata".

Thats the default "draw annoying warnings" setting for PyCharm (and IMO a reasonable line length).

xdustinface · 2021-01-28T03:42:06Z

Please see 07a906d

Picked 👍 Added some more commits to clean it up a bit, see commit messages. (224a98a + be7c66d are just two more unrelated tweaks)

PastaPastaPasta

I don't think I'm a fan of c1a3387, seems like a regression creating a more complex testing method, instead of having small as reasonable, atomic tests

UdjinM6 · 2021-01-28T07:32:31Z

Pls see UdjinM6@4bcd660

xdustinface · 2021-01-28T14:55:46Z

@PastaPastaPasta I don't see how this makes anything "too complex".. it just additionally tests a bunch of error codes in one function with mostly the same lines of code.

instead of having small as reasonable, atomic tests

IMO we should rather see p2p_quorum_data.py as the atomic test here like we have many tests in separate files and they are all independent from each other (atomic tests), and not worry too much about "making parts of the tests inside the test atomic" because inside the test file they probably always depend on each other due to using the same nodes/blockchain. Now, the sections inside test_basics() are still as "atomic" as they were before (they still share resources btw so they anyway aren't atomic, are they?) with the difference that they are not longer separated by the name of the function but with a log line. Don't get me wrong, i like to give it a bit more structure in general but i honestly don't see a lot value in splitting them up into multiple functions where each function is just written and called below the other in the same file while they still all dependent on the same resources of the function they are written in i.e. the variables defined in run_test() and everything which belongs to the class QuorumDataMessagesTest. It's IMO basically the same effect then just separating them by just a log message with the only difference, that you can navigate into the separate functions which is IMO not a huge benefit in a file with 460 lines which is just an autonomous test for one feature.

Pls see UdjinM6@4bcd660

@UdjinM6 I like that approach in general and i had it like this in the evolution of the test but for some reason it seems to fail to get the correct ID on CI sometimes, see https://gitlab.com/UdjinM6/dash/-/jobs/991707418#L3241 (assert node.mnauth(node_id, protx_hash, operator_pubkey) fails which can obviously only happen if the provided nodeId is incorrect/does not belong to a connected node?. Thats why i ended up with the "directly return the found ID" approach :D If you have any idea how this can happen, tell me!

xdustinface · 2021-01-28T17:47:38Z

Okay i just saw this one https://gitlab.com/xdustinface/dash/-/jobs/991429505 where it fails the same for be7c66d so its obviously not related to cc38df2. But i think i at least didn't see this failure before all the recent refactoring.

UdjinM6

utACK

(#3971 should fix random mnauth failures mentioned above)

xdustinface · 2021-01-28T21:22:05Z

Nice, so if #3971 fixes it, this means the issue is that mininode didn't send verack the moment we do the mnauth call which leads to not having fSuccessfullyConnected (in core) set to true for the mininode connection and thats why then doesn't find it in the "fully connected" peers? i.e. 2220d61 should also do it?

I suspect the actual issue then to be this change where it initially slept 1s which then was maybe just enough to get the verack sent?

UdjinM6 · 2021-01-28T21:57:26Z

this means the issue is that mininode didn't send verack the moment we do the mnauth call

Not necessarily, could be that mininnode sent it but core was simply too slow at processing the message queue or busy with smth else. But yeah, 2220d61 would do it too I think 👍

PastaPastaPasta

utACK

…hpay#3953) * version: Bump PROTOCOL_VERSION and MIN_MASTERNODE_PROTO_VERSION * version: Introduce LLMQ_DATA_MESSAGES_VERSION for QGETDATA/QDATA support * test: Bump MY_VERSION to 70219 (LLMQ_DATA_MESSAGES_VERSION) * llmq: Introduce CQuorumDataRequest as wrapper for QGETDATA requests * llmq: Implement CQuorum::{SetVerificationVector, SetSecretKeyShare} * llmq|net|protocol: Implement QGETDATA/QDATA P2P messages * llmq: Restrict processing QGETDATA/QDATA to masternodes only * llmq: Implement request limiting for QGETDATA/QDATA * llmq: Implement CQuorumManger::RequestQuorumData * rpc: Implement "quorum getdata" as wrapper around QGETDATA Allows to trigger sending QGETDATA messages to connected peers by RPC. * test: Handle QGETDATA/QDATA messages in mininode * test: Add data structures to support QGETDATA/QDATA * test: Add some helper in test_framework.py * test: Implement tests for QGETDATA/QDATA in p2p_quorum_data.py * test: Add p2p_quorum_data.py to BASE_SCRIPTS * llmq|test: Add QWATCH support for QGETDATA/QDATA * llmq: Store CQuorumPtr in cache, not CQuorumCPtr * llmq: Fix cache usage after recent changes * Use uacomment to create/find specific p2ps * No need to use network adjusted time here, GetTime should be enough * rpc: check proTxHash * minor tweaks * test: Adjustments after 4e27d65 * llmq: Rename and improve error lambda in CQuorumManager::ProcessMessage * llmq: Process QDATA if -watchquorums is enabled * test: Handle qwatch messages in mininode * test: Add test for -watchquorums support * test: Just some empty lines * test: Properly stop the p2p network thread at the end of the test * rpc: Adjust "quorum getdata" parameter descriptions Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> * rpc: Fix optionality of proTxHash in "quorum getdata" command * test: Test optionality of proTxHash for "quorum getdata" command * test: Be more specific about imports in p2p_quorum_data.py * llmq|rpc: Add some comments about the request.GetDataMask checks * test: Some more empty lines * rpc: One more parameter description Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> * test: Unify assert statements / drop parentheses for all of them * fix typo Signed-off-by: pasta <pasta@dashboost.org> * adjust some line wrapping to 80 chars Signed-off-by: pasta <pasta@dashboost.org> * tests: Seperate out into dif atomic methods, add logging Signed-off-by: pasta <pasta@dashboost.org> * test: Avoid restarting masternodes, just let available requests expire Just takes a lot time and isn't required imo. * test: Drop redundant code/tests after separation This was introduced in 9e224ec * test: Merge three tests "test_mnauth_restriction", "test_invalid_messages" and "test_invalid_unexpected_qdata" with the resulting name "test_basics" because i don't feel like DKG recovery thing should be part of a test called "test_invalid_messages" and giving it an own test probably wouldn't make a lot sense because it would still depend on "test_invalid_messages". I also think there is no need for a separated "test_invalid_unexpected_qdata". * test: Rename test_ratelimiting_banscore -> test_request_limit * test: Apply python style * test: Wrap all at 120 characters Thats the default "draw annoying warnings" setting for PyCharm (and IMO a reasonable line length). * test: Move some variables * test: Optimize for speed * tests: use wait_until in get_mininode_id * test: Don't use `!=` to check for `None` Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com> Co-authored-by: pasta <pasta@dashboost.org>

xdustinface added RPC Some notable changes to RPC params/behaviour/descriptions P2P Some notable changes on p2p level labels Jan 19, 2021

xdustinface added this to the 17 milestone Jan 19, 2021

xdustinface marked this pull request as draft January 19, 2021 05:53

xdustinface force-pushed the pr-p2p-llmq-data branch from be47c80 to a55ac9a Compare January 19, 2021 05:57

xdustinface mentioned this pull request Jan 19, 2021

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

Merged

xdustinface force-pushed the pr-p2p-llmq-data branch 2 times, most recently from 51c69a3 to eed0929 Compare January 20, 2021 03:15

xdustinface mentioned this pull request Jan 22, 2021

llmq|init|test: Implement DKG data recovery / quorum verification vector sync #3964

Merged

xdustinface force-pushed the pr-p2p-llmq-data branch from eed0929 to 4411f35 Compare January 25, 2021 09:25

xdustinface marked this pull request as ready for review January 25, 2021 09:25

xdustinface requested review from PastaPastaPasta and UdjinM6 January 25, 2021 09:25

xdustinface marked this pull request as draft January 25, 2021 10:16

xdustinface force-pushed the pr-p2p-llmq-data branch from 4411f35 to 1b7519e Compare January 25, 2021 10:27

xdustinface marked this pull request as ready for review January 25, 2021 10:27

UdjinM6 mentioned this pull request Jan 25, 2021

tests: Add uacomment in P2PConnection and use it to create (correct network specific) strSubVer #3969

Merged

UdjinM6 requested changes Jan 25, 2021

View reviewed changes

xdustinface marked this pull request as draft January 25, 2021 17:43

xdustinface added 9 commits January 25, 2021 19:35

version: Bump PROTOCOL_VERSION and MIN_MASTERNODE_PROTO_VERSION

d08b8ab

version: Introduce LLMQ_DATA_MESSAGES_VERSION for QGETDATA/QDATA support

4768c5e

test: Bump MY_VERSION to 70219 (LLMQ_DATA_MESSAGES_VERSION)

2fedc66

llmq: Introduce CQuorumDataRequest as wrapper for QGETDATA requests

4a1c833

llmq: Implement CQuorum::{SetVerificationVector, SetSecretKeyShare}

2bc6534

llmq|net|protocol: Implement QGETDATA/QDATA P2P messages

987f210

llmq: Restrict processing QGETDATA/QDATA to masternodes only

6b5d6d7

llmq: Implement request limiting for QGETDATA/QDATA

74aecc0

llmq: Implement CQuorumManger::RequestQuorumData

1c70e1d

PastaPastaPasta reviewed Jan 26, 2021

View reviewed changes

adjust some line wrapping to 80 chars

c97f1a1

Signed-off-by: pasta <pasta@dashboost.org>

UdjinM6 previously approved these changes Jan 27, 2021

View reviewed changes

PastaPastaPasta and others added 9 commits January 28, 2021 01:33

tests: Seperate out into dif atomic methods, add logging

9e224ec

Signed-off-by: pasta <pasta@dashboost.org>

test: Avoid restarting masternodes, just let available requests expire

e562313

Just takes a lot time and isn't required imo.

test: Drop redundant code/tests after separation

09e4b3b

This was introduced in 9e224ec

test: Rename test_ratelimiting_banscore -> test_request_limit

672afa4

test: Apply python style

ea30c19

test: Wrap all at 120 characters

e59e034

Thats the default "draw annoying warnings" setting for PyCharm (and IMO a reasonable line length).

test: Move some variables

224a98a

test: Optimize for speed

be7c66d

xdustinface dismissed UdjinM6’s stale review via be7c66d January 28, 2021 03:39

PastaPastaPasta reviewed Jan 28, 2021

View reviewed changes

tests: use wait_until in get_mininode_id

cc38df2

test: Don't use != to check for None

3eb405a

UdjinM6 approved these changes Jan 28, 2021

View reviewed changes

PastaPastaPasta approved these changes Jan 28, 2021

View reviewed changes

PastaPastaPasta merged commit 21cfb4c into dashpay:develop Jan 28, 2021

thephez mentioned this pull request Mar 10, 2021

DIP 21: LLMQ DKG Data Sharing dashpay/dips#87

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

llmq|rpc|test|version: Implement P2P messages QGETDATA <-> QDATA #3953

llmq|rpc|test|version: Implement P2P messages QGETDATA <-> QDATA #3953

xdustinface commented Jan 19, 2021 •

edited

Loading

xdustinface commented Jan 25, 2021

UdjinM6 left a comment

xdustinface commented Jan 25, 2021

PastaPastaPasta commented Jan 26, 2021 •

edited

Loading

PastaPastaPasta Jan 26, 2021

xdustinface Jan 26, 2021

xdustinface commented Jan 26, 2021

UdjinM6 left a comment

PastaPastaPasta commented Jan 27, 2021

xdustinface commented Jan 28, 2021

PastaPastaPasta left a comment

UdjinM6 commented Jan 28, 2021

xdustinface commented Jan 28, 2021 •

edited

Loading

xdustinface commented Jan 28, 2021 •

edited

Loading

UdjinM6 left a comment

xdustinface commented Jan 28, 2021 •

edited

Loading

UdjinM6 commented Jan 28, 2021

PastaPastaPasta left a comment

llmq|rpc|test|version: Implement P2P messages QGETDATA <-> QDATA #3953

llmq|rpc|test|version: Implement P2P messages QGETDATA <-> QDATA #3953

Conversation

xdustinface commented Jan 19, 2021 • edited Loading

xdustinface commented Jan 25, 2021

UdjinM6 left a comment

Choose a reason for hiding this comment

xdustinface commented Jan 25, 2021

PastaPastaPasta commented Jan 26, 2021 • edited Loading

PastaPastaPasta Jan 26, 2021

Choose a reason for hiding this comment

xdustinface Jan 26, 2021

Choose a reason for hiding this comment

xdustinface commented Jan 26, 2021

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Jan 27, 2021

xdustinface commented Jan 28, 2021

PastaPastaPasta left a comment

Choose a reason for hiding this comment

UdjinM6 commented Jan 28, 2021

xdustinface commented Jan 28, 2021 • edited Loading

xdustinface commented Jan 28, 2021 • edited Loading

UdjinM6 left a comment

Choose a reason for hiding this comment

xdustinface commented Jan 28, 2021 • edited Loading

UdjinM6 commented Jan 28, 2021

PastaPastaPasta left a comment

Choose a reason for hiding this comment

xdustinface commented Jan 19, 2021 •

edited

Loading

PastaPastaPasta commented Jan 26, 2021 •

edited

Loading

xdustinface commented Jan 28, 2021 •

edited

Loading

xdustinface commented Jan 28, 2021 •

edited

Loading

xdustinface commented Jan 28, 2021 •

edited

Loading