feat: add signature verification for delegated accounts #5255

akaladarshi · 2025-02-08T06:08:37Z

Summary of changes

Changes introduced in this pull request:

Add signature verification for delegated accounts

Reference issue to close (if applicable)

Closes #5242

Change checklist

I have performed a self-review of my own code,
I have made corresponding changes to the documentation. All new code adheres to the team's documentation standards,
I have added tests that prove my fix is effective or that my feature works (if possible),
I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

LesnyRumcajs

Thanks. Some technical comments, otherwise LGTM.

LesnyRumcajs · 2025-02-10T10:45:44Z

src/shim/crypto.rs

+    if addr.protocol() != Delegated {
+        return Err(format!(
+            "cannot validate a delegated signature against a {} address expected",
+            addr.protocol(),
+        ));
+    }
+
+    if signature.len() != SECP_SIG_LEN {
+        return Err(format!(
+            "invalid delegated signature length. Was {}, must be {}",
+            signature.len(),
+            SECP_SIG_LEN
+        ));
+    }


nit: there's a nice macro anyhow::ensure that makes such validations more expressive.

LesnyRumcajs · 2025-02-10T10:47:03Z

src/shim/crypto.rs

+    signature: &[u8],
+    data: &[u8],
+    addr: &crate::shim::address::Address,
+) -> Result<(), String> {


Let's use anyhow::Result<()>. String itself should rarely be an error class.

But then we will have to refactor existing functions: as you can see here.

Also the fvm_shared: verify_bls_sig, verify_secp256k1_sig returns Result<(), String>.

Yes, it would need to be refactored, and it would be a good change. From briefly looking at the calls, it shouldn't be too complex.

The functions from fvm_shared should also be refactored (historical trivia: those were likely copied from Forest in 2021). However, given that it's a library's public interface, they should rather use a more constrained error type enumeration. Of course, refactoring ref-fvm methods is out of the scope of this issue, but it might be interesting to do so in the future.

If that's the case then I will update the existing verify function to use anyhow and we can raise a separate issue for updating ref_evm functions.

Excellent, thanks @akaladarshi!

LesnyRumcajs · 2025-02-10T10:48:04Z

src/shim/crypto.rs

+    // check address against recovered address
+    if rec_addr == *addr {
+        Ok(())
+    } else {
+        Err("Delegated signature verification failed".to_owned())
+    }


ensure! could be used here as well.

elmattic · 2025-02-11T13:29:49Z

@akaladarshi You can enable "format on save" in your favorite editor to prevent those pesky linter errors.

LesnyRumcajs · 2025-02-11T13:38:54Z

@akaladarshi You can enable "format on save" in your favorite editor to prevent those pesky linter errors.

Either that or run make lint before submitting to ensure no lints fail during CI. You might need to install some additional tooling.

forest/Makefile

Line 58 in ae5e424

lint: license clean lint-clippy

LesnyRumcajs · 2025-02-11T17:38:11Z

@akaladarshi, did you try running the node locally with your changes? If you look at the logs for calibnet RPC checks for Forest, you will see:

2025-02-11T16:20:30.441807Z WARN forest::chain_sync::tipset_syncer: Validating block [CID = bafy2bzacedyionjhebhdnqox3qd77vvdd3mvqrwhk27mqnvyl4tkr6qkzf6f2] in EPOCH = 2398815 failed: Validation error: Processing error: Error calculating weight: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaepph3yqaewolhvxsgisqjctlse3qtled37khcofkgsny33uw7uu, state_root_version=unknown, Processing error: Could not update state: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaepph3yqaewolhvxsgisqjctlse3qtled37khcofkgsny33uw7uu, state_root_version=unknown, Processing error: Failed to calculate state: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaepph3yqaewolhvxsgisqjctlse3qtled37khcofkgsny33uw7uu, state_root_version=unknown, Validation error: Consensus error: StateManager error: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaepph3yqaewolhvxsgisqjctlse3qtled37khcofkgsny33uw7uu, state_root_version=unknown, Consensus error: Power actor not found

akaladarshi · 2025-02-11T17:46:23Z

@akaladarshi, did you try running the node locally with your changes? If you look at the logs for calibnet RPC checks for Forest, you will see:

2025-02-11T16:20:30.441807Z WARN forest::chain_sync::tipset_syncer: Validating block [CID = bafy2bzacedyionjhebhdnqox3qd77vvdd3mvqrwhk27mqnvyl4tkr6qkzf6f2] in EPOCH = 2398815 failed: Validation error: Processing error: Error calculating weight: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaepph3yqaewolhvxsgisqjctlse3qtled37khcofkgsny33uw7uu, state_root_version=unknown, Processing error: Could not update state: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaepph3yqaewolhvxsgisqjctlse3qtled37khcofkgsny33uw7uu, state_root_version=unknown, Processing error: Failed to calculate state: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaepph3yqaewolhvxsgisqjctlse3qtled37khcofkgsny33uw7uu, state_root_version=unknown, Validation error: Consensus error: StateManager error: Can't create a valid state tree from the given root. This error may indicate unsupported version. state_root_cid=bafy2bzaceaepph3yqaewolhvxsgisqjctlse3qtled37khcofkgsny33uw7uu, state_root_version=unknown, Consensus error: Power actor not found

@LesnyRumcajs Yeah it was mentioned in the issue as well, that we are able to verify the signature by comparing with lotus signature but while syncing it's failing #5242 (comment)

akaladarshi · 2025-02-11T17:47:25Z

I am looking into this behaviour only why it is happening.

akaladarshi · 2025-02-17T05:21:28Z

@LesnyRumcajs Based on my investigation, I found out that there were few issues in how we were processing Ethereum tx -> Filecoin message vice versa i.e (method params was supposed to be encoded in CBOR). Also few functions were missing so I have updated the code accordingly. Please review the changes.

Meanwhile I will try to add few unit tests for the new changes.

LesnyRumcajs · 2025-02-17T09:30:51Z

@LesnyRumcajs Based on my investigation, I found out that there were few issues in how we were processing Ethereum tx -> Filecoin message vice versa i.e (method params was supposed to be encoded in CBOR). Also few functions were missing so I have updated the code accordingly. Please review the changes.

Meanwhile I will try to add few unit tests for the new changes.

@elmattic would you mind looking taking a look?

elmattic · 2025-02-17T09:36:50Z

@LesnyRumcajs Based on my investigation, I found out that there were few issues in how we were processing Ethereum tx -> Filecoin message vice versa i.e (method params was supposed to be encoded in CBOR). Also few functions were missing so I have updated the code accordingly. Please review the changes.
Meanwhile I will try to add few unit tests for the new changes.

@elmattic would you mind looking taking a look?

Of course.

elmattic · 2025-02-17T10:08:46Z

@akaladarshi Looks like test_eip_1559 is failing.

akaladarshi · 2025-02-17T10:29:16Z

@akaladarshi Looks like test_eip_1559 is failing.

I am looking into adding more unit tests and fixing/updating existing once, just wanted to get few comments on the changes.

elmattic

Great work, looks good. Just a few nits, and consider adding tests for functions like verify_delegated_sig and authenticate_msg.

src/shim/crypto.rs

elmattic · 2025-02-17T14:44:57Z

There are also a few small typos to fix, it's delegated, not delegate. For example:

fn delegate_sign() {

Search for occurrences of delegate_ in the codebase.

src/eth/eip_1559_transaction.rs

akaladarshi · 2025-02-17T16:32:08Z

@elmattic one more thing, I guess test is failing because perviously we were checking self.chain_id != chain_id here , and which was working because in tests, since we are providing different chain id's ( ETH_CHAIN_ID should have been set as calibnet::ETH_CHAIN_ID because that's what we are expecting here)

elmattic · 2025-02-19T09:18:22Z

@akaladarshi Great! Can you just fix the small lint error? Thanks! 🙏

LesnyRumcajs · 2025-02-19T11:37:02Z

src/key_management/wallet.rs

+        let delegated_key_info = KeyInfo::new(SignatureType::Delegated, delegated_priv_key);
+        let delegated_key = Key::try_from(delegated_key_info).unwrap();
+        let addr = delegated_key.address;
+        println!("addr: {}", addr);


Suggested change

println!("addr: {}", addr);

leftover?

LesnyRumcajs · 2025-02-19T11:40:24Z

src/shim/crypto.rs

+        msg: &SignedMessage,
+        addr: &crate::shim::address::Address,
+    ) -> anyhow::Result<()> {
+        let mut sig = self.clone();


Why do we need to clone the entire instance? Does it need to always be cloned?

self is a Signature itself, cloning it, so we can update the signature bytes in line 157, without actually affecting the self.

You can use this syntax as well, which is a bit more idiomatic too: https://doc.rust-lang.org/book/ch05-01-defining-structs.html#creating-instances-from-other-instances-with-struct-update-syntax

match self.sig_type { SignatureType::Delegated => { ... let sig = Signature { bytes: eth_tx.to_verifiable_signature(Vec::from(self.bytes()), eth_chain_id)?, ..*self }; let digest = eth_tx.rlp_unsigned_message(eth_chain_id)?; sig.verify(&digest, addr) } _ => { let digest = msg.message().cid().to_bytes(); self.verify(&digest, addr) } }

@elmattic are you talking about something like this ?.

Let me re-phrase. Do you think cloning (a relatively costly operation; you need to heap-allocate the entire vector) is necessary in case the signature type is not Delegated (which is the case most of the time)?

are you talking about something like this ?.

Yes, that's it, this will only clone in case of Delegated sig. and doing ..*self should be cheap here.

@LesnyRumcajs Thanks for pointing it out, I think you're right it doesn't make sense at all. I will update it as per my last comment.

LesnyRumcajs

LGTM, thanks!

feat: add delegated verify

a1e00e1

akaladarshi requested a review from a team as a code owner February 8, 2025 06:08

akaladarshi requested review from lemmih and LesnyRumcajs and removed request for a team February 8, 2025 06:08

linter fixes

5ba099b

LesnyRumcajs reviewed Feb 10, 2025

View reviewed changes

refactor: return anyhow result from signature verify

ab26fd9

akaladarshi added 2 commits February 11, 2025 19:03

linter error

68231bb

Merge branch 'main' into akaladarshi/delegated-verify

8eb2250

akaladarshi added 5 commits February 17, 2025 10:11

Merge branch 'main' into akaladarshi/delegated-verify

109fa60

fix: verify delegated signature while syncing

3c229c9

refactor: get signed message to use get unsigned message

93f211b

update changelog

a48dc78

linter fixes

bdab038

akaladarshi requested a review from LesnyRumcajs February 17, 2025 05:20

elmattic requested changes Feb 17, 2025

View reviewed changes

src/shim/crypto.rs Show resolved Hide resolved

src/shim/crypto.rs Outdated Show resolved Hide resolved

akaladarshi commented Feb 17, 2025

View reviewed changes

src/eth/eip_1559_transaction.rs Show resolved Hide resolved

akaladarshi added 3 commits February 19, 2025 01:00

test: add unit tests and address comments

26ec8c6

fix: CI failing tests

a017462

small refactor

27c3c51

akaladarshi requested a review from elmattic February 18, 2025 19:31

akaladarshi and others added 2 commits February 19, 2025 01:02

Merge branch 'main' into akaladarshi/delegated-verify

c2335a7

Merge branch 'main' into akaladarshi/delegated-verify

ad02c6f

elmattic approved these changes Feb 19, 2025

View reviewed changes

akaladarshi added 2 commits February 19, 2025 15:08

fix: ci linter issue

d76f0fb

Merge branch 'main' into akaladarshi/delegated-verify

1959586

akaladarshi requested a review from elmattic February 19, 2025 09:39

elmattic approved these changes Feb 19, 2025

View reviewed changes

LesnyRumcajs reviewed Feb 19, 2025

View reviewed changes

akaladarshi added 2 commits February 19, 2025 17:41

nit: remove leftover print statement

7d20425

address comment

d1baaa6

LesnyRumcajs approved these changes Feb 19, 2025

View reviewed changes

elmattic added this pull request to the merge queue Feb 19, 2025

Merged via the queue into ChainSafe:main with commit fb4f407 Feb 19, 2025
42 checks passed

akaladarshi deleted the akaladarshi/delegated-verify branch February 19, 2025 16:25

feat: add signature verification for delegated accounts #5255

feat: add signature verification for delegated accounts #5255

Conversation

akaladarshi commented Feb 8, 2025 • edited Loading

Summary of changes

Reference issue to close (if applicable)

Other information and links

Change checklist

LesnyRumcajs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akaladarshi Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akaladarshi Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmattic commented Feb 11, 2025

LesnyRumcajs commented Feb 11, 2025

LesnyRumcajs commented Feb 11, 2025

akaladarshi commented Feb 11, 2025

akaladarshi commented Feb 11, 2025

akaladarshi commented Feb 17, 2025

LesnyRumcajs commented Feb 17, 2025

elmattic commented Feb 17, 2025

elmattic commented Feb 17, 2025

akaladarshi commented Feb 17, 2025

elmattic left a comment • edited Loading

Choose a reason for hiding this comment

elmattic commented Feb 17, 2025

akaladarshi commented Feb 17, 2025 • edited Loading

elmattic commented Feb 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elmattic Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LesnyRumcajs left a comment

Choose a reason for hiding this comment

akaladarshi commented Feb 8, 2025 •

edited

Loading

akaladarshi Feb 10, 2025 •

edited

Loading

akaladarshi Feb 10, 2025 •

edited

Loading

elmattic left a comment •

edited

Loading

akaladarshi commented Feb 17, 2025 •

edited

Loading

elmattic Feb 19, 2025 •

edited

Loading