feat: split type of error in submitchainlock - return enum in CL verifying code #6096

knst · 2024-07-05T09:50:21Z

Issue being fixed or feature implemented

Currently by result of submitchainlock impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet.

This PR aims to fix this issue, as requested by shumkov for needs of platform:

mailformed signature and can’t verify signature due to unknown quorum is the same error?
possible to distingush ?

What was done?

Return enum in CL verifying code chainlock_handler.VerifyChainLock.
The RPC submitchainlock now returns error with code=-1 and message no quorum found. Current tip height: {N} hash: {HASH}

How Has This Been Tested?

Functional test feature_llmq_chainlocks.py is updated

Breaking Changes

submitchainlock return one more error code - not really a breaking change though, because v21 hasn't released yet.

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

DashCoreAutoGuix · 2024-07-05T09:54:04Z

Guix Automation has began to build this PR tagged as v21.0.0-devpr6096.c36c8c71. A new comment will be made when the image is pushed.

DashCoreAutoGuix · 2024-07-05T10:51:28Z

Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v21.0.0-devpr6096.c36c8c71. The image should be on dockerhub soon.

PastaPastaPasta · 2024-07-06T17:57:45Z

Please object if there is a reason for this to be back ported to v21.0

UdjinM6 · 2024-07-06T10:58:15Z

src/llmq/chainlocks.cpp

+    // Verify a recovered sig that was signed while the chain tip was at signedAtTip
+    const auto& llmq_params_opt = Params().GetLLMQ(llmqType);
+    assert(llmq_params_opt.has_value());
+    auto quorum = SelectQuorumForSigning(llmq_params_opt.value(), m_chainstate.m_chain, qman, nRequestId, clsig.getHeight(), SIGN_HEIGHT_OFFSET);
+    if (!quorum) {
+        return VerifyCLStatus::NoQuorum;
+    }
+
+    uint256 signHash = BuildSignHash(llmqType, quorum->qc->quorumHash, nRequestId, clsig.getBlockHash());
+    bool ret = clsig.getSig().VerifyInsecure(quorum->qc->quorumPublicKey, signHash);


Could probably modify VerifyRecoveredSig to return error/status codes instead of reimplementing its logic here. Thoughts?

because logically type "status of recovered sig verification" and "status of chainlock verifications" are on the different abstraction levels, without bunch of boiler code it makes new circular dependencies; also the function chainlock_handler.VerifyChainLock should not return recovery sig verification status as it is just simple wrong level of abstraction. Let me think how to refactor it better, but at current moment this is the best approach

Actually that's not as bad as I described. Done:

@knst knst force-pushed the feat-submitchainlock-ret branch from c36c8c7 to 8728464

shumkov · 2024-07-08T06:28:31Z

@PastaPastaPasta Currently, we have a bug in Drive when we submit chainlock we get the same error in case if Core is not synced up to the height when this CL quorum is created or if a malicious proposer sends a malformed signature. In the first case, we would like to wait a bit and try again. In the second case, we want to reject a proposal immediately. It opens an attack vector if we wait in both cases or often leads to multiple rounds per height if we reject blocks for both.

UdjinM6

few small suggestions

UdjinM6 · 2024-07-08T14:59:45Z

src/rpc/quorums.cpp

+    //for (int signOffset : {0, llmq_params.dkgInterval}) {
+    const int signOffset{llmq_params.dkgInterval};
+
+    return llmq::VerifyRecoveredSig(llmq_params.type, active_chain, qman, signHeight, id, msgHash, sig, 0) == llmq::VerifyRecSigStatus::Valid ||
+           llmq::VerifyRecoveredSig(llmq_params.type, active_chain, qman, signHeight, id, msgHash, sig, signOffset) == llmq::VerifyRecSigStatus::Valid;


Suggested change

//for (int signOffset : {0, llmq_params.dkgInterval}) {

const int signOffset{llmq_params.dkgInterval};

return llmq::VerifyRecoveredSig(llmq_params.type, active_chain, qman, signHeight, id, msgHash, sig, 0) == llmq::VerifyRecSigStatus::Valid ||

llmq::VerifyRecoveredSig(llmq_params.type, active_chain, qman, signHeight, id, msgHash, sig, signOffset) == llmq::VerifyRecSigStatus::Valid;

// First check against the current active set, if it fails check against the last active set

return llmq::VerifyRecoveredSig(llmq_params.type, active_chain, qman, signHeight, id, msgHash, sig, 0) == llmq::VerifyRecSigStatus::Valid ||

llmq::VerifyRecoveredSig(llmq_params.type, active_chain, qman, signHeight, id, msgHash, sig, llmq_params.dkgInterval) == llmq::VerifyRecSigStatus::Valid;

UdjinM6 · 2024-07-08T15:00:53Z

src/rpc/quorums.cpp

@@ -546,9 +556,7 @@ static RPCHelpMan quorum_verify()
            signHeight = ParseInt32V(request.params[5], "signHeight");
        }
        // First check against the current active set, if it fails check against the last active set


Suggested change

// First check against the current active set, if it fails check against the last active set

UdjinM6 · 2024-07-08T15:01:05Z

src/rpc/quorums.cpp

@@ -1033,9 +1041,7 @@ static RPCHelpMan verifyislock()
    // First check against the current active set, if it fails check against the last active set


Suggested change

// First check against the current active set, if it fails check against the last active set

UdjinM6 · 2024-07-08T15:01:35Z

src/rpc/quorums.cpp

+        LOCK(cs_main);
+        const ChainstateManager& chainman = EnsureChainman(node);
+        const CBlockIndex* pIndex{chainman.ActiveChain().Tip()};
+        throw JSONRPCError(RPC_MISC_ERROR, strprintf("no quorum found. Current tip height: %d hash: %s\n", pIndex->nHeight, pIndex->GetBlockHash().ToString()));


Suggested change

throw JSONRPCError(RPC_MISC_ERROR, strprintf("no quorum found. Current tip height: %d hash: %s\n", pIndex->nHeight, pIndex->GetBlockHash().ToString()));

throw JSONRPCError(RPC_MISC_ERROR, strprintf("No quorum found. Current tip height: %d hash: %s\n", pIndex->nHeight, pIndex->GetBlockHash().ToString()));

UdjinM6 · 2024-07-08T15:01:47Z

test/functional/feature_llmq_chainlocks.py

@@ -84,11 +84,14 @@ def run_test(self):
            block = self.nodes[0].getblock(self.nodes[0].getblockhash(h))
            assert block['chainlock']

-        # Update spork to SPORK_19_CHAINLOCKS_ENABLED and test its behaviour
+        self.log.info(f"Test submitchainlock for too high block")
+        assert_raises_rpc_error(-1, f"no quorum found. Current tip height: {self.nodes[1].getblockcount()}", self.nodes[1].submitchainlock, '0000000000000000000000000000000000000000000000000000000000000000', 'a5c69505b5744524c9ed6551d8a57dc520728ea013496f46baa8a73df96bfd3c86e474396d747a4af11aaef10b17dbe80498b6a2fe81938fe917a3fedf651361bfe5367c800d23d3125820e6ee5b42189f0043be94ce27e73ea13620c9ef6064', self.nodes[1].getblockcount() + 300)


Suggested change

assert_raises_rpc_error(-1, f"no quorum found. Current tip height: {self.nodes[1].getblockcount()}", self.nodes[1].submitchainlock, '0000000000000000000000000000000000000000000000000000000000000000', 'a5c69505b5744524c9ed6551d8a57dc520728ea013496f46baa8a73df96bfd3c86e474396d747a4af11aaef10b17dbe80498b6a2fe81938fe917a3fedf651361bfe5367c800d23d3125820e6ee5b42189f0043be94ce27e73ea13620c9ef6064', self.nodes[1].getblockcount() + 300)

assert_raises_rpc_error(-1, f"No quorum found. Current tip height: {self.nodes[1].getblockcount()}", self.nodes[1].submitchainlock, '0000000000000000000000000000000000000000000000000000000000000000', 'a5c69505b5744524c9ed6551d8a57dc520728ea013496f46baa8a73df96bfd3c86e474396d747a4af11aaef10b17dbe80498b6a2fe81938fe917a3fedf651361bfe5367c800d23d3125820e6ee5b42189f0043be94ce27e73ea13620c9ef6064', self.nodes[1].getblockcount() + 300)

…tchainlock It helps to detect case of Tip is way too far behind: the signature can be still valid but core can't verify it yet but later it may become a valid signature.

UdjinM6

utACK 0133c98

PastaPastaPasta

utACK 0133c98

…turn enum in CL verifying code 0133c98 feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov) 6004e06 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov) 130b6d1 refactor: replace static private member method to static method (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Currently by result of `submitchainlock` impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet. This PR aims to fix this issue, as requested by shumkov for needs of platform: > mailformed signature and can’t verify signature due to unknown quorum is the same error? > possible to distingush ? ## What was done? Return enum in CL verifying code `chainlock_handler.VerifyChainLock`. The RPC `submitchainlock` now returns error with code=-1 and message `no quorum found. Current tip height: {N} hash: {HASH}` ## How Has This Been Tested? Functional test `feature_llmq_chainlocks.py` is updated ## Breaking Changes `submitchainlock` return one more error code - not really a breaking change though, because v21 hasn't released yet. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK 0133c98 PastaPastaPasta: utACK 0133c98 Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7

db82817 Merge #6106: feat: create new composite quorum-command platformsign (pasta) a45e6df Merge #6104: fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta) 7330982 Merge #6100: feat: make whitelist works with composite commands for platform needs (pasta) 9998ffd Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code (pasta) cdf7a25 Merge #6095: fix: createwallet to require 'load_on_startup' for descriptor wallets (pasta) c1c2c55 Merge #6092: fix: mixing for partially unlocked descriptor wallets (pasta) 1175486 Merge #6073: feat: add logging for RPC HTTP requests: command, user, http-code, time of running (pasta) Pull request description: ## Issue being fixed or feature implemented Backports a set of 6 PRs needed in rc.2 ## What was done? Backported PRs with labels ## How Has This Been Tested? ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: LGTM, utACK db82817 UdjinM6: utACK db82817 Tree-SHA512: 1b242c5db04bd5873ef622543bc2a25e29567f15962c677ea51ff05cb784291968d18f419bf611c206b912e8f15d687208ae75af33aab89038b6f0167d99c4bf

PastaPastaPasta · 2024-07-23T17:44:51Z

back ported in #6115

98a3393 chore: set release to true (pasta) cd0a3a6 Merge #6154: chore: remove trailing whitespaces in release notes (pasta) 6bc60a7 Merge #6151: chore: update seeds for v21 release (pasta) 88e949a Merge #6146: chore: bump assumevalid, minchainwork, checkpoints, chaintxdata (pasta) cc14427 Merge #6144: docs: release notes for v21.0.0 (pasta) 0a8ece1 Merge #6122: chore: translations 2024-07 (pasta) 146d244 Merge #6140: feat: harden all sporks on mainnet to current values (pasta) 024d272 Merge #6126: feat: enable EHF activation of MN_RR on mainnet (pasta) e780b3d Merge #6125: docs: update manpages for 21.0 (pasta) 5ede23c Merge #6118: docs: add release notes notifying change of default branch to `develop` (pasta) 1b6fe9c Merge #6117: docs: update supported versions in SECURITY.md (pasta) 27d20be Merge #6116: fix: mitigate crashes associated with some upgradetohd edge cases (pasta) db82817 Merge #6106: feat: create new composite quorum-command platformsign (pasta) a45e6df Merge #6104: fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta) 7330982 Merge #6100: feat: make whitelist works with composite commands for platform needs (pasta) 9998ffd Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code (pasta) cdf7a25 Merge #6095: fix: createwallet to require 'load_on_startup' for descriptor wallets (pasta) c1c2c55 Merge #6092: fix: mixing for partially unlocked descriptor wallets (pasta) 1175486 Merge #6073: feat: add logging for RPC HTTP requests: command, user, http-code, time of running (pasta) Pull request description: ## Issue being fixed or feature implemented Suppressed changes from be83865 so the diff is empty. ## What was done? ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: ACK 158cf86; no diff knst: ACK 158cf86 Tree-SHA512: 3310a39fbcb45bdf09f885fe77ba769c0a715869a3bb287eaf0f2cf54b35a7e1f832c88df3bd31097eabf2d375515c1b87ff05e0c3282cef642833a154c42bbe

refactor: replace static private member method to static method

130b6d1

knst added this to the 21 milestone Jul 5, 2024

knst requested review from shumkov, UdjinM6 and PastaPastaPasta July 5, 2024 09:50

knst added the guix-build label Jul 5, 2024

knst removed the guix-build label Jul 5, 2024

PastaPastaPasta modified the milestones: 21, 21.1 Jul 6, 2024

UdjinM6 reviewed Jul 6, 2024

View reviewed changes

knst added the backport-candidate-21.0.x label Jul 8, 2024

knst requested a review from UdjinM6 July 8, 2024 11:47

knst force-pushed the feat-submitchainlock-ret branch from c36c8c7 to 8728464 Compare July 8, 2024 13:04

UdjinM6 reviewed Jul 8, 2024

View reviewed changes

knst added 2 commits July 9, 2024 00:09

feat: return enum in RecoveredSig verifying code, apply for RPC submi…

6004e06

…tchainlock It helps to detect case of Tip is way too far behind: the signature can be still valid but core can't verify it yet but later it may become a valid signature.

feat: add functional test for submitchainlock far ahead in future

0133c98

knst force-pushed the feat-submitchainlock-ret branch from 8728464 to 0133c98 Compare July 8, 2024 17:10

UdjinM6 approved these changes Jul 8, 2024

View reviewed changes

knst changed the title ~~refactor: return enum in CL verifying code, apply for submitchainlock~~ feat: split type of error in submitchainlock - return enum in CL verifying code Jul 9, 2024

PastaPastaPasta approved these changes Jul 9, 2024

View reviewed changes

PastaPastaPasta merged commit 38249a5 into dashpay:develop Jul 9, 2024
8 of 9 checks passed

PastaPastaPasta removed the backport-candidate-21.0.x label Jul 23, 2024

UdjinM6 modified the milestones: 21.1, 21.2 Aug 8, 2024

thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Sep 17, 2024

UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: split type of error in submitchainlock - return enum in CL verifying code #6096

feat: split type of error in submitchainlock - return enum in CL verifying code #6096

knst commented Jul 5, 2024 •

edited

Loading

DashCoreAutoGuix commented Jul 5, 2024

DashCoreAutoGuix commented Jul 5, 2024

PastaPastaPasta commented Jul 6, 2024

UdjinM6 Jul 6, 2024

knst Jul 8, 2024 •

edited

Loading

knst Jul 8, 2024

shumkov commented Jul 8, 2024

UdjinM6 left a comment

UdjinM6 Jul 8, 2024

UdjinM6 Jul 8, 2024

UdjinM6 Jul 8, 2024

UdjinM6 Jul 8, 2024

UdjinM6 Jul 8, 2024

UdjinM6 left a comment

PastaPastaPasta left a comment

PastaPastaPasta commented Jul 23, 2024

		@@ -1033,9 +1041,7 @@ static RPCHelpMan verifyislock()
		// First check against the current active set, if it fails check against the last active set

	throw JSONRPCError(RPC_MISC_ERROR, strprintf("no quorum found. Current tip height: %d hash: %s\n", pIndex->nHeight, pIndex->GetBlockHash().ToString()));
	throw JSONRPCError(RPC_MISC_ERROR, strprintf("No quorum found. Current tip height: %d hash: %s\n", pIndex->nHeight, pIndex->GetBlockHash().ToString()));

feat: split type of error in submitchainlock - return enum in CL verifying code #6096

feat: split type of error in submitchainlock - return enum in CL verifying code #6096

Conversation

knst commented Jul 5, 2024 • edited Loading

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

DashCoreAutoGuix commented Jul 5, 2024

DashCoreAutoGuix commented Jul 5, 2024

PastaPastaPasta commented Jul 6, 2024

UdjinM6 Jul 6, 2024

Choose a reason for hiding this comment

knst Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

knst Jul 8, 2024

Choose a reason for hiding this comment

shumkov commented Jul 8, 2024

UdjinM6 left a comment

Choose a reason for hiding this comment

UdjinM6 Jul 8, 2024

Choose a reason for hiding this comment

UdjinM6 Jul 8, 2024

Choose a reason for hiding this comment

UdjinM6 Jul 8, 2024

Choose a reason for hiding this comment

UdjinM6 Jul 8, 2024

Choose a reason for hiding this comment

UdjinM6 Jul 8, 2024

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

PastaPastaPasta commented Jul 23, 2024

knst commented Jul 5, 2024 •

edited

Loading

knst Jul 8, 2024 •

edited

Loading