onomyprotocol / arc Goto Github PK

Change all possible althea reference to onomy

• Refactor relayer config to have clear blocks such as, orchestrator, relayer[batch, valset, logicacll] with the child configs, instead of log one variable config such as "batch_relaying_mode".
• Try to reduce the batch_request_mode since we have the batch_relaying_mode which should cover both

this needs to be done to prevent potential signature reuse

Splitted to multiple

Add and start checking for the batch min-fee pararm on the orchsestrator/relayer side.

• The param is a param of the gbt config toml file.
• This param should be optional - if not provided we accept all batches
• And example of the param is: 100anom,20stake. Means we should support a list of fees. This state is possible because we allow fee denom change, so to forse it we should implement it this way.
• If the total fee of a batch < min-fee the batch should be skipped.
• It is better to filter the batches on the cosmos rather then on the relayer side.

This issue can be done from the "native-reward" base branch.

It is ultimately used by a rust-decimal dependency of deep_space. I need to update the trio of our custom dependencies. I can't remember why rust-decimal is in there, we need to update or remove it. borsh 1.0 is close to releasing, may want to wait for that. Looking at https://github.com/althea-net/deep_space/pulls?q=is%3Apr+is%3Aclosed there are probably some things I need to merge.

Add a new integration test to the orchestrator - remote_stress_test, which we will use to load the real chain (testnet).
This test should be based on "BATCH_STRESS/transaction_stress_test" test. But with some adjustments.

• The test execution should take less than 1ETH, the less the better.
• The test should read some EVN variables and use the default values if a variable is not provided:
  • ADDRESS_PREFIX, STAKING_TOKEN, COSMOS_NODE_GRPC, COSMOS_NODE_ABCI, ETH_NODE - are already configured in test_runner/main.rs to read ENV or default. Be we need to adjust test behavior:
    • If COSMOS_NODE_GRPC and COSMOS_NODE_ABCI are provided the test shouldn't launch the gravity chain and use the provided COSMOS_NODE_* connections.
    • If ETH_NODE is provided the test shouldn't launch the geth and use the provided ETH_NODE connection.
  • GRAVITY_ADDRESS - the address of the gravity contract. If the address is provided use that address for the test.
  • NUM_USERS - the number of users who participate in the test (by default 100) (the constant already exists in the BATCH_STRESS test)
  • MINER_PRIVATE_KEY - the private key which is used for the contract deployment (the constant already exists in the tests)
    Also, that test should be integrated to all-up-test.sh -> REMOTE_STRESS and workflows/integration-tests.yml with the default variable values, because we will adjust it in the future and need to see that it works fine with the adjustments.

In case you will have to change the global defaults variables, it is ok.

This is a tracking issue for the state of the different bridge ports.

Moonbeam: merged as a branch https://github.com/onomyprotocol/cosmos-gravity-bridge/tree/moonbeam. Latest progress on https://github.com/AaronKutch/cosmos-gravity-bridge/tree/moonbeam

Fantom (go-opera): merged as a branch https://github.com/onomyprotocol/cosmos-gravity-bridge/tree/fantom
The only real issue currently is Fantom-foundation/go-opera#374. My branch is https://github.com/AaronKutch/cosmos-gravity-bridge/tree/fantom

Polygon (bor): merged as a branch https://github.com/onomyprotocol/cosmos-gravity-bridge/tree/polygon .

Near-Aurora: has its own repo https://github.com/onomyprotocol/near-aurora-bridge
This is a few months out of date and needs to be rebased.

Neon: Almost done, encountering too few topics in strange error https://github.com/AaronKutch/cosmos-gravity-bridge/tree/neon

AVAX (avalanchego): merged as a branch https://github.com/onomyprotocol/arc/tree/avax

BNB: merged as a branch https://github.com/onomyprotocol/arc/tree/bnb

Replace ETH geth in the integration test and emulate the happy-path integration test for the Avalanche chain.
The replacement might be done in the docker like in the current repo or docker-compose like in the near-aurora bridge..
The AVAX repositories: https://github.com/ava-labs
Use the new branch "ava" branch to keep changes.
The good point to start is to run the localnet in the docker with some initial balance on account

This is a reminder to myself that when we update arc for a consumer chain that we need this

For some reason, INVALID_EVENTS is hanging in the CI when it isn't locally. It isn't failing on the same nonce every time, which means there is some weird race condition.

this module was left out for some reason

Refactor all places with exit to return errors instead of exit.

Change root name of the module from bridge or althea to "onomyprotocol/cosmos-gravity-bridge"

implement Result<T, Error> type for functions that can fail but don't currently return anything at all

For some reason, the code on the main_loop.rs crashes with out of index exception.

       last_checked_event = nonces.event_nonce;
                        if !last_checked_event.to_u64_digits().is_empty() {
                            metrics_latest(
                                last_checked_event.to_u64_digits()[0],
                                "last_checked_event",
                            );
                        }

Our repo link
so we need to understand it and fix it because the original code doesn't have this and works properly Gravity repo link.

Set power reduction value eq 18 instead of 6 and fix tests (the problems will be with the AIRDROP test because the test constants are based on the current stake values). We need it to integrate the bridge tests with the onomy chain tests.

I noticed in the Actions CI that deprecated warnings are showing, and it seems the action-docker-layer-caching repo is unmaintained. We need to find a new caching method before July 2023

• Fix the ordering in dependencies in all Cargo files to follow the same pattern
• Validate whether we need all of them and remove redundant

Start returning errors in proposals CLI:

Here are the 5 TODOs. So instead of OK() the functions above must return Ok() or Error() which we just forward.

We need to add an option for the orchestrator to expose Prometheus metrics, in order to understand whether the orchestrator works or not. We need generic RUST metrics, and custom as well, for example, current nonce on the gravity and so on. It should be possible to set the host and port for the metrics.

It turns out bor has probabilistic finality, according to maticnetwork/bor#549 if we want to wait for eth checkpoints then we should have a delay of 512.

Remove all instances of logging secret keys in plaintext. Ensure that all secret data are saved
in encrypted form. See OWASP’s key storage best practices for a brief description.

We can use password-based encryption as a first option.
In order to simplify the support, the description should be implemented similar to the cosmos pass encryption.
https://docs.cosmos.network/master/run-node/keyring.html#the-pass-backend

It is also possible to stop saving the keys and showing the generated keys at all. In that case, the keys might be encrypted and stored by the OS, for example using pass. And then it will be the user/script's responsibility to extract it and use it as a variable for the gbt.

Update the "valset reward" docs with a details description of the new way of the reward reception.
This issue can be done from the "native-reward" base branch.

In PR #104 the most important finalization change is check_for_events in ethereum_event_watcher.rs, but I searched through the code and found get_last_checked_block and find_latest_valset that also relay information from the ethereum side. Before any of my changes, the latter two were coded to just use the latest changes, and I am suspicious of them. get_last_checked_block is concerning because it is used twice in a nested pattern in eth_oracle_main_loop, and I don't know if the possible implications of a invalidation from a reorg have been analyzed. find_latest_valset is also in the same situation, what would happen if a valset update was invalidated? For now I have changed them to used finalized blocks, and the test on ava-bridge doesn't show any problems I haven't already encountered from before the change. find_latest_valset delays results by no more than 20 minutes on the different chains, so I think I haven't introduced significant problems there.

I didn't realize that there were race conditions both ways, Neon produces a block for every event. I will simply remove the condition altogether, I added it initially because PoS was a new thing, but now I don't think we need the extra failsafe

Update the "batch reward" docs with a details description of the new way of the reward reception.
This issue can be done from the "native-reward" base branch.

Description: Exiting the main relayer loop in case of a valset parsing failure, could be used by a maliciousEthereum node to perform a Denial of Service attack on the orchestrator.

Location: https://github.com/onomyprotocol/cosmos-gravity-bridge/blob/99e66bc37069bc8fd37ff40157a7abc93f8d3dbb/orchestrator/relayer/src/main_loop.rs#L46

Description: Failure to check the configured loop speed could allow a relayer to transmit duplicate mes-sages.

Location: https://github.com/onomyprotocol/cosmos-gravity-bridge/blob/99e66bc37069bc8fd37ff40157a7abc93f8d3dbb/orchestrator/relayer/src/main_loop.rs#L102

Replace ETH geth in the integration test and emulate the happy-path integration test for the Fantom chain.
The replacement might be done in the docker like in the current repo or docker-compose like in the near-aurora bridge..
The Fantom repositories: https://github.com/Fantom-Foundation
Use the new branch "fantom" branch to keep changes.
The good point to start is to run the localnet in the docker with some initial balance on account

• Validate all errors types (had to create one more RecoverableError, potentially we don't need it, and just should refactor the use case where it is used)
• There are warn! or error! multiline usage after with we return an error. So we need to validate whether we can put the error!
• Discuss the errors list in the GravityBridge and start using them they work for us

Review the differences between the current "main" branch and Gravity-Bridge with tag v1.4.1.
The main focus should be on the parts where we have conflicts and I had to do fix them.
In case you find an issue it is possible to fix it fast then do it. If the fix is complex then create a new issue for it. Also if you find a point to improve then create a new issue with the description.

the gas limits can change quite a bit between chains and can change over time, I need to add an environment variable for it

In case of RPC error sendToCosmos client prints INFO instead of an error. And say "Your tokens should show up in the account" which is a lie.

[2022-02-22T11:05:27Z INFO  gbt::client::eth_to_cosmos] Sending 10000000000000000000 / 0xFab46E002BbF0b4509813474841E0716E6730136 to Cosmos from 0x2d9480eBA3A001033a0B8c3Df26039FD3433D55d to onomy104n3g00hms7kycg54jml24s84jjt9s2agq6r07
[2022-02-22T11:05:32Z INFO  gbt::client::eth_to_cosmos] Failed to send tokens! RpcError(JsonRpcError { code: -32000, message: "replacement transaction underpriced", data: "None" })
[2022-02-22T11:05:32Z INFO  gbt::client::eth_to_cosmos] Your tokens should show up in the account onomy104n3g00hms7kycg54jml24s84jjt9s2agq6r07 on Gravity Bridge within 5 minutes

Now the orchestrator/relayer uses the orchestrator cosmos address to receive the (valset) reward.
We need to add an additional reward-recipient param for the orchestrator/relayer and use it to receive the reward.
This "reward-recipient" param is optional, but if provided it should be used instead of the "orchestrator" address for as a reward_recipient.

This issue can be done from the "native-reward" base branch.

As soon as a new release candidate (probably rc6) with my PR for the official binary releases is released https://github.com/Fantom-foundation/go-opera/tags we need to update the dockerfile on fantom branch. I'm not sure how long it will be before a full release and not just a release candidate is made

Replace atom and graviton denom in code to anom since the onmy chain primary token is anom.
Also pay attention that anom uses 6digits, anom uses 18.

• Refactor loops like: "let start = Instant::now()" to follow common style with the tokio timeout

Refactor the ethereum_events.rs code. Now, most of the methods have code duplication and parse the events in a weird manner one attribute by one (incrementing the next byte index), which makes the support extremely hard, because the change of the first lines of the code significantly changes the following code.

The refactored code should either use ABI-generated code to parser the events or a number of functions to parse the events attribute by index, like here.
Also, it should validate and return the details errors if case any of the sub-decoding functions return an error.

All methods of the "ethereum_events.rs" must be covered by unit tests. The input data might be taken from the solidity tests.

This issue can be done from the "native-reward" base branch.

Ethereum ABI docs

this test fails sometimes due to badly constructed logic within the test itself,

the comment says it all:

// this is hacky and not really a good way to test validator set updates in a highly
    // repeatable fashion. What we really need to do is be aware of the total staking state
    // and manipulate the validator set very intentionally rather than kinda blindly like
    // we are here. For example the more your run this function the less this fixed amount
    // makes any difference, eventually it will fail because the change to the total staked
    // percentage is too small.

This is a problem I will fix, where https://github.com/onomyprotocol/cosmos-gravity-bridge/blob/8cbc8bcf2aead49ed02a68586fe312269bb3d8ec/orchestrator/orchestrator/src/ethereum_event_watcher.rs#L44 panics if the latest block number is less than the block delay. This shouldn't happen in practice, except I have experienced it because of bugs. We should return a proper error message in case this would happen in production (which could happen if something is misconfigured).

Based on the conversation and the audit, the onomy team decide to have the bloc delay equal 35.

Reference implementation.
https://github.com/onomyprotocol/cosmos-gravity-bridge/pull/23/files

Current (sub)module structure is using Rust 2015 outdated way, let's switch it to the newer one.
https://doc.rust-lang.org/nightly/edition-guide/rust-2018/path-changes.html#no-more-modrs

As per Fantom-foundation/go-opera#374 we can use USE_FINALIZATION = false with a zero block delay

Find all places with usage of check_for_fee and ```check_for_eth `` function and fix the usage to forward errors.

In order to understand the intended logic, you can match the v1.4.1 implementation of the gravity-bridge/gravity-bridge.

Every validator on Avalanche has https://github.com/ava-labs/coreth/blob/master/core/tx_pool.go#L60-L66 which prevents transaction sizes above 32 kb. This causes one of the tests to fail, we need to make sure normal operation is not close to the 32 kb limit and that the valset updates could not be softlocked or something because of this