feat(cosmos): komodo-defi-proxy support

[onur-ozkan]

Mandatory Items:

• Implemented a new signature algorithm for komodo-defi-proxy that's coin-agnostic and works in pubkey-only mode.
• Refactored the nft and eth proxy to use the new algorithm.
• Added proxy support for tendermint (including websocket connections).

Optional Items (will be handled in the next sprint to avoid blocking the Moon-Fi release):

• Implement node querying rpc.

Breaking Changes:

Tendermint activation payloads have been updated as follows:

Previously tendermint activations required rpc_urls field as a list of plain string values. This has now been changed to nodes, which expects a list of the following json structure:

{
    "url": "$node_url" (required),
    "komodo_proxy": $is_behind_komodo_defi_proxy (optional, default is false)
}

All previous fields in RPC methods that controlled komodo-defi-proxy (e.g., in eth activations) have been updated to komodo_proxy.

Docs Issue:

KomodoPlatform/komodo-docs-mdx#311

[onur-ozkan]

I'll PR the needed changes to the coins file, and update

enable_tendermint_with_assets & enable_eth_with_tokens.

Thanks!

What duration range of delay in server restart should be tested?

1-3 seconds.

[shamardy]

Annoyingly this part is causing those docker tests to fail, I will dig into this problem.

I see, before this PR a different peer id was used on restart but now when restarting a previous connection was still established on the relay side with same peer id (due to restarting very quickly)

https://github.com/KomodoPlatform/rust-libp2p/blob/6fc061b58853c1b0dafaa19a4a29343c0ac6eab3/protocols/gossipsub/src/behaviour.rs#L3164-L3178

        if other_established == 0 {
            // Ignore connections from blacklisted peers.
            if self.blacklisted_peers.contains(&peer_id) {
                debug!("Ignoring connection from blacklisted peer: {}", peer_id);
            } else {
                debug!("New peer connected: {}", peer_id);

                if self.config.i_am_relay {
                    debug!("Sending IAmRelay to peer {:?}", peer_id);
                    let event = Rpc {
                        messages: Vec::new(),
                        subscriptions: Vec::new(),
                        control_msgs: vec![ControlAction::IAmRelay(true)],
                    };
                    self.notify_primary(peer_id, event);

which causes the below log to never occur
https://github.com/KomodoPlatform/rust-libp2p/blob/6fc061b58853c1b0dafaa19a4a29343c0ac6eab3/protocols/gossipsub/src/behaviour.rs#L3564

        debug!("Completed IAmrelay handling for peer: {:?}", peer_id);

I thought we shouldn't rely on this log in tests and use get_relay_mesh RPC instead. So I tried this below (without the commit that uses random peer id in docker tests) and still the test doesn't pass

    /// Repeatedly calls the `get_relay_mesh` RPC method until it returns a non-empty result or a timeout occurs.
    /// This function is used to ensure that the relay mesh is populated before proceeding.
    #[cfg(not(target_arch = "wasm32"))]
    pub async fn check_seednodes(&mut self) -> Result<(), String> {
        let timeout_sec = 22.0;
        let start_time = now_float();
        let delay_ms = 500;

        loop {
            let response = self.rpc(&json!({"userpass": self.userpass, "method": "get_relay_mesh"})).await?;
            let relay_mesh: Json = json::from_str(&response.1).map_err(|e| ERRL!("{}", e))?;

            if !relay_mesh["result"].as_array().unwrap_or(&vec![]).is_empty() {
                return Ok(());
            }

            if now_float() - start_time > timeout_sec {
                return Err(ERRL!("Timeout while waiting for relay mesh to be populated"));
            }

            Timer::sleep_ms(delay_ms).await;
        }
    }

It passes after I add sleep here for 1 second

    block_on(mm_alice.wait_for_log(120., |log| log.contains(WATCHER_MESSAGE_SENT_LOG))).unwrap();
    alice_conf.conf["dbdir"] = mm_alice.folder.join("DB").to_str().unwrap().into();
    block_on(mm_alice.stop()).unwrap();
    thread::sleep(Duration::from_secs(1));

    let mut mm_alice = block_on(MarketMakerIt::start_with_envs(
        alice_conf.conf,
        alice_conf.rpc_password.clone(),
        None,
        &[],
    ))
    .unwrap();

So this gives the relay time to close the old connection and avoid problems, and it sends IAmrelay message to the light node to add it to the relay mesh, @onur-ozkan using log will also work but using get_relay_mesh as above is better in case libp2p changes the log in the future. Using a persistent peer id is actually better than the previous approach and might help us in whitelisting/blacklisting in the future if we need to do it at the p2p layer. I always like PRs that enhance other things that were not intended :)

What duration range of delay in server restart should be tested?
1-3 seconds.

@smk762 @onur-ozkan I guess the above does this test but you can test again @smk762

[onur-ozkan]

Using a persistent peer id is actually better than the previous approach and might help us in whitelisting/blacklisting in the future if we need to do it at the p2p layer.

That was one of the main purposes. We already have these lists on the proxy project and we also do rate limiting based on the peer address. Using dynamic peer addresses would be problematic on the proxy side, and it would also complicate debugging the network.

To bypass the current proxy logic, you would need to run MM2 with different passphrases and generate signed messages for each request, which is quite expensive. We will be handling this case by checking the "is this peer part of the network?" RPC (which I will be implementing in the coming sprint). The goal is to put a heavy load (by requiring to run mm2 on the main network) on the suspect's machine so they cannot abuse our services without significantly exhausting their resources.

[shamardy]

Using a persistent peer id is actually better than the previous approach and might help us in whitelisting/blacklisting in the future if we need to do it at the p2p layer.

That was one of the main purposes.

I understand the purpose of this PR for the komodo proxy, what I meant by pubkey whitelisting/blacklisting is the Q3 roadmap item related to compliance :)

[shamardy]

LGTM! I will add a commit that handles this comment #2173 (comment) as agreed with @onur-ozkan. This commit will need to checked and approved by someone else other than me.

[shamardy]

For these test without this commit 3d0c05e

docker_tests::docker_tests_inner::test_maker_and_taker_order_created_with_same_priv_should_not_match
docker_tests::docker_tests_inner::test_orders_should_match_on_both_nodes_with_same_priv
docker_tests::swap_proto_v2_tests::test_v2_swap_utxo_utxo_file_lock

The relay/seednode allows another connection from the same peer_id but doesn't notify the peer that it was added for the second connection due to the check for other_established mentioned here #2173 (comment) and I don't think that it receives messages from it (not sure if it relays messages to it or not). This introduces some undefined behavior in our p2p network that wasn't present when each light node used a random peer_id.

E.g. In test_orders_should_match_on_both_nodes_with_same_priv, only the first node's order gets processed unlike before. But when I tested with 3 relays by making mm_bob, mm_alice_1 and mm_alice_2 connect to different relays initially, each node's order is processed separately like before.

In real environment the below scenario can happen:

• For the second connection from the same peer, the relay is not added to the connected_relays list in the second node.
• However, is_connected_to_addr for this relay will still return true on the second node. (Debugged this myself to make sure)
• Over time, maintain_connection_to_relays will cause the second node to connect to a different set of relays.
• Once connected to different relays, placing an order from the second node will work as before.

This change introduces timing-dependent behavior in our p2p network (for the rare case of using the same seed on 2 different devices at the same time):

• Order processing now depends on which node connects to a relay first.
• The second node's behavior becomes unpredictable and time-dependent.

These issues were not present when each light node used a random peer_id. Also, do we actually want to allow the same peer to connect multiple times to the p2p network but to a different set of relays if it has a different IP address?

It's not a big deal as it's a very rare condition, but I am not sure why test_orders_should_match_on_both_nodes_with_same_priv was added in the past unless we wanted each node to handle the orders placed from it. If a user starts komodo wallet from mobile and desktop simultaneously we can get these cases.

@onur-ozkan I think these tests should be ignored for now as the behaviour is undefined. If you agree, I can ignore these tests and push the fixes for the other tests that only needed a delay on restart, we can merge the PR after that and think about this case separately. What do you think?

[onur-ozkan]

Thanks for the detailed debug report! I think we should revert this "persistent peer ID" logic for now, until we add the account locking functionality at the network level or until we add a "multi-ip - single-peer" support for out network (both can be done together with the next p2p breaking change). Otherwise users might run into this confusing issue and not know what's wrong. Even if they report it to the Komodo (e.g., via discord channels), it will be hard for us to figure out the real problem.

[shamardy]

I think we should revert this "persistent peer ID" logic for now, until we add the account locking functionality at the network level or until we add a "multi-ip - single-peer" support for out network (both can be done together with the next p2p breaking change).

I agree, you can revert the persistent peer ID, users will be able to reset their proxy usage stats by restarting the app but it's not that big of a problem and will be solved once we do persistent peer ID again. I think we will opt for a "multi-ip - single-peer" support but account locking at the network level is not bad too. The decision will be related to how we want to handle multi-device support, p2p account locking will simplify it a lot actually specially when/if we do cloud/IPFS data backup which means a single shared storage between all devices.

[shamardy]

🔥

[shamardy]

but we still need @KomodoPlatform/qa team to test this PR (by running and restarting nodes while running swaps) and approve it.

@smk762 I believe this test is not required anymore as we reverted back to random p2p key / peer_id for light nodes. Please confirm this @onur-ozkan

[onur-ozkan]

but we still need @KomodoPlatform/qa team to test this PR (by running and restarting nodes while running swaps) and approve it.

@smk762 I believe this test is not required anymore as we reverted back to random p2p key / peer_id for light nodes. Please confirm this @onur-ozkan

That's correct.

[cipig]

if i try to enable with

curl --url "http://127.0.0.1:7783" --data "{\"method\":\"enable_tendermint_with_assets\",\"mmrpc\":\"2.0\",\"params\": {\"ticker\":\"IRIS\",\"tokens_params\": [{\"ticker\":\"ATOM-IBC_IRIS\"}],\"nodes\":[{\"url\":\"https://rpc-irisnet-ia.cosmosia.notional.ventures\"}],\"tx_history\":false},\"userpass\":\"${userpass}\"}"

i get this error

{"mmrpc":"2.0","error":"Error parsing request: missing field `komodo_proxy`","error_path":"dispatcher","error_trace":"dispatcher:118]","error_type":"InvalidRequest","error_data":"missing field `komodo_proxy`","id":null}

shouldn't komodo_proxy be optional?

if i add \"komodo_proxy\":false it works:

{"mmrpc":"2.0","result":{"ticker":"IRIS","address":"iaa1zf9ss33z8mmczv9cu4ztnt7rkzvcsguxedvs6v","current_block":26226317,"balance":{"spendable":"1651.869209","unspendable":"0"},"tokens_balances":{"ATOM-IBC_IRIS":{"spendable":"0","unspendable":"0"}}},"id":null}

[onur-ozkan]

if i try to enable with

curl --url "http://127.0.0.1:7783" --data "{\"method\":\"enable_tendermint_with_assets\",\"mmrpc\":\"2.0\",\"params\": {\"ticker\":\"IRIS\",\"tokens_params\": [{\"ticker\":\"ATOM-IBC_IRIS\"}],\"nodes\":[{\"url\":\"https://rpc-irisnet-ia.cosmosia.notional.ventures\"}],\"tx_history\":false},\"userpass\":\"${userpass}\"}"

i get this error

{"mmrpc":"2.0","error":"Error parsing request: missing field `komodo_proxy`","error_path":"dispatcher","error_trace":"dispatcher:118]","error_type":"InvalidRequest","error_data":"missing field `komodo_proxy`","id":null}

shouldn't komodo_proxy be optional?

if i add \"komodo_proxy\":false it works:

{"mmrpc":"2.0","result":{"ticker":"IRIS","address":"iaa1zf9ss33z8mmczv9cu4ztnt7rkzvcsguxedvs6v","current_block":26226317,"balance":{"spendable":"1651.869209","unspendable":"0"},"tokens_balances":{"ATOM-IBC_IRIS":{"spendable":"0","unspendable":"0"}}},"id":null}

It should be optional, will fix it. Thanks.