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

Should we apply the same logic to all reserved system contracts addresses? #1590

Closed
yangby-cryptape opened this issue Nov 23, 2023 · 2 comments · Fixed by #1597 or #1619
Closed

Should we apply the same logic to all reserved system contracts addresses? #1590

yangby-cryptape opened this issue Nov 23, 2023 · 2 comments · Fixed by #1597 or #1619
Assignees

Comments

@yangby-cryptape
Copy link
Collaborator

yangby-cryptape commented Nov 23, 2023

Description

  • As the following code defined:

    pub const fn system_contract_address(addr: u8) -> H160 {
    H160([
    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
    0xff, 0xff, 0xff, 0xff, addr,
    ])
    }

    All addresses between 0xffff..ff00 and 0xffff..ffff are system contracts.

  • But only 3 addresses in them are used.

    pub const METADATA_CONTRACT_ADDRESS: H160 = system_contract_address(0x1);

    pub const CKB_LIGHT_CLIENT_CONTRACT_ADDRESS: H160 = system_contract_address(0x2);

    pub const IMAGE_CELL_CONTRACT_ADDRESS: H160 = system_contract_address(0x3);

  • When executing a system contract, only above 3 contracts that have been clearly defined are dispatched to the system contracts process.

    let mut r = system_contract_dispatch(adapter, tx)
    .unwrap_or_else(|| Self::evm_exec(adapter, &config, &precompiles, tx));

    pub fn system_contract_dispatch<Adapter: ExecutorAdapter + ApplyBackend>(
    adapter: &mut Adapter,
    tx: &SignedTransaction,
    ) -> Option<TxResp> {
    if let Some(addr) = tx.get_to() {
    log::debug!("execute addr {:#x}", addr);
    if addr == NATIVE_TOKEN_CONTRACT_ADDRESS {
    return Some(NativeTokenContract::default().exec_(adapter, tx));
    } else if addr == METADATA_CONTRACT_ADDRESS {
    return Some(MetadataContract::default().exec_(adapter, tx));
    } else if addr == CKB_LIGHT_CLIENT_CONTRACT_ADDRESS {
    return Some(CkbLightClientContract::default().exec_(adapter, tx));
    } else if addr == IMAGE_CELL_CONTRACT_ADDRESS {
    return Some(ImageCellContract::default().exec_(adapter, tx));
    }
    }
    None
    }

    Other executions which are to the reserved addresses of system contracts are processed as normal executions.

  • As far as we know, a normal contract and a system contract are very different.


❓ So, my question is: once a reserved system contract is enabled in the future, how to treat the difference between now and then after that?

An example of current Axon:

  • Now, 0xffff..ff04 is an undeployed system contract address.
    • It's payable.
    • The data, even be empty, should be accessed with eth_getStorageAt and the proofs of its storage should be generated with eth_getProof.
  • Once it is deployed.
    • It's NOT payable.
    • The data should be accessed with axon_getSystemContractStorageAt and the proofs of its storage should be generated with axon_getSystemContractProof.1

Should we apply the same logic to those reserved system contracts now?

Let all system contracts, even reserved system contracts:

  • Should NOT be payable.
  • The data should be accessed with axon_getSystemContractStorageAt and the proofs of its storage should be generated with axon_getSystemContractProof.

Footnotes

  1. https://github.com/axonweb3/axon/issues/1561#issuecomment-1814445346

@Flouse Flouse added the agenda label Nov 23, 2023
@yangby-cryptape yangby-cryptape changed the title [Discuss] Should we apply the same logic to all reserved system contracts addresses? Should we apply the same logic to all reserved system contracts addresses? Nov 24, 2023
@yangby-cryptape yangby-cryptape added the d:confirmation Discussion required to confirm whether it's a bug label Nov 24, 2023
@Flouse
Copy link
Contributor

Flouse commented Nov 24, 2023

My Humble Opinion

  1. reserved system_contract_addresses is defined in

pub const fn system_contract_address(addr: u8) -> H160 {
H160([
0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
0xff, 0xff, 0xff, 0xff, addr,
])
}

  1. Just return error in eth_getStorageAt and eth_getProof when an address is system_contract_address

  2. fix system_contract_dispatch function
    It should handle all reserved system_contract_addresses.

Need confirmations before coding

@Flouse Flouse removed agenda d:confirmation Discussion required to confirm whether it's a bug labels Nov 27, 2023
@Flouse Flouse linked a pull request Dec 3, 2023 that will close this issue
6 tasks
@Flouse Flouse closed this as completed Dec 4, 2023
@Flouse
Copy link
Contributor

Flouse commented Dec 4, 2023

  • Just return error in eth_getStorageAt and eth_getProof when an address is system_contract_address

I think this change is not implemented.

Test

curl -X POST localhost:8000 \
  --data '{"jsonrpc":"2.0", "method": "eth_getStorageAt", "params": ["0xffffffffffffffffffffffffffffffffffffff04", "0x0", "latest"], "id": 1}' \
  --header 'Content-Type: application/json' 

# Result
{"jsonrpc":"2.0","id":1,"result":"0x00000000000000000000000000000000000000000000000000000000000004d2"}

Expected result

Not allow to call system contract address

ref: #1561 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants