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

Implement LLMQ DKG #2617

Merged
merged 21 commits into from
Jan 15, 2019
Merged

Implement LLMQ DKG #2617

merged 21 commits into from
Jan 15, 2019

Conversation

codablock
Copy link

This is extracted from #2311 and only contains the DKG. It also removes the dummy DKG in the first commit.

It does not contain the signing sessions code as I'm planning to create another PR for this.

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.

Just some general comments for now, will take some time to dig in and do a full review 😅

src/batchedlogger.h Outdated Show resolved Hide resolved
src/consensus/params.h Outdated Show resolved Hide resolved
src/llmq/quorums_dkgsession.cpp Outdated Show resolved Hide resolved
@@ -110,6 +110,7 @@ BITCOIN_CORE_H = \
core_memusage.h \
cuckoocache.h \
ctpl.h \
cxxtimer.hpp \
Copy link

Choose a reason for hiding this comment

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

Why using some new library for timers instead of simply

int64_t nTimeStart = GetTimeMillis();
// some code
LogPrintf("Some code took %dms\n", GetTimeMillis() - nTimeStart);

like we already do?

Copy link
Author

Choose a reason for hiding this comment

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

Because a class for timers makes using them a lot easier, especially when you have multiple timers per method and need to start/stop them in different places. I initially thought about implementing my own timer class, but then found this header-only "library" and took it instead.

src/rpc/register.h Show resolved Hide resolved
src/rpc/register.h Show resolved Hide resolved
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.

2 suggestions to fix compilation issues

src/llmq/quorums_debug.cpp Outdated Show resolved Hide resolved
src/llmq/quorums_debug.h Outdated Show resolved Hide resolved
src/consensus/params.h Outdated Show resolved Hide resolved
Nodes started randomly voting other members as bad members because
contributions were not received fast enough.
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.

I have few patches and one question so far :)

  • 4ead34438 Batched logger should not break log parsing
  • e4c1b02da Add some consts
  • ed00a9761 some trivial cleanup

src/net.cpp Show resolved Hide resolved
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.

Few more small suggestions

src/rpc/rpcquorums.cpp Outdated Show resolved Hide resolved
src/rpc/rpcquorums.cpp Outdated Show resolved Hide resolved
src/rpc/rpcquorums.cpp Outdated Show resolved Hide resolved
UdjinM6 and others added 3 commits January 11, 2019 07:07
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.

A couple of suggestions to fix quorum dkgstatus help message format to match the one used in other places and a question regarding "0", see below.

src/rpc/rpcquorums.cpp Outdated Show resolved Hide resolved
src/rpc/rpcquorums.cpp Outdated Show resolved Hide resolved
src/rpc/rpcquorums.cpp Show resolved Hide resolved
src/rpc/rpcquorums.cpp Outdated Show resolved Hide resolved
@UdjinM6
Copy link

UdjinM6 commented Jan 11, 2019

Suggesting another few patches after the discussion in previous ones :)

  • 0114a531a c3ade4a6a Replace Seen with AddSeenMessage and move its use out of PreVerifyMessage
  • f003c1ed9 1669d6980 Add some consts (modified version)

EDIT: forgot to pull the latest changes first, fixed

@codablock
Copy link
Author

@UdjinM6 Can you explain why c3ade4a moves the AddSeenMessage calls into SendContributions and friends? Did you try to remove the Seen logic completely and then observed double-verification/processing of the messages created by the local node?

@UdjinM6
Copy link

UdjinM6 commented Jan 11, 2019

I just wanted to split "seen" and actual (pre)verification steps, cause it seems like they should not be coupled. Or am I missing smth?

@codablock
Copy link
Author

Hmm, problem with that is that it doesn't do what one might expect. AddSeenMessage is now only called for the self-created message and never for one that is received. This will still work, as CDKGPendingMessages has another layer of protection against duplicate messages, but it's confusing and unexpected.

I added a commit that does a completely different thing: Instead of calling PreVerify+ReceiveMessage, SendContribution and friends will now push the message into the pendingMessages object so that the messages are processed the same way as actually received messages. This adds some overhead due to unnecessary serialization/deserialization, but I think that's ok (it's just one out of up to 400 messages). The advantage is that messages now always run through the same code path and the "seen messages" logic is applied equally to self created and received messages.

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.

Looks good IMO, though I'm still a bit confused with all the crypto magic happening here :D

utACK

@UdjinM6 UdjinM6 merged commit 4671c98 into dashpay:develop Jan 15, 2019
@codablock codablock deleted the pr_llmq_dkg branch January 15, 2019 12:53
@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Mar 5, 2019
@UdjinM6 UdjinM6 added the P2P Some notable changes on p2p level label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2P Some notable changes on p2p level RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants