feat(rpc): submit chainlock signature if needed RPC #5765

ogabrielides · 2023-12-13T23:32:23Z

Issue being fixed or feature implemented

Once Platform is live, there could be an edge case where the CL could arrive to an EvoNode faster through Platform quorum than regular P2P propagation.

What was done?

This PR introduces a new RPC submitchainlock with the following 3 mandatory parameters:

blockHash, signature and height.

Besides some basic tests:

If the block is unknown then ~~the RPC returns an error (could happen if the node is stucked)~~ the CL still proceed, RPC returns true and CL is broadcasted
If the signature is not verified then the RPC return an error.
If the node already has this CL, the RPC returns true.
If the node doesn't have this CL, it inserts it, broadcast it through the inv system and return true.

How Has This Been Tested?

feature_llmq_chainlocks.py was modified with the following scenario:

node0 is isolated from the rest of the network
node1 mines a new block and waits for CL
Make sure node0 doesn't know the new block/CL (by checking getbestchainlock())
CL is submitted via the new RPC on node0
checking getbestchainlock() and make sure the CL was processed + 'known_block' is false
reconnect node0

Breaking Changes

no

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)

PastaPastaPasta · 2023-12-14T00:30:10Z

submitchainlock would make this more in line with other RPCs such as submitblock or submitrawtransaction

UdjinM6

there could be an edge case where the CL could arrive to an EvoNode faster through Platform quorum than regular P2P propagation.

But why is this an issue exactly? Why do we want to submit chainlock ourselves instead of maybe verifying it via verifychainlock and allowing the node to receive it the usual way?

Also, that's a lot of code duplication with verifychainlock but for some reason behaviour differs in some parts. Why is that?

src/rpc/quorums.cpp

PastaPastaPasta · 2023-12-14T16:12:47Z

What if we simplify this down to

Subject: [PATCH] simplify everything
---
Index: src/rpc/quorums.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/rpc/quorums.cpp b/src/rpc/quorums.cpp
--- a/src/rpc/quorums.cpp	(revision 9b401107067b73eb0719813783ae150ae008f2da)
+++ b/src/rpc/quorums.cpp	(date 1702570318089)
@@ -1007,49 +1007,23 @@
 
     const uint256 nBlockHash(ParseHashV(request.params[0], "blockHash"));
 
-    const NodeContext& node = EnsureAnyNodeContext(request.context);
-    const ChainstateManager& chainman = EnsureChainman(node);
-
     const int nBlockHeight = ParseInt32V(request.params[2], "blockHeight");
-    CBlockIndex* pIndex{nullptr};
-    {
-        LOCK(cs_main);
-        if (nBlockHeight < 0) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid block height");
-        }
-        if (nBlockHeight > chainman.ActiveChain().Height()) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "future block");
-        }
-        pIndex = chainman.ActiveChain()[nBlockHeight];
-        if (nBlockHash != pIndex->GetBlockHash()) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid blockhash");
-        }
+    if (nBlockHeight <= 0) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid block height");
     }
 
     CBLSSignature sig;
-    if (pIndex) {
-        bool use_legacy_signature = !llmq::utils::IsV19Active(pIndex);
-        if (!sig.SetHexStr(request.params[1].get_str(), use_legacy_signature)) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
-        }
-    } else {
-        if (!sig.SetHexStr(request.params[1].get_str(), false) &&
-            !sig.SetHexStr(request.params[1].get_str(), true)
-                ) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
-        }
+    if (!sig.SetHexStr(request.params[1].get_str(), false) &&
+        !sig.SetHexStr(request.params[1].get_str(), true) ) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature format");
     }
 
-    const LLMQContext& llmq_ctx = EnsureLLMQContext(node);
-    llmq::CChainLockSig clsig = llmq::CChainLockSig(nBlockHeight, nBlockHash, sig);
+    const LLMQContext& llmq_ctx = EnsureLLMQContext(EnsureAnyNodeContext(request.context));
+    auto clsig = llmq::CChainLockSig(nBlockHeight, nBlockHash, sig);
     if (!llmq_ctx.clhandler->VerifyChainLock(clsig)) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "invalid signature");
     }
 
-    if (llmq_ctx.clhandler->HasChainLock(nBlockHeight, nBlockHash)) {
-        return true;
-    }
-
     llmq_ctx.clhandler->ProcessNewChainLock(-1, clsig, ::SerializeHash(clsig));
     return true;
 }

ogabrielides · 2023-12-14T17:41:31Z

@UdjinM6

there could be an edge case where the CL could arrive to an EvoNode faster through Platform quorum than regular P2P propagation.

But why is this an issue exactly? Why do we want to submit chainlock ourselves instead of maybe verifying it via verifychainlock and allowing the node to receive it the usual way?

Also, that's a lot of code duplication with verifychainlock but for some reason behaviour differs in some parts. Why is that?

I will try to answer to all of your comments here.

But why is this an issue exactly? Why do we want to submit chainlock ourselves instead of maybe verifying it via verifychainlock and allowing the node to receive it the usual way?

Regarding Core only, this is indeed not an issue. But if we see Core+Platform as a system, this is a race condition for Chainlock signature. This PR will help Platform avoid waiting for Core to get the CL from regular propagation.

We usually relay "future" chainlocks (check ProcessNewChainLock). Why is it not the case here?

Didn't know that. All right, will change it. That means that height can be optional yes.

ogabrielides · 2023-12-14T21:06:27Z

@UdjinM6 Applied @PastaPastaPasta 's suggestions.
It still feels weird to have optional height and accept CL for "future" blocks.

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

test/functional/feature_llmq_chainlocks.py

src/rpc/quorums.cpp

src/rpc/server.cpp

test/functional/rpc_platform_filter.py

test/functional/feature_llmq_chainlocks.py

UdjinM6

utACK

UdjinM6 · 2023-12-16T17:55:04Z

Not sure what's going on but Gitlab CI just won't start here for some reason...

EDIT: pushed the branch manually https://gitlab.com/dashpay/dash/-/pipelines/1109351723

UdjinM6

re-utACK

ogabrielides · 2023-12-16T21:23:25Z

re-utACK

Thanks for approving your changes 😃

PastaPastaPasta

utACK for squash merge

PastaPastaPasta

Sorry; needs release notes first; otherwise looks good

knst

LGTM

knst

LGTM

thephez

Small typo fix in release notes

doc/release-notes-5765.md

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

thephez

👍

UdjinM6

utACK

PastaPastaPasta

utACK for squash merge

QuantumExplorer · 2024-01-08T08:10:01Z

So we have If the block is unknown then the RPC returns an error (could happen if the node is stucked) If the signature is not verified then the RPC return an error.

Is it really an error if the block is unknown?, Shouldn't we just be returning false? From platform's perspective this is not an error, and while we do vote down the proposal, this is not an "error" pathway.

knst · 2024-01-08T08:27:44Z

Is it really an error if the block is unknown?, Shouldn't we just be returning false?

sounds reasonable.
So, if we don't have a block with blockHash known yet - we would return false, all other cases are not changed.

QuantumExplorer · 2024-01-08T10:23:22Z

Yes @knst could you make that happen?

knst · 2024-01-08T12:25:34Z

test/functional/feature_llmq_chainlocks.py

+        assert_equal(best_0['signature'], best_1['signature'])
+        assert_equal(best_0['known_block'], False)
+        self.reconnect_isolated_node(0, 1)
+        self.sync_all()


@QuantumExplorer that works not as described in the PR description.
If block is unknown -> CL is still submitted and if it succeeds we return true.

We do not return error if block is not know, we just quietly process it, verify, re-transmit to network and add to known chainlocks.
Nothing to change already.

here's even tests for it.

QuantumExplorer · 2024-01-09T03:38:56Z

@knst Part of the requirement is that it return true or false based on whether it had the block (so we didn't have to call another RPC), right now it can't ever return false.

PastaPastaPasta · 2024-01-09T04:48:21Z

Talked to Sam; we want change the API in this way
First (if height <= bestchainlockheight return bestchainlockheight)
else submitchainlock then return bestchainlockheight

This provides two benefits; 1st is a short circuiting if we know the proposed chainlock could not overwrite the current one (due to lower height)
Second; returns the bestchainlockheight to platform

addchainlock rpc

197c7f3

ogabrielides changed the title ~~feat(rpc): add chainlock signature if needed RPC.~~ feat(rpc): add chainlock signature if needed RPC Dec 13, 2023

ogabrielides added the RPC Some notable changes to RPC params/behaviour/descriptions label Dec 13, 2023

ogabrielides added this to the 20.1 milestone Dec 13, 2023

fix for rpc_platform_filter

4bb3bfa

ogabrielides changed the title ~~feat(rpc): add chainlock signature if needed RPC~~ feat(rpc): submit chainlock signature if needed RPC Dec 14, 2023

renamed rpc to submitchainlock

9b40110

ogabrielides requested review from UdjinM6, knst and PastaPastaPasta December 14, 2023 10:00

UdjinM6 requested changes Dec 14, 2023

View reviewed changes

src/rpc/quorums.cpp Outdated Show resolved Hide resolved

src/rpc/quorums.cpp Outdated Show resolved Hide resolved

src/rpc/quorums.cpp Outdated Show resolved Hide resolved

src/rpc/quorums.cpp Outdated Show resolved Hide resolved

src/rpc/quorums.cpp Outdated Show resolved Hide resolved

ogabrielides added 2 commits December 14, 2023 22:54

suggestions

0c34c2c

suggestions

8997a33

ogabrielides requested a review from UdjinM6 December 14, 2023 21:06

Update src/rpc/quorums.cpp

7bb8ecd

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

UdjinM6 requested changes Dec 15, 2023

View reviewed changes

test/functional/feature_llmq_chainlocks.py Outdated Show resolved Hide resolved

src/rpc/quorums.cpp Outdated Show resolved Hide resolved

src/rpc/server.cpp Show resolved Hide resolved

test/functional/rpc_platform_filter.py Outdated Show resolved Hide resolved

knst reviewed Dec 15, 2023

View reviewed changes

test/functional/feature_llmq_chainlocks.py Show resolved Hide resolved

suggestions

d1b742f

ogabrielides requested review from UdjinM6 and knst December 16, 2023 16:34

UdjinM6 previously approved these changes Dec 16, 2023

View reviewed changes

fix test

cfd44b1

UdjinM6 dismissed their stale review via cfd44b1 December 16, 2023 17:49

fix linter

554b4af

UdjinM6 previously approved these changes Dec 16, 2023

View reviewed changes

PastaPastaPasta approved these changes Dec 17, 2023

View reviewed changes

PastaPastaPasta requested changes Dec 17, 2023

View reviewed changes

knst previously approved these changes Dec 17, 2023

View reviewed changes

Create release-notes-5765.md

ed11f0c

ogabrielides dismissed stale reviews from knst and UdjinM6 via ed11f0c December 18, 2023 08:07

ogabrielides requested review from PastaPastaPasta, UdjinM6 and knst December 18, 2023 08:10

knst previously approved these changes Dec 18, 2023

View reviewed changes

thephez reviewed Dec 18, 2023

View reviewed changes

doc/release-notes-5765.md Outdated Show resolved Hide resolved

ogabrielides dismissed knst’s stale review via 7917ff3 December 18, 2023 13:40

Update doc/release-notes-5765.md

7917ff3

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

ogabrielides requested a review from thephez December 18, 2023 13:40

thephez approved these changes Dec 18, 2023

View reviewed changes

UdjinM6 approved these changes Dec 18, 2023

View reviewed changes

PastaPastaPasta approved these changes Dec 19, 2023

View reviewed changes

PastaPastaPasta merged commit 3bc77a6 into dashpay:develop Dec 19, 2023
9 checks passed

ogabrielides deleted the rpc_insert_clsig branch December 19, 2023 10:28

knst reviewed Jan 8, 2024

View reviewed changes

PastaPastaPasta mentioned this pull request Jan 9, 2024

feat: Adjust API of submitchainlock #5805

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rpc): submit chainlock signature if needed RPC #5765

feat(rpc): submit chainlock signature if needed RPC #5765

ogabrielides commented Dec 13, 2023 •

edited by knst

Loading

PastaPastaPasta commented Dec 14, 2023

UdjinM6 left a comment

PastaPastaPasta commented Dec 14, 2023

ogabrielides commented Dec 14, 2023

ogabrielides commented Dec 14, 2023

UdjinM6 left a comment

UdjinM6 commented Dec 16, 2023 •

edited

Loading

UdjinM6 left a comment

ogabrielides commented Dec 16, 2023

PastaPastaPasta left a comment

PastaPastaPasta left a comment

knst left a comment

knst left a comment

thephez left a comment

thephez left a comment

UdjinM6 left a comment

PastaPastaPasta left a comment

QuantumExplorer commented Jan 8, 2024

knst commented Jan 8, 2024

QuantumExplorer commented Jan 8, 2024

knst Jan 8, 2024 •

edited

Loading

QuantumExplorer commented Jan 9, 2024

PastaPastaPasta commented Jan 9, 2024

feat(rpc): submit chainlock signature if needed RPC #5765

feat(rpc): submit chainlock signature if needed RPC #5765

Conversation

ogabrielides commented Dec 13, 2023 • edited by knst Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

PastaPastaPasta commented Dec 14, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta commented Dec 14, 2023

ogabrielides commented Dec 14, 2023

ogabrielides commented Dec 14, 2023

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 commented Dec 16, 2023 • edited Loading

UdjinM6 left a comment

Choose a reason for hiding this comment

ogabrielides commented Dec 16, 2023

PastaPastaPasta left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

knst left a comment

Choose a reason for hiding this comment

thephez left a comment

Choose a reason for hiding this comment

thephez left a comment

Choose a reason for hiding this comment

UdjinM6 left a comment

Choose a reason for hiding this comment

PastaPastaPasta left a comment

Choose a reason for hiding this comment

QuantumExplorer commented Jan 8, 2024

knst commented Jan 8, 2024

QuantumExplorer commented Jan 8, 2024

knst Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

QuantumExplorer commented Jan 9, 2024

PastaPastaPasta commented Jan 9, 2024

ogabrielides commented Dec 13, 2023 •

edited by knst

Loading

UdjinM6 commented Dec 16, 2023 •

edited

Loading

knst Jan 8, 2024 •

edited

Loading