Implement LLMQ based ChainLocks by codablock · Pull Request #2643 · dashpay/dash

codablock · 2019-01-23T17:00:07Z

It currently only supports a single attempt and either fails or succeeds. Support for multiple attempts as described in the DIP will be added later.

Turned out there were also some errors in DIP8, which I'm going to fix in a few minutes. For example, attemptNum should not have been part of CLSIG and the verification of the signature in CLSIG is described wrong.

codablock · 2019-01-23T17:03:15Z

See dashpay/dips#39 for the fixes in DIP8

UdjinM6

👍

few comments/suggestions below

PastaPastaPasta · 2019-01-23T21:53:54Z

We should limit this to a certain height, otherwise a bug in CL code etc could invalidate the entire chain or something crazy like that

Hmm not sure if that would really help. If such a bug happens and we stop the invalidation at some point (e.g. -100 blocks)...how would this help the user of the node? In that case the node is in an undefined state nevertheless and you can't continue using that node.

I'm thinking of more of a network wide error, instead of a single client

PastaPastaPasta · 2019-01-23T22:06:03Z

I know with relaying txs we have a delay for privacy reasons. Is there any delay for relaying CL messages? I would think there shouldn't be (also applies to IS lock messages)

codablock · 2019-01-24T06:24:16Z

@PastaPastaPasta No there is no delay for relaying CLSIG messages. The trickle logic only applies to TXs and only for non-MN nodes.

codablock · 2019-01-24T07:05:25Z

Handled review comments and pushed new version.

UdjinM6

Looks good 👍

I have just one question #2643 (comment) i.e. do we need smth like 7c3f311 or should we just store the best chainlock only since this seems to be enough.

codablock · 2019-01-24T13:20:01Z

@UdjinM6 @PastaPastaPasta After the discussion in the comments and the discussion we had on Slack, I decided to reimplement CLSIG so that it commits to the height instead of prevBlockHash. It will also only store and propagate the current best CLSIG from now on and ignore older ones.

I also added a changes so that quorum selection happens based on a block that 8 blocks in the past. Otherwise receivers of CLSIG would not be able to verify the CLSIG before the header for that block arrives, which is not guaranteed to happen in a consistent order.

I will squash the changes into the initial commits to get a clean history after getting approvals. As I'll not do a rebase at the same time, it should then be easy to verify that no other changes came in so that re-ACKs can be given shortly after that.

I'll also update DIP8 later.

PastaPastaPasta · 2019-01-24T14:25:10Z

Should this increase misbehaving score?

I decided to not increase DoS score here as you don't know if it's the new CLSIG or the old CLSIG that's the misbehaving one. The peer that sends the bad CLSIG might just be unlucky and accepted it because it didn't know better (e.g. after start or because it missed the other one)

Shouldn't it be fairly easy to tell if it's a valid but old CLSIG or malicious?

Not really. Let's say there are 2 sigs for the same height, but both for different blocks. Node A receives S1 first, node B receives S2 first. Both will now propagate their sig to the other node, resulting in BOTH nodes believing that the other one is malicious. In such cases, it's better to not increase DoS score.

But there should never be two different CLSIGs for the same height... unless there is a fork eight blocks back (since that is what the quorum is based on)

Even such a small fork should not create conflicting CLSIGs. The fork would have to be at least as long as a quorum interval for the ChainLocks quorum is. That would have to be a 288 (12 hours) blocks long fork. And even then, the chance is only 25% to get a conflicting CLSIG (because 3 out of the 4 active quorums are still the same).

So yes, in practice there should never be 2 conflicting CLSIGs.

I mean, if it would take a fork of over 12 hours to get a conflicting CLSIG I think it is safe to assume anyone who sends one is malicious (or you are messed up locally)

UdjinM6

see inline comments

codablock · 2019-01-24T14:53:01Z

@UdjinM6 applied your suggestions

changes were applied

codablock · 2019-01-28T07:19:50Z

I've just squashed all changes which I've done after the initial PR into the original commits to get a clean history again. You can verify that the code has no changes by running:
git diff bf96814c0d4e bcff2006e (should give no output)
bf96814c0d4e14fd35fb7c43a41f756756e14fd3 is the last pushed commit from a few days ago. bcff2006e is the rebased/squashed and force-pushed commit.

…P messages

…ased

Otherwise nodes might already fail to verify sigs when just 1 block away from the tip.

Allows to cheaply find all possible children of a block.

Also don't set indexDummy.phashBlock in TestBlockValidity anymore.

codablock · 2019-01-28T11:27:21Z

Rebased on develop to include #2652 and added 2 more commits to handle #2643 (comment) and partially #2643 (comment)

UdjinM6 · 2019-01-28T13:20:03Z

llmq-signing.py still fails sometimes it seems but other than that looks good now imo 👍

…expected state

UdjinM6

utACK, the test failure is (most likely) not related

codablock force-pushed the pr_llmq_chainlocks branch from 7964a31 to 49fb7e4 Compare January 23, 2019 17:22

UdjinM6 added this to the 14.0 milestone Jan 23, 2019

UdjinM6 previously requested changes Jan 23, 2019

View reviewed changes

PastaPastaPasta reviewed Jan 23, 2019

View reviewed changes

codablock force-pushed the pr_llmq_chainlocks branch from 0d3b532 to 2dcab73 Compare January 24, 2019 08:32

UdjinM6 reviewed Jan 24, 2019

View reviewed changes

codablock force-pushed the pr_llmq_chainlocks branch from d06496a to 4fd48ae Compare January 24, 2019 10:16

PastaPastaPasta reviewed Jan 24, 2019

View reviewed changes

UdjinM6 reviewed Jan 24, 2019

View reviewed changes

Comment thread src/llmq/quorums_chainlocks.cpp Outdated

Comment thread src/llmq/quorums_chainlocks.cpp Outdated

Comment thread src/llmq/quorums_chainlocks.cpp Outdated

Comment thread src/llmq/quorums_chainlocks.cpp Outdated

Comment thread src/llmq/quorums_chainlocks.h Outdated

UdjinM6 reviewed Jan 25, 2019

View reviewed changes

Comment thread src/llmq/quorums_chainlocks.cpp Outdated

Comment thread src/llmq/quorums_chainlocks.cpp Outdated

Comment thread src/llmq/quorums_chainlocks.cpp Outdated

UdjinM6 requested changes Jan 26, 2019

View reviewed changes

Comment thread src/validation.cpp Outdated

Comment thread src/validation.cpp Outdated

UdjinM6 reviewed Jan 26, 2019

View reviewed changes

Comment thread src/llmq/quorums_chainlocks.h Outdated

codablock force-pushed the pr_llmq_chainlocks branch from bf96814 to 828872c Compare January 28, 2019 07:18

codablock force-pushed the pr_llmq_chainlocks branch from 828872c to bcff200 Compare January 28, 2019 07:21

codablock added 8 commits January 28, 2019 12:24

Add missing new-line character in log output

1385567

Don't pass poolSize to SelectQuorum and instead use consensus params

189cee2

Add listener interface to listen for recovered sigs

9f211ef

Implement VerifyRecoveredSig to allow verifcation of sigs found in P2…

4026ea2

…P messages

Move SelectQuorumForSigning into CSigningManager and make it height b…

cf33efc

…ased

Use a block that is 8 blocks in the past for SelectQuorumForSigning

04a51c9

Otherwise nodes might already fail to verify sigs when just 1 block away from the tip.

Track parent->child relations for blocks

2bf6eb1

Allows to cheaply find all possible children of a block.

Implement and enforce ChainLocks

29532ba

codablock added 6 commits January 28, 2019 12:24

Add SPORK_19_CHAINLOCKS_ENABLED

135829d

Add info about ChainLocks to block and transaction RPCs

3413ff9

Implement llmq-chainlocks.py integration tests

886299a

Increase waiting time in LLMQ signing tests

08d915d

Don't check for conflicting ChainLocks when phashBlock is not set

5033d5e

Also don't set indexDummy.phashBlock in TestBlockValidity anymore.

Rename inInvalidate->inEnforceBestChainLock and make it atomic

3237668

codablock force-pushed the pr_llmq_chainlocks branch from 7177391 to 3237668 Compare January 28, 2019 11:24

Instead of asserting signatures once, wait for them to change to the …

ad4b3cd

…expected state

codablock force-pushed the pr_llmq_chainlocks branch from 954a882 to ad4b3cd Compare January 28, 2019 13:45

UdjinM6 approved these changes Jan 28, 2019

View reviewed changes

codablock merged commit f6828b1 into dashpay:develop Jan 29, 2019

codablock deleted the pr_llmq_chainlocks branch January 29, 2019 14:59

UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Mar 5, 2019

UdjinM6 added the P2P Some notable changes on p2p level label Mar 20, 2019

Conversation

codablock commented Jan 23, 2019

Uh oh!

codablock commented Jan 23, 2019

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

PastaPastaPasta commented Jan 23, 2019

Uh oh!

codablock commented Jan 24, 2019

Uh oh!

codablock commented Jan 24, 2019

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

codablock commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codablock commented Jan 24, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codablock commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codablock commented Jan 28, 2019

Uh oh!

UdjinM6 commented Jan 28, 2019

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

codablock commented Jan 24, 2019 •

edited

Loading

codablock commented Jan 28, 2019 •

edited

Loading