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

Add verification feature #631

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kairoski03
Copy link

Hello,
I added code verification feature.
The feature enables user to check if the source code matches to the on-chain bytecode.
User can choose verification service by changing url input.
The verification server get source code by account resource query and builds the source code
and compares the built bytecode to on-chain bytecode.
Please refer to below screenshots.

01
02
03

Copy link
Contributor

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, it looks alright. I have a few requests though:

  1. Would like to see how it's being verified / a link to the source code for it. Error messages are a little cloudy / it's failing to compile on the framework, which is strange.
  2. Minor: Verification service should be a little smarter. No reason to compile if it's already been compiled and not changed (this might require more work). This is purely an optimization though, and could still always compile on each button click.
  3. Please fix the lints so the build succeeds, as shown in the CI below

src/pages/Account/Components/codeDescription.ts Outdated Show resolved Hide resolved
src/pages/Account/Components/VerificationButton.tsx Outdated Show resolved Hide resolved
src/pages/Account/Components/VerificationButton.tsx Outdated Show resolved Hide resolved
src/pages/Account/Components/VerificationButton.tsx Outdated Show resolved Hide resolved
setIsInProgress(false);
if (dto.errMsg) {
setVerificationStatus("NOT_VERIFIED");
setVerificationServerErr(`${dto.errMsg}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern is the messages that currently come out of this need to be cleaned up a bit. Would be great to see some of the code for the verification service.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed verification server to show the actual error message.
(ex. Move compilation failed: Unable to resolve packages for package 'AptosFramework': While resolving dependency 'AptosStdlib' in package 'AptosFramework': While processing dependency 'AptosStdlib': Unable to find package manifest for 'AptosStdlib' at "~/mainnet/0x1/1704956814227/../aptos-stdlib" )

src/pages/Account/Components/CodeSnippet.tsx Outdated Show resolved Hide resolved
Comment on lines +22 to +24
export const defaultVerificationServiceUrl =
"https://verify.welldonestudio.io/aptos";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's a service for this, we should possibly provide help details somewhere for how someone can run it themselves. Since, this is essentially trusting this URL entirely, it would be good to provide a more trustless system (you can run it yourself), or that the foundation will run it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will inform you once I have prepared for it.

@kairoski03
Copy link
Author

kairoski03 commented Jan 11, 2024

Hello, @gregnazario.

Thank you for your feedback.

I answered your questions below.

  1. Would like to see how it's being verified / a link to the source code for it. Error messages are a little cloudy / it's failing to compile on the framework, which is strange.
    → I tried to add your github account to the verification server source code repository, but i had a some trouble. I will let you know after adding you.

  2. Minor: Verification service should be a little smarter. No reason to compile if it's already been compiled and not changed (this might require more work). This is purely an optimization though, and could still always compile on each button click.
    → I agree with you. I will optimize it after solving other things.

  3. Please fix the lints so the build succeeds, as shown in the CI below
    → Fixed.

I updated the version of Aptos CLI to the latest 2.4.0 which the verification server uses to compile.


Current verification service has two limitations.

  1. In Move.toml from on-chain, dependencies should be defined by git.
    Because verification server can't get dependencies defined by local, compile errors occur.
  • - AptosStdlib = { local = "../aptos-stdlib" }
    → It’s not available. ( dependency defined by local path in upper directory )

  • - AptosStdlib = { local = "./aptos-stdlib" }
    → It’s not available. ( dependency defined by local path in current directory )

  • - AptosStdlib = { git = "https://github.com/aptos-labs/aptos-core.git", subdir = "aptos-move/framework/aptos-stdlib/", rev = "main" }
    → It’s available.

  1. When a move file has a reference to spec, verification is not available.
    For example, coin.move in aptos-framework has a part which refers to coin.spec.move.
    Because the coin.spec.move is not uploaded when publishing, the verification server can’t download coin.spec.move and compile errors occur.
    compile_error

I am considering below solutions for this limitation.

  • Add a file upload UI so that the package publisher or someone can upload full source code.
  • If a user publish using remix wellondone studio plugin ( web ide ), the source files are uploaded on the cloud. I will use the source code when verifying the source code.
    스크린샷 2024-01-11 오후 6 20 24

It would be appreciate if you have a comment about this limitations.

kairoski03 added a commit to welldonestudio/explorer-aptos that referenced this pull request Jan 11, 2024
kairoski03 added a commit to welldonestudio/explorer-aptos that referenced this pull request Jan 11, 2024
kairoski03 added a commit to welldonestudio/explorer-aptos that referenced this pull request Jan 11, 2024
kairoski03 added a commit to welldonestudio/explorer-aptos that referenced this pull request Jan 11, 2024
@gregnazario
Copy link
Contributor

Hello, @gregnazario.

Thank you for your feedback.

I answered your questions below.

  1. Would like to see how it's being verified / a link to the source code for it. Error messages are a little cloudy / it's failing to compile on the framework, which is strange.
    → I tried to add your github account to the verification server source code repository, but i had a some trouble. I will let you know after adding you.
  2. Minor: Verification service should be a little smarter. No reason to compile if it's already been compiled and not changed (this might require more work). This is purely an optimization though, and could still always compile on each button click.
    → I agree with you. I will optimize it after solving other things.
  3. Please fix the lints so the build succeeds, as shown in the CI below
    → Fixed.

I updated the version of Aptos CLI to the latest 2.4.0 which the verification server uses to compile.

Current verification service has two limitations.

  1. In Move.toml from on-chain, dependencies should be defined by git.
    Because verification server can't get dependencies defined by local, compile errors occur.
  • - AptosStdlib = { local = "../aptos-stdlib" }
    → It’s not available. ( dependency defined by local path in upper directory )
  • - AptosStdlib = { local = "./aptos-stdlib" }
    → It’s not available. ( dependency defined by local path in current directory )
  • - AptosStdlib = { git = "https://github.com/aptos-labs/aptos-core.git", subdir = "aptos-move/framework/aptos-stdlib/", rev = "main" }
    → It’s available.
  1. When a move file has a reference to spec, verification is not available.
    For example, coin.move in aptos-framework has a part which refers to coin.spec.move.
    Because the coin.spec.move is not uploaded when publishing, the verification server can’t download coin.spec.move and compile errors occur.
    compile_error

I am considering below solutions for this limitation.

  • Add a file upload UI so that the package publisher or someone can upload full source code.
  • If a user publish using remix wellondone studio plugin ( web ide ), the source files are uploaded on the cloud. I will use the source code when verifying the source code.
    스크린샷 2024-01-11 오후 6 20 24

It would be appreciate if you have a comment about this limitations.

Yeah, I'm aware of those limitations, which can be a pain.

For now, maybe when we know it's address 0x1, that we can use the dependencies direct from git just so it doesn't say "it's unverified", especially since. The source code is uploaded at the same time

Copy link
Contributor

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The service doesn't seem to handle named addresses correctly. If there's a named address, it just fails.

@kairoski03
Copy link
Author

kairoski03 commented Jan 15, 2024

The service doesn't seem to handle named addresses correctly. If there's a named address, it just fails.

I heard that you inquired with sooyoung as follows.
How do we handle named addresses in verification?

and I received a below link for test.
https://explorer.aptoslabs.com/account/0x3b36cac0ec1054b6a99facdef2a0015a2858ff75d10251590e606365394ac5bd/modules/code/tic_tac_toe?network=mainnet

I could not compile the source code from on-chain even though I filled named address with CLI 2.4.0.
I had to change CLI version to 1.0.7 and AptosFramework rev from main to eafa8d2.

CLI Version Github Rev Compile Result Bytecode Comparison
1.0.7 eafa8d2 Success Matches
1.0.7 main (d20fab97) Fail -
2.4.0 eafa8d2 Fail -
2.4.0 main (d20fab97) Success Doesn’t Match

I seems that additional version information needs to be uploaded to onchain when publishing packages to verify stable.

For now, the service does not handle named addresses.

To test,

  1. added additional named address. ( @address_a, @address_b, @address_c )
  2. added initialize function
  3. changed winner func name to a_winner

and compiled and analyzed bytecode.

a11ceb0b // magic word
06000000 // file format version
0c // table count
01000c ...omit...

// ------------- module name -------------
0b 7469635f7461635f746f65 // tic_tac_toe
06 7369676e6572 0a 73696d706c655f6d6170 ... omit ...
// ---------------------------------------

// ------------------- func -------------------
08 615f77696e6e6572                 // a_winner
0e 63757272656e745f706c61796572     // current_player
0b 64656c6574655f67616d65           // delete_game
0c 64656c6574655f73746f7265         // delete_store
09 6765745f626f617264               // get_board
0a 696e697469616c697a65             // initialize
0a 706c61795f7370616365             // play_space
07 706c6179657273                   // public fun players
0a 72657365745f67616d65             // reset_game
0a 73746172745f67616d65             // start_game
// --------------------------------------------

// ------------- member of struct, external struct ... -------------
05 626f617264 // board
08 785f706c61796572 // x_player
... omit ...
// -----------------------------------------------------------------

// ---------------------------- const ----------------------------
ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff // package address
0000000000000000000000000000000000000000000000000000000000000001 // std, aptos_framework, aptos_std address
0201 03               // const DRAW: u8 = 3;
0308 0900000000000000 // const EGAME_ALREADY_EXISTS: u64 = 9;
0308 0800000000000000 // const EGAME_NOT_FOUND: u64 = 8;
0308 0100000000000000 // const EGAME_NOT_OVER: u64 = 1;
0308 0600000000000000 // const EGAME_OVER: u64 = 6;
0308 0e00000000000000 // const EINVALID_ADDRESS: u64 = 14;
0308 0300000000000000 // const EINVALID_PLAYER: u64 = 3;
0308 0a00000000000000 // const EINVALID_RESETTER: u64 = 10;
0308 0500000000000000 // const ENOT_O_PLAYER_TURN: u64 = 5;
0308 0400000000000000 // const ENOT_X_PLAYER_TURN: u64 = 4;
0308 0c00000000000000 // const EOUT_OF_BOUNDS: u64 = 12;
0308 0200000000000000 // const EPLAYER_NOT_WINNER: u64 = 2;
0308 0b00000000000000 // const EINVALID_PLAYER: u64 = 3;
0308 0700000000000000 // const ESPACE_ALREADY_PLAYED: u64 = 7;
0308 0d00000000000000 // const ESTORE_NOT_FOUND: u64 = 13;
0201 00               // const NONE: u8 = 0;
0201 02               // const O: u8 = 2;
0201 01               // const X: u8 = 1;
0520 bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb // named address @address_b in func a_winner
0520 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa // named address @address_a in func current_player
0520 cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc // named address @address_c in func initialize
// -----------------------------------------------------------------

12617074 ...omit...

I have some questions.

  1. Could you provide some document about the bytecode structure?
    I don't understand exactly the structure of bytecode.
  2. Does the order in which addresses are displayed within the bytecode match the alphabetical order of function names?
  3. Is it possible to know the start offset and end offset of func part?
  4. Is it possible to know the start offset and end offset of named address part?

I am considering trying the following method.

  1. Extract the actual address of named address from bytecode.
  2. Extract pairs of func and named addresses from onchain module source code.
  3. Sort the func and named addresses pair by alphabetical func name and map the named address to actual address.
  4. Replace underbar in named address of actual address in Move.toml.

I would appreciate your opinion on whether above method is a feasible approach.

@kairoski03
Copy link
Author

I added @gregnazario to the repository of the verification server.

Below is the part of verification logic.

https://github.com/dsrvlabs/wds-compiler/blob/26c0310dc9f537a72aac4eb455f81e453b0bc713/src/domain/verification/aptos/aptos-verification.service.ts#L93

@kairoski03
Copy link
Author

@gregnazario Hello, Greg.

I granted a github repo read permission of verification server as you requested.
( https://github.com/dsrvlabs/wds-compiler.git )

But it seems that the progress is pending for a long time.
So I'm about to revoke your GitHub permission for now.

When you can initiate the review again, please let me know, and I'll grant the permission again.

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.

3 participants