[Feat] Add On-Demand Bytecode Verification API - #272
Conversation
Add configurable bytecode verification that allows nodes to verify source
code stored in PackageRegistry compiles to matching on-chain bytecode.
Changes:
- Add api.bytecode_verification_enabled config flag (default: disabled)
- Add GET /accounts/{address}/modules/{module_name}/verification_status endpoint
- Implement on-demand verification with in-memory caching (10k entry LRU)
- Add VerificationStatus enum for internal caching (VerifiedSuccess/Failure/Unverified)
- API returns boolean: true (verified), false (verification failure), 404 (no source), 503 (disabled)
- Verification is node-local, on-demand, and does not affect consensus
The endpoint performs verification when requested by clients (e.g., explorers),
caches results to avoid re-compilation, and returns verification status.
When the flag is disabled, the endpoint returns 503 Service Unavailable.
5915ec3 to
f5a8323
Compare
There was a problem hiding this comment.
@apenzk looking good; tested it with the explorer and it works for the simple examples.
Raised a few issues; see what you think.
Would also like a variety of contracts requiring different deps than only std, in https://github.com/movementlabsxyz/analysis-explorer-vulnerability
| /// Tests verification returns HTTP 503 when feature is disabled. | ||
| /// Why: Ensures feature flag correctly gates access. |
There was a problem hiding this comment.
| /// Tests verification returns HTTP 503 when feature is disabled. | |
| /// Why: Ensures feature flag correctly gates access. | |
| /// Tests verification returns HTTP 503 when feature is disabled. |
| /// Tests verification returns `false` when source doesn't match bytecode. | ||
| /// Why: Core security feature - detecting mismatched source code. |
There was a problem hiding this comment.
| /// Tests verification returns `false` when source doesn't match bytecode. | |
| /// Why: Core security feature - detecting mismatched source code. | |
| /// Tests verification returns `false` when source doesn't match bytecode. |
| /// Tests verification returns `true` when source matches bytecode. | ||
| /// Why: Core happy path validation. |
There was a problem hiding this comment.
| /// Tests verification returns `true` when source matches bytecode. | |
| /// Why: Core happy path validation. | |
| /// Tests verification returns `true` when source matches bytecode. |
There was a problem hiding this comment.
Think the four tests' "why" comments are redundant / over-stating the obvious.
| /// Tests verification returns HTTP 404 for non-existent module. | ||
| /// Why: Proper error handling for invalid queries. |
There was a problem hiding this comment.
| /// Tests verification returns HTTP 404 for non-existent module. | |
| /// Why: Proper error handling for invalid queries. | |
| /// Tests verification returns HTTP 404 for non-existent module. |
| } | ||
| } | ||
|
|
||
| pub fn get(&self, address: &AccountAddress, module_name: &str) -> Option<VerificationStatus> { |
There was a problem hiding this comment.
@apenzk think the cache needs to include upgrade_number from PackageMetadata, otherwise the cache will have outdated stale versions of contracts even after they are upgraded.
pub fn get(&self, address: &AccountAddress, module_name: &str, upgrade_number: u64) -> Option<VerificationStatus> {
self.cache.get(&(*address, module_name.to_string(), upgrade_number))
}
pub fn insert(&self, address: AccountAddress, module_name: String, upgrade_number: u64, status: VerificationStatus) {
self.cache.insert((address, module_name, upgrade_number), status);
}
}
If you go with this approach, update the cache type at line 77:
pub struct VerificationCache {
cache: Cache<(AccountAddress, String, u64), VerificationStatus>,
}
Then the callers in state.rs would need to pass upgrade_number from the PackageMetadata.
There was a problem hiding this comment.
you are correct
i fix this now in commit ea9903a
I also add a test case in https://github.com/movementlabsxyz/analysis-explorer-vulnerability that checks that the explorer displays the upgraded contract.
| if let Some(cached_status) = self | ||
| .context | ||
| .verification_cache() | ||
| .get(&address_inner, &module_name_str) |
There was a problem hiding this comment.
if you include upgrade_number, seems like you'll need to fetch PackageRegistry first and do something like
let upgrade_number = registry.packages.iter()
.find(|pkg| pkg.modules.iter().any(|m| m.name == module_name_str))
.map(|pkg| pkg.upgrade_number)
.unwrap_or(0);
before calling get and insert.
| drop(file); | ||
|
|
||
| // Get minimal framework dependency - just move-stdlib for basic types like string | ||
| // Keep it minimal to avoid stack overflow from processing large frameworks |
There was a problem hiding this comment.
Wouldn't this be insufficient for anything other than the simplest contracts? In deps don't you need something more like
// 0x1 - core framework
aptos_framework::path_in_crate("move-stdlib/sources").to_string_lossy().to_string(),
aptos_framework::path_in_crate("aptos-stdlib/sources").to_string_lossy().to_string(),
aptos_framework::path_in_crate("aptos-framework/sources").to_string_lossy().to_string(),
// 0x3 - token standard
aptos_framework::path_in_crate("aptos-token/sources").to_string_lossy().to_string(),
// 0x4 - token objects
aptos_framework::path_in_crate("aptos-token-objects/sources").to_string_lossy().to_string(),
and in named addresses also include format!("aptos_token_objects=0x4"), ?
Did you encounter stack overflow issues?
There was a problem hiding this comment.
Wouldn't this be insufficient for anything other than the simplest contracts?
you are 100% correct.
fixing this ( see commit 03fae01) turned out to be quite tricky. i had to
A) add support for the above frameworks
B) increase the COMPILATION_STACK_SIZE to overcome stack overflow issues
C) cache also cases where compilation did not work
D) add following limiation: unfortunatelly for now i limit to the above deps and additional deps throw an error for when it cannot be compiled
There was a problem hiding this comment.
@apenzk if a dependency of alice:: produces a warning then I think there will be too many such cases to consider this system complete enough to be useful. I can potentially try to give some time to working out a more complete solution after the super app release, and will be happy to review further iterations.
| match self { | ||
| VerificationStatus::VerifiedSuccess => Some(true), | ||
| VerificationStatus::VerifiedFailure => Some(false), | ||
| VerificationStatus::Unverified => None, |
There was a problem hiding this comment.
Perhaps there should be another status like VerificationError for when verifier couldn't recompile (missing deps, tooling issue)?
There was a problem hiding this comment.
great idea. i have added error 422, which handles this now
see commit 03fae01
- Update VerificationCache to use (address, module_name, upgrade_number) as cache key - Fetch PackageRegistry before cache lookup to extract upgrade_number - Update cache.get() and cache.insert() calls to include upgrade_number parameter - Add comprehensive test for upgrade_number handling in cache - Ensures each package version is verified independently
|
@andygolay i have added
I also added two cases in analysis-explorer-vulnerability to test correct behavior |
- Replace ledger_version query param with upgrade_number - Always use latest state; read current upgrade_number from PackageRegistry - If client's upgrade_number != current, return 404 and do not serve from cache - Cache key: (address, module_name, upgrade_number) PackageRegistry only holds latest package metadata, so we cannot verify at an arbitrary ledger version. upgrade_number identifies the contract version and lets us reject stale clients so we never return verification for the wrong package version.
There was a problem hiding this comment.
Let me know if this comment misinterprets the state of the system #272 (comment)
I generally can't approve unless the system is complete. I don't think it's fair to show a warning on contracts this system can't verify as it will likely cause end user confusion and FUD toward the modules with such a warning displayed in the explorer. I'll try to give some time to consider how to make the system complete.
2 participants
Add On-Demand Bytecode Verification API
Related Issues
Related to movement-network/explorer#114
Related Work
Explorer PR: movement-network/explorer#115
Description
This PR adds an on-demand bytecode verification feature to the Aptos Node API. The feature allows nodes (when enabled via a configuration flag) to verify that source code stored in
PackageRegistrycompiles to the same bytecode as what's deployed on-chain. Verification results are cached locally on the node to avoid re-verification on every request.New API Endpoint:
GET /v1/accounts/{address}/modules/{module_name}/verification_status{"verified": true}{"verified": false}{"error_code": "compilation_error", "message": "..."}{"error_code": "resource_not_found", ...}{"error_code": "api_disabled", ...}Configuration:
api.bytecode_verification_enabled(default:false)Caching: Verification results are cached using
mini_mokaLRU cache (10,000 entry limit) keyed by(address, module_name, upgrade_number). Theupgrade_numberfromPackageMetadataensures that package upgrades invalidate cached verification results.Type of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Unit Tests
Unit tests are included in
api/src/tests/verification_test.rscovering:verified: trueverified: falseaptos_std::compile correctlyaptos_token::(which requiresaptos-frameworktransitively) compile correctlyCompilationErrorstatus (HTTP 422) and results are cachedTest packages are in
api/src/tests/move/.E2E Testing
A test setup is available at analysis-explorer-vulnerability that demonstrates:
Key Areas to Review
Verification Logic (
api/src/verification.rs):move-stdlib,aptos-stdlib,aptos-framework,aptos-token,aptos-token-objectsCompiledModuleobjects and clearing metadata before comparisonCaching Strategy (
api/src/context.rs):mini_moka::sync::Cachewith LRU eviction (10,000 entry limit)(AccountAddress, module_name, upgrade_number)upgrade_numberfromPackageMetadataensures package upgrades invalidate cached resultsVerificationStatusenum internally, converts to boolean for API responseAPI Endpoint (
api/src/state.rs):bytecode_verification_enabledflag before processingConfiguration (
config/src/config/api_config.rs):bytecode_verification_enableddefaults tofalse(opt-in feature)Limitations
Supported Dependencies: This verification feature supports contracts that depend only on standard Aptos framework packages:
move-stdlibaptos-stdlibaptos-frameworkaptos-tokenaptos-token-objectsUnsupported: Contracts that depend on other user-deployed packages (e.g.,
use alice::library) cannot be verified, as we don't have access to those dependencies at compile time. In such cases, the API returns HTTP 422 witherror_code: "compilation_error"- this is distinct fromverified: false(which indicates a verified mismatch and is suspicious).Checklist
Configuration
To enable bytecode verification, add to your node configuration:
For Movement testnet, create an override file at
.movement/testnet_config_override.yaml:Then start the node with the
--config-overrideflag pointing to this file.