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

Handle private channel hop hints #487

Open
wants to merge 28 commits into
base: develop
Choose a base branch
from

Conversation

contrun
Copy link
Collaborator

@contrun contrun commented Jan 17, 2025

This PR depends on Use the fee rate in oubound channel to calculate the TLC fee by contrun · Pull Request #489 · nervosnetwork/fiber.

The migration code is not well-tested. I only tried the following.

  1. Run git checkout v0.3.1 to checkout to the v0.3.1 version code.
  2. Run REMOVE_OLD_STATE=y ./tests/nodes/start.sh to start a few fnn nodes with all new states.
  3. Run (cd ./tests/bruno; npm exec -- @usebruno/[email protected] run e2e/router-pay -r --env test) to create a few payment sessions.
  4. Run ldb --db=tests/nodes/1/fiber/store --secondary_path=/tmp/test/ dump --hex | awk '/0xC0/ {print $1}' | sed 's/0xC0/0x/g' to see all the payment sessions created.
  5. Run ldb --db=tests/nodes/1/fiber/store --secondary_path=/tmp/test/ dump --hex | awk '/0xC0/ {print $1}' | sed 's/0xC0/0x/g' | xargs -I __ curl -sS --json '{"id": "42", "jsonrpc": "2.0", "method": "get_payment", "params": [{"payment_hash": "__"}]}' http://127.0.0.1:21714/ | jq -s to see all the payment session detailed information.
  6. Stop the nodes created in step 2.
  7. Run git checkout handle-private-channel-hop-hints to checkout the code of this PR.
  8. Run (cd migrate; cargo run -- --path ../tests/nodes/1/fiber/store) to start migration.
  9. Run cargo build && (clear; cd ./tests/nodes/; ../../target/debug/fnn -d 1) to start node 1.
  10. Run ldb --db=tests/nodes/1/fiber/store --secondary_path=/tmp/test/ dump --hex | awk '/0xC0/ {print $1}' | sed 's/0xC0/0x/g' | xargs -I __ curl -sS --json '{"id": "42", "jsonrpc": "2.0", "method": "get_payment", "params": [{"payment_hash": "__"}]}' again to check the payment session data have been migrated (by eyes).

@contrun contrun force-pushed the handle-private-channel-hop-hints branch from 8f14f6b to 3dd662a Compare January 22, 2025 06:58
@contrun contrun marked this pull request as ready for review January 23, 2025 05:52
Comment on lines +124 to +125
pub channel_outpoint: OutPoint,

Copy link
Collaborator

Choose a reason for hiding this comment

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

any special reason to change from tx hash to channel outpoint?

Copy link
Collaborator Author

@contrun contrun Jan 24, 2025

Choose a reason for hiding this comment

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

Only to be more consistent with other source code in the code base. Either tx hash or outpoint is fine for me. We just need to be consistent.


assert!(res.is_ok(), "Send payment failed: {:?}", res);
Copy link
Collaborator

Choose a reason for hiding this comment

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

from the code in find_path, seems we will first try to pick channels with hint, should we add unit test for channels with and without hint, and make sure channels with hint will be selected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As discussion on discord, the decision is that

那就还是简单点? hints 的意思就是 hints,真正用的 payment path 可能就是不采用这些 hints。用户想要限制使用某些路径就用 send_to_route

}

// #[tokio::test]
// async fn test_send_payment_with_route_to_self_with_hop_hints() {
// init_tracing();
Copy link
Collaborator

Choose a reason for hiding this comment

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

we expect to add these tests back after we have send_to_route?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.

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.

2 participants