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

AIP-X Code Verification API #150

Closed
wants to merge 9 commits into from

Conversation

0xhsy
Copy link

@0xhsy 0xhsy commented May 22, 2023

No description provided.

@bowenyang007
Copy link

Interesting idea! A couple of questions:

  • How does the user get the move.toml file?
  • What exactly are you validating? move.toml is very minimal. Maybe we should consider returning package metadata and do the verification on client side.

Copy link
Contributor

@banool banool left a comment

Choose a reason for hiding this comment

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

Hey, thanks for this proposal! I love the idea of a standard for this, I know multiple providers are looking into providing an API like this and I know certain tools (e.g. the explorer) will benefit from it.

There are a few things I don't quite understand, if you could help clear them up that'd be great.

  1. Is this API spec for a service that runs independent of a fullnode?
  2. How does this work if the code is not available via the fullnode API, e.g. because the developer chose not to upload it?
  3. How come you need to provide the Move.toml file, you can retrieve this when retrieving the module code.
  4. How come we need to include chainId in the response?
  5. How come we need to include isImmutable in the response? If we include this, why not include a more complete response containing all the information from Move.toml / the ABI.
  6. How is the requester meant to know the compilerVersion?

@0xhsy
Copy link
Author

0xhsy commented May 23, 2023

  • How does the user get the move.toml file?
  • What exactly are you validating? move.toml is very minimal. Maybe we should consider returning package metadata and do the verification on client side.

Hi @bowenyang007

  • I have confirmed that the manifest can be obtained through the getAccountResource of the Aptos SDK.

  • The key point here is accurately compiling the metadata stored on-chain and returning its match with the stored bytecode. While other data can be easily obtained by anyone accessing the on-chain, matching the stored code requires a mandatory compilation process.

Thanks for your feedback :)

@0xhsy
Copy link
Author

0xhsy commented May 23, 2023

Hey, thanks for this proposal! I love the idea of a standard for this, I know multiple providers are looking into providing an API like this and I know certain tools (e.g. the explorer) will benefit from it.

There are a few things I don't quite understand, if you could help clear them up that'd be great.

  1. Is this API spec for a service that runs independent of a fullnode?
  2. How does this work if the code is not available via the fullnode API, e.g. because the developer chose not to upload it?
  3. How come you need to provide the Move.toml file, you can retrieve this when retrieving the module code.
  4. How come we need to include chainId in the response?
  5. How come we need to include isImmutable in the response? If we include this, why not include a more complete response containing all the information from Move.toml / the ABI.
  6. How is the requester meant to know the compilerVersion?

Hi dport, good to see you again :)

  1. This is, in essence, the process of performing the role of a compiler on behalf of the requester.
  2. I perceive this as an API for service providers who want to disclose their code and seek its verification. If the code is not uploaded, it can be seen that the service provider or developer does not want to disclose the code. Like Etherscan, it is possible to verify the code after the upload and compilation process, but as Aptos allows the on-chain code to be switched to public, developers can verify after changing the on-chain code to public.
  3. I didn't realize this. Having seen your feedback and looked it up, I've confirmed that the manifest can be obtained through the getAccountResource of the Aptos SDK. The Move.toml is not necessary.
  4. This is simply for convenience, to make it easier to store the state in the DB in the explorer with only the response coming from the API. I agree that it's better to remove unnecessary parts. (This has been changed to 'network' to match the name of the Request parameter. )
  5. The 'isImmutable' status is quite important. It would be even better to have cross-verification with the client from the API response. This was designed with a call from the explorer in mind, and other information is already provided in the explorer.
  6. It might be difficult to check for already deployed contracts, but developers should realize they need to check it themselves for verification. Although it's a bit inconvenient, I don't think it would be a significant problem for the purpose of verification.

@banool, Thank you so much for your detailed feedback.

@banool
Copy link
Contributor

banool commented May 23, 2023

  1. The reason I ask is because tools like the explorer can already read whether a module is immutable from the ABI.
  2. I mean in your proposed API you must provide the compiler version. How is a tool like the explorer meant to know the compiler version to use in the request?

@0xhsy
Copy link
Author

0xhsy commented May 23, 2023

  1. The reason I ask is because tools like the explorer can already read whether a module is immutable from the ABI.
  2. I mean in your proposed API you must provide the compiler version. How is a tool like the explorer meant to know the compiler version to use in the request?
  1. Upon reflection, I agree that there doesn't seem to be a need to include this in the response. I will update accordingly.
  2. Much like in the EVM ecosystem where anyone can request verification, provided they have compiler information and source code, Aptos developers must also remember their Aptos client version. When they wish to verify their contract through the explorer, they must provide this information. While this might be a bit inconvenient, it is a necessary step in the verification process. Developers should be aware of the version they are using when deploying contracts. Cultivating this habit will aid in maintaining the accuracy and trustworthiness of contract verification on the Aptos network.
etherscan

@banool
Copy link
Contributor

banool commented May 23, 2023

  1. I think for tools like the explorer this isn't an acceptable requirement. For example I might want to know if the code for a module published by a third party matches the bytecode on chain. I didn't publish the module so I have no way to know what compiler version was used. I'll do some digging, perhaps we can store the compiler version in the module metadata (or perhaps it is already available somehow!)

@0xhsy
Copy link
Author

0xhsy commented May 23, 2023

I think for tools like the explorer this isn't an acceptable requirement. I'll do some digging, perhaps we can store the compiler version in the module metadata (or perhaps it is already available somehow!)

Oh, thank you! That's excellent. If we can store it in the module metadata, that would be the cleanest solution. May I ask why you mentioned that this is an unacceptable requirement for tools like the explorer?

@banool
Copy link
Contributor

banool commented May 23, 2023

Mostly because of that use case I mentioned. If user X publishes a module and user Y wants to verify it, user Y will never be able to because they don't know what compiler version user X used.

@0xhsy
Copy link
Author

0xhsy commented May 23, 2023

Mostly because of that use case I mentioned. If user X publishes a module and user Y wants to verify it, user Y will never be able to because they don't know what compiler version user X used.

You're correct. However, fundamentally, verification serves as a means for the publisher X to gain trust from all users including Y. Don't you think X has sufficient motivation to provide their code and compiler information to Y in some way?

Even so, incorporating it into the module's metadata would be the most effective approach.

@banool
Copy link
Contributor

banool commented May 23, 2023

Let me chat with the team about this one.

@banool
Copy link
Contributor

banool commented May 31, 2023

Hey sorry for the delay here, we're still discussing how we can expose this information.

@0xhsy
Copy link
Author

0xhsy commented Jun 1, 2023

Hey sorry for the delay here, we're still discussing how we can expose this information.

That's fine. I'm also preparing in various ways. I will share more soon.

@banool
Copy link
Contributor

banool commented Jun 12, 2023

Hey we're still chatting about this internally. If we don't have a resolution by the end of the week we can move forward with the compiler version field as part of the API.

I will have another round of feedback soon.

Copy link
Contributor

@wrwg wrwg left a comment

Choose a reason for hiding this comment

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

Of course, code verification can be performed locally through the SDK. However, to increase accessibility and usability, it's most desirable to provide it through an API in the explorer.

  1. Actually, I tend to disagree with this. A local verification via the SDK (as supported already) seems to me the most reliable one. It is easier and decentralized to verify that a locally installed SDK is not corrupted then to trust a centralized API. How can I ensure that the API provider is not colluding with a malicious smart contract developer to hide bad code from me? The SDK solution seems to me actually superior. I trust my SDK to generate my code -- and to verify others code as I need.

  2. If we really want this, perhaps we should do this differently. For packages with source onchain (majority), we should verify integrity of source and bytecode at deployment time. For packages without source code, we can generate source from the bytecode (possible in Move without information loss).

@0xhsy
Copy link
Author

0xhsy commented Jun 13, 2023

If we really want this, perhaps we should do this differently. For packages with source onchain (majority), we should verify integrity of source and bytecode at deployment time. For packages without source code, we can generate source from the bytecode (possible in Move without information loss).

Thank you for your feedback. I have a few questions. According to your suggestion, does this rely on the developer's voluntary action at the time of package deployment? Are you suggesting that there is a way to enforce verification in open-source blockchain SDKs or CLIs?

As you said, when I was in the Ethereum ecosystem, it was unsettling to rely entirely on Explorer. However, most users and developers did not verify the code themselves and pursued convenience. This is despite the fact that Etherscan is not open-source and is not officially operated by the foundation.

So, I think that if the foundation's official explorer just sets the verification API specification, various verification server operators can participate in the verification, reducing dependence on a specific server and dealing with various compiler versions.

@banool
Copy link
Contributor

banool commented Jun 13, 2023

If the approach you suggest is possible @wrwg, where source and bytecode are verified to match as part of consensus before being committed, that would be the ideal approach. It would presumably make package publishing much more expensive though, since all module publishing would require every validator to either:

  1. Compile the source into bytecode themselves and confirm they all get the same bytecode.
  2. Decompile the bytecode back into source and confirm they all get the same source.

I still think it's worth it though, this would be a big win for user safety.

@0xhsy
Copy link
Author

0xhsy commented Jun 13, 2023

Performing compilation by every validator for code verification might seem intuitively right, but there are several problems that follow this approach. @wrwg @banool

  1. Performance issue: When each validator performs a compilation, it requires significant computational resources. This can result in an overall degradation of the network's performance.

  2. Version compatibility issue: All validators need to be compatible with past compiler versions. Otherwise, they cannot correspond to various compiler versions from developers and verifications from other validators.

  3. Security issue: When validators perform the compilation process, there's a possibility that malicious validators may manipulate the bytecode. This necessitates additional security mechanisms.

  4. Cost issue: When validators perform a compilation, it generates additional costs. These costs can be reflected in the network usage fees.

  5. Complexity issue: In the future, smart contract codes and their dependencies will become increasingly complex. Inevitably, this places a burden on the compiler, which can lower the overall efficiency of the system, ultimately adversely affecting network performance.

Evaluating these issues and finding the right balance is an important task that lies before us.

@wrwg
Copy link
Contributor

wrwg commented Jun 17, 2023

Sorry for the delay on responding.

The arguments of cost and complexity of validators performing the compilation check, and then via consensus agree on this, make sense. It is too expensive like this. I was hoping for some kind of cryptographic approach (ZK proof perhaps) but did not had a chance yet to interview the experts about this.

While waiting to hear back about this, lets entertain the API idea.

  • First of all, we are in a rather different situation then Eth/EtherScan because Move bytecode can be transparently decompiled. This makes the need for a feature like EtherScan less important for a Move blockchain. If someone want to really see what code is executed, with 100% certainty, they can download the bytecode and decompile it. While we do not have a decompiler today, we have something into an intermediate format which is already pretty high-level, and a full decompiler is on the roadmap.

  • Having said so, we may still want to have the functionality of code verification. I'm just not sure why we want to have an API standard for that. Rather I would say this should be integrated into the fullnode HTTP APIs. There seems to be no need to have different providers for this. In fact, different providers integrating the Move compiler would become a drag on the agility how Aptos Labs can evolve the compiler. Every new version of the compiler would need to be coordinated with those providers. In turn, as part of an Aptos fullnode, the release model can be streamlined and synchronized.

Could we perhaps evolve the AIP to make this a new functionality of a fullnode? There would be basically a single call which takes the same parameters as package deployment with sources but minus bytecode, and verifies the code on chain by compiling the sources.

@0xhsy
Copy link
Author

0xhsy commented Jun 20, 2023

@wrwg, I deeply appreciate your insightful observations and suggestions on my proposed AIP.

One of the first things that captivated me about the Aptos ecosystem was indeed the innovative design of the Move language. Its transparency and ability to decompile bytecode into a high-level format is indeed impressive and a unique feature.

This API proposal was initiated with the goal of facilitating code verification based on these aspects of Move, aiming to enhance the accessibility of these innovative features to both users and developers. I believe it has the potential to provide meaningful value to many people.

Your suggestion about code verification by all nodes seems very effective, but I agree that such an approach could require substantial resources and management efforts.

Through this AIP, my objective is to minimize such complexities while enabling as many people as possible to utilize the code verification service. Aptos is still a blockchain in progress, moving towards its roadmap. This AIP could become a part of that journey.

When I presented this idea at a hackathon, my team received a lot of positive feedback and encouragement. This API, even in its early stages, has already attracted a lot of attention, and I am indeed planning to implement it in reality. If there is an AIP standard, not a proprietary format, various third parties could participate.

Looking at your suggestions, I am pondering how to harmoniously integrate these two approaches. I deeply appreciate your feedback and welcome any further thoughts you might have.

Thank you again, @wrwg. I consider it a great honor to be able to contribute to the ecosystem while discussing with someone like you. I am confident that through our continued dialogue, we can collectively strive for better solutions.

@wrwg
Copy link
Contributor

wrwg commented Jun 22, 2023

@gregnazario FYI

Copy link
Contributor

@wrwg wrwg left a comment

Choose a reason for hiding this comment

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

Thanks 0xshy for explaining the background of this project. I love the excitement and engagement. I left some constructive feedback on the current API.

If we want to move forward with this as an AIP, we need to make clear that the AIP does not cover how a service provider is actually certified. I added some suggestion how this could be clarified.

aips/code-verification-api.md Outdated Show resolved Hide resolved
aips/code-verification-api.md Show resolved Hide resolved
aips/code-verification-api.md Outdated Show resolved Hide resolved
aips/code-verification-api.md Outdated Show resolved Hide resolved
aips/code-verification-api.md Show resolved Hide resolved
Copy link
Contributor

@banool banool left a comment

Choose a reason for hiding this comment

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

Just some final nits, but I'd say this AIP is in a sufficiently detailed state to land as an AIP and start soliciting broader community feedback. Thanks a lot for working through our feedback!

aips/code-verification-api.md Outdated Show resolved Hide resolved
aips/code-verification-api.md Outdated Show resolved Hide resolved
aips/code-verification-api.md Outdated Show resolved Hide resolved
aips/code-verification-api.md Outdated Show resolved Hide resolved
0xhsy and others added 2 commits June 28, 2023 10:12
Co-authored-by: Daniel Porteous (dport) <daniel@dport.me>
@0xhsy 0xhsy requested a review from wrwg July 7, 2023 07:50
wrwg
wrwg previously approved these changes Jul 14, 2023
@davidiw
Copy link
Contributor

davidiw commented Jul 21, 2023

Hey @0xhsy, we discussed this internally and feel at this point in time this does not meet the criteria for a published AIP. Specifically AIPs are about proposed standards that have already had some form of implementation backing them and will become center points of the Aptos ecosystem. To date, all approved AIPs have a nature of impacting the Rest APIs, the VM, Framework, or underlying blockchain.

This AIP introduces an optional service that does not fit as part of a yet need to standardize part of the Aptos ecosystem. While it is a great idea, it would be better started off as a general community proposal, obtain some amount of adoption from both consumers and implementers proving the value and utility. This would likely result in some changes to the current approach. At which point, we would benefit from a standard so that we don't see dozens of different approaches.

So a couple things, 1) The team is going to explore an appropriate medium for communication about exploratory ideas. We already have the community forum, but perhaps that is insufficient for discovery? 2) I'm going to request changes on this but leave it open until we actually have adequately resolved 1 and updated the documentation around AIPs to be more explicit.

@davidiw davidiw dismissed wrwg’s stale review July 21, 2023 15:21

After internal discussion on AIPs, this was deemed not yet ready.

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

After internal discussion on AIPs, this was deemed not yet ready.

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.

None yet

6 participants