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

feat: add delegated address in forest-wallet #5217

Merged

Conversation

akaladarshi
Copy link
Contributor

Summary of changes

Changes introduced in this pull request:

-Adds Delegated signature and address type in forest-wallet

Reference issue to close (if applicable)

Closes #4769

Other information and links

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.

@akaladarshi akaladarshi requested a review from a team as a code owner February 2, 2025 15:41
@akaladarshi akaladarshi requested review from lemmih and elmattic and removed request for a team February 2, 2025 15:41
@akaladarshi
Copy link
Contributor Author

akaladarshi commented Feb 2, 2025

@elmattic I have added the Delegated address type, but the verify function is still unimplemented, because there is no verify_delegated_sig function in fvm_shared crate.

Should we add it locally?

@elmattic
Copy link
Contributor

elmattic commented Feb 3, 2025

It would be great to update the code so that default status/ address balances are displayed in aligned columns:

$ forest-wallet list           
Address                                   Default Balance
t1wlfkoqqh2xqlxj2uew66fmj6s4iobzonal6r64q           1 FIL
t3ucoqm2wqocfpi5dv2cywyejkdnfpau7uj4ifevaxjn23gizo4gwly7zeklxeemjkaawbkcgxbuq4ls3bqnza           0 FIL

@elmattic
Copy link
Contributor

elmattic commented Feb 3, 2025

@elmattic I have added the Delegated address type, but the verify function is still unimplemented, because there is no verify_delegated_sig function in fvm_shared crate.

Should we add it locally?

Yes. The equivalent of lotus wallet sign/verify should work with f4-type addresses.

@akaladarshi akaladarshi requested a review from elmattic February 4, 2025 13:02
@elmattic
Copy link
Contributor

elmattic commented Feb 4, 2025

@akaladarshi is wallet verify subcommand working? I'm unable to verify c0ffeebabe message:

forest-wallet verify -a t410f2g4hvnfne4zluqvhrjpeonptlchqjzvxa6d5kla -m c0ffeebabe -s 03f7261df1d45a51c66023803f54ae700942edb37789150576e868f295af9cfe18002421ecf22b9fff8d8782bef38ce056e8a83761bccdf74163f710ce2206308100
false

@akaladarshi
Copy link
Contributor Author

akaladarshi commented Feb 4, 2025

@akaladarshi is wallet verify subcommand working? I'm unable to verify c0ffeebabe message:

forest-wallet verify -a t410f2g4hvnfne4zluqvhrjpeonptlchqjzvxa6d5kla -m c0ffeebabe -s 03f7261df1d45a51c66023803f54ae700942edb37789150576e868f295af9cfe18002421ecf22b9fff8d8782bef38ce056e8a83761bccdf74163f710ce2206308100
false

@elmattic I am checking that, wrote the test for it but, I guess it's not correctly working.
I also am seeing the error for Delegated address while syncing.

Let me figure out what's going on and fix it.

@akaladarshi
Copy link
Contributor Author

t410f2g4hvnfne4zluqvhrjpeonptlchqjzvxa6d5kla

@elmattic I tried this locally, with the signature generated through forest-wallet sign command
Is there something wrong here?

Screenshot 2025-02-04 at 10 30 36 PM

@elmattic
Copy link
Contributor

elmattic commented Feb 4, 2025

t410f2g4hvnfne4zluqvhrjpeonptlchqjzvxa6d5kla

@elmattic I tried this locally, with the signature generated through forest-wallet sign command Is there something wrong here?

Screenshot 2025-02-04 at 10 30 36 PM

Ah, I tried using a key and signature generated by Lotus. Isn't it supposed to work?

@akaladarshi
Copy link
Contributor Author

@elmattic Ideally it should work, also while going over this issue I noticed same thing with the secp256k1:

  • Signature output is different for the forest and lotus
  • Forest is not able to verify lotus generated signature
Screenshot 2025-02-05 at 9 04 26 AM Screenshot 2025-02-05 at 9 01 21 AM

@elmattic
Copy link
Contributor

elmattic commented Feb 5, 2025

@elmattic Ideally it should work, also while going over this issue I noticed same thing with the secp256k1:

  • Signature output is different for the forest and lotus
  • Forest is not able to verify lotus generated signature

Ok, then maybe it’s not intended to work that way. We can investigate later to determine if it’s actually an issue.

@akaladarshi
Copy link
Contributor Author

@elmattic don't just merge this yet, I found out why lotus and forest have different signatures, and there are few changes I am verifying as well. Will pushing that change as well

@akaladarshi
Copy link
Contributor Author

@elmattic So the issue is of two parts:

  • The forest-wallet sign command was not printing the signature with it's type, like lotus does

  • Ideally the signature will contains [type+data] the forest-wallet verify command was suppose to remove the first byte and consider rest of it as the signature data. instead it was passing the whole signature as it's data.

So after this change we will be able to verify the lotus wallet generated signature with forest-wallet.

@akaladarshi
Copy link
Contributor Author

@elmattic One last thing I wanted to point out, even thought the verify command is working correctly with lotus but I am facing issue in syncing with the calibnet:

2025-02-05T12:13:04.218525Z WARN forest::chain_sync::tipset_syncer: Validating block [CID = bafy2bzacedpmqrksuxntogj4rcifhryiap6kidjpw6wgrj5q6kv57gg2mdyf4] in EPOCH = 2375810 failed: Validation error: Message signature invalid: Delegated signature verification failed
2025-02-05T12:13:04.219148Z ERROR forest::chain_sync::tipset_syncer: Sync messages check state failed for tipset range
2025-02-05T12:13:04.219241Z ERROR forest::chain_sync::chain_muxer: Bootstrapping failed, re-evaluating the network head to retry the bootstrap. Error = TipsetRangeSyncer(Validation("Message signature invalid: Delegated signature verification failed"))

@elmattic
Copy link
Contributor

elmattic commented Feb 5, 2025

So after this change we will be able to verify the lotus wallet generated signature with forest-wallet.

Good catch!

@elmattic
Copy link
Contributor

elmattic commented Feb 5, 2025

@elmattic One last thing I wanted to point out, even thought the verify command is working correctly with lotus but I am facing issue in syncing with the calibnet:

2025-02-05T12:13:04.218525Z WARN forest::chain_sync::tipset_syncer: Validating block [CID = bafy2bzacedpmqrksuxntogj4rcifhryiap6kidjpw6wgrj5q6kv57gg2mdyf4] in EPOCH = 2375810 failed: Validation error: Message signature invalid: Delegated signature verification failed
2025-02-05T12:13:04.219148Z ERROR forest::chain_sync::tipset_syncer: Sync messages check state failed for tipset range
2025-02-05T12:13:04.219241Z ERROR forest::chain_sync::chain_muxer: Bootstrapping failed, re-evaluating the network head to retry the bootstrap. Error = TipsetRangeSyncer(Validation("Message signature invalid: Delegated signature verification failed"))

If Forest is not able to sync correctly after those changes, I propose that we leave the last fixes you did and just keep the code that introduces 'delegated' addresses and wallet cli improvements. We can do the signature fixes in a following PR.

Sounds good?

@akaladarshi
Copy link
Contributor Author

@elmattic One last thing I wanted to point out, even thought the verify command is working correctly with lotus but I am facing issue in syncing with the calibnet:

2025-02-05T12:13:04.218525Z WARN forest::chain_sync::tipset_syncer: Validating block [CID = bafy2bzacedpmqrksuxntogj4rcifhryiap6kidjpw6wgrj5q6kv57gg2mdyf4] in EPOCH = 2375810 failed: Validation error: Message signature invalid: Delegated signature verification failed
2025-02-05T12:13:04.219148Z ERROR forest::chain_sync::tipset_syncer: Sync messages check state failed for tipset range
2025-02-05T12:13:04.219241Z ERROR forest::chain_sync::chain_muxer: Bootstrapping failed, re-evaluating the network head to retry the bootstrap. Error = TipsetRangeSyncer(Validation("Message signature invalid: Delegated signature verification failed"))

If Forest is not able to sync correctly after those changes, I propose that we leave the last fixes you did and just keep the code that introduces 'delegated' addresses and cli improvements. We can do the signature fixes in a following PR.

Sounds good?

@elmattic Yeah make sense.

I will just revert the verify part of delegated address

@akaladarshi akaladarshi force-pushed the akaladarshi/add-delegated-address branch from 1aaee90 to 3221430 Compare February 5, 2025 14:29
@akaladarshi akaladarshi requested a review from elmattic February 5, 2025 14:54
@akaladarshi
Copy link
Contributor Author

@elmattic I am done with the changes, can you allow CI workflow to run so we can merge

@akaladarshi akaladarshi force-pushed the akaladarshi/add-delegated-address branch from 79536e1 to 9c7d833 Compare February 5, 2025 16:54
@elmattic
Copy link
Contributor

elmattic commented Feb 6, 2025

@akaladarshi There is a few spellcheck errors to fix.

@akaladarshi
Copy link
Contributor Author

@akaladarshi There is a few spellcheck errors to fix.

I had updated the signature_type field comment in forest-wallet new command to secp256k1 from SECP256k1 to reflect the new changes, otherwise it becomes misleading. SignatureType

So should I update the comment it or disable the linter

@elmattic
Copy link
Contributor

elmattic commented Feb 6, 2025

So should I update the comment it or disable the linter

Ah, I see. The best approach here is to ignore them by enclosing them in single quotes (e.g., secp256k1), and voilà.

@elmattic elmattic added this pull request to the merge queue Feb 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 7, 2025
@elmattic elmattic added this pull request to the merge queue Feb 7, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 7, 2025
@elmattic elmattic added this pull request to the merge queue Feb 7, 2025
Merged via the queue into ChainSafe:main with commit 6477b4f Feb 7, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add delegated addresses to forest-wallet new
3 participants