Reorganise and refactor repository to prepare for integration testing. #10

evanlinjin · 2022-08-14T17:50:05Z

Description

This is basically a rewrite of the codebase. In summary, the codebase is now split into the following modules:

lnd: Contains the lnd-client codebase.
scheduler: Contains the business logic of scheduling and "payjoin-channel-funding" requests.
api: Contains all the endpoint logic (currently only for HTTP).

lnd:

The lnd client (now named lnd::LndClient) now implements Sync and Send allowing for better parallelism.
Lnd client "Verify funding" logic now interacts with remote lightning nodes in parallel (lnd::LndClient::verify_funding).
Added lnd::LndError to properly catch errors instead of panicing.

scheduler:

ScheduledPayJoin includes additional member functions for checkout output amounts and generating open_channel messages (from it's scheduled channels).
ScheduledPayJoin::test_connections now tests connections in parallel.
Introduce scheduler::ProposalPsbt (which needs a better name), that handles state for satisfying a scheduled payjoin-channel-funding.
Introduce SchedulerError for handling error cases instead of panicing.
Scheduler contains the business logic.

api:

Handles HTTP requests with a given Scheduler and options.
Introduce ApiError which can turn into a non-SUCCESS HTTP response instead of panicing.
We embed static files into the binary with include_bytes! macro instead of requiring specification of static file location.

For Reviewers

As mentioned by @DanGould, I should have kept this as multiple commits from the beginning to have 1 commit reflect 1 change (as this is easier on the reviewer). However, I have already merged all commits into one and splitting things out is not worth it (imo).

Instead, I have included various comments in the codebase prepended with REPLACES: that informs of where the associated logic is in the original code (before this PR).

Next Steps

I think it is important to prioritize changes that will allow us to write a simple integration test with a (relatively) simple bash script. Even if we transition over to container-based integration tests, having a bash script is something to start off from.

To get there faster, I propose integrating HTTPS (we won't need to depend on a reverse proxy this way) and add associated cli flags. The fewer dependencies our bash-integration-test has, the better. This should not take long.

Then we need to add the payjoin-client executable into this repository.

The actual integration test should use the btcd and lnd stack (as it seems the easiest to set up).

We need to know whether these changes break anything before merging.

DanGould · 2022-08-15T11:37:38Z

Test fails: ./target/debug/loptos --conf=LOPTOS.conf 10 03dde129510a7075c76cd759d1b29fea22d1ba1047a7a11b063008936931882fd0@192.168.160.7:9735 250000 10000
Error: Lnd(Generic(Status { code: Unknown, message: "already connected to peer: 03dde129510a7075c76cd759d1b29fea22d1ba1047a7a11b063008936931882fd0@192.168.160.7:48358", metadata: MetadataMap { headers: {"content-type": "application/grpc"} }, source: None }))

we get this error even without a connection to the peer. Maybe there's a transform where there shouldn't be.

DanGould · 2022-08-15T11:40:10Z

src/main.rs


 #[macro_use]
 extern crate serde_derive;
 extern crate configure_me;

 configure_me::include_config!();

-#[cfg(not(feature = "test_paths"))]


This was removed and not replaced. Please put that change in its own commit to document the change for review

DanGould · 2022-08-15T11:48:03Z

Another test error after the latest revision:

Running target/debug/payjoin-client 18443 none 'bitcoin:bcrt1qzw7pxt4qlvll0dg0lvxjazs6f339v9qvzsmff6?amount=0.00260420&pj=http://127.0.0.1:3000/pj '
thread 'main' panicked at 'called Result::unwrap() on an Err value: Extras(PjParseError(UnsecureEndpoint))', payjoin-client/src/main.rs:30:46
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

DanGould · 2022-08-15T11:56:38Z

Please separate this into commits each consist of 1 change and all build. That's easier for us to review and find errors

src/scheduler.rs

src/lnd.rs

In summary, the codebase is now split into the following modules: * `lnd`: Contains the lnd-client codebase. * `scheduler`: Contains the business logic of scheduling and satisfying "payjoin-channel-funding" requests. * `api`: Contains all the endpoint logic (currently only for HTTP). `lnd`: * The lnd client (now named `lnd::LndClient`) now implements `Sync` and `Send` allowing for better parallelism. * Lnd client "Verify funding" logic now interacts with remote lightning nodes in parallel (`lnd::LndClient::verify_funding`). * Added `lnd::LndError` to properly catch errors instead of panicing. `scheduler`: * `ScheduledPayJoin` includes additional member functions for checkout output amounts and generating `open_channel` messages (from it's scheduled channels). * `ScheduledPayJoin::test_connections` now tests connections in parallel. * Introduce `scheduler::ProposalPsbt` (which needs a better name), that handles state for satisfying a scheduled payjoin-channel-funding. * Introduce `SchedulerError` for handling error cases instead of panicing. * `Scheduler` contains the business logic. `api`: * Handles HTTP requests with a given `Scheduler` and options. * Introduce `ApiError` which can turn into a non-SUCCESS HTTP response instead of panicing. * We embed static files into the binary with `include_bytes!` macro instead of requiring specification of static file location.

These comments express the following: * In general, further explains what the code does. * Show where the original logic resides in the original codebase via `REPLACES`. * Show intended future changes via `TODO`. Additionally, I have tweaked a comment in the `lnd` crate as well.

DanGould · 2022-08-29T20:04:00Z

Manual integration test passing ✅

The code just needs thorough review

DanGould · 2022-10-04T15:51:57Z

Seems like ProposalPsbt is 1 to 1 with Scheduled PayJoin. Why not just add it to Scheduled PayJoin? If it's a separate struct, I'd say ScheduledPayJoin has a Psbt rather than the other way around, which is proposed in this pr

evanlinjin · 2022-10-06T16:07:47Z

Seems like ProposalPsbt is 1 to 1 with Scheduled PayJoin. Why not just add it to Scheduled PayJoin? If it's a separate struct, I'd say ScheduledPayJoin has a Psbt rather than the other way around, which is proposed in this pr

ScheduledPayJoin represents an (admin) request via /pj/request. It represents the channel opens that should happen when we get an original psbt (bip 78) from a sender, that contains a spend to our "fallback" address.

ProposalPsbt is badly named. It represents the "state" of operations after we receive the user's payjoin original psbt (bip 78). This includes:

Scheduler::pop_payjoin - check the original psbt to see whether an output contains the scriptPubKey which was provided as fallback, and if so, we know that this should trigger these channel opens (as defined in ScheduledPayJoin). Hence, Scheduler::pjs is a map of scriptPubKey -> ScheduledPayJoin.
If everything is okay, we attempt to "satisfy" the scheduled payjoin, recording state within ProposalPsbt (which, again, is badly named). So we do channel opens (as defined in out ScheduledPayJoin request) and whatnot to finalize the proposal psbt (bip 78) that we send back to the sender.

I hope that makes sense!

DanGould · 2022-10-08T23:46:22Z

The proposed changes have been adapted to #17

DanGould · 2022-10-06T20:39:26Z

src/api.rs

+        let sched = sched.clone();
+        async move {
+            let handler = move |req| handle_request(sched.clone(), opts, req);


@evanlinjin What's going on with the double clone here? Are we going to get inconsistencies with differing copies of the Scheduler rather than a singleton?

evanlinjin force-pushed the reorganize_for_integration branch 5 times, most recently from fdd1f87 to c20e380 Compare August 15, 2022 09:02

DanGould reviewed Aug 15, 2022

View reviewed changes

evanlinjin commented Aug 15, 2022

View reviewed changes

src/scheduler.rs Outdated Show resolved Hide resolved

src/scheduler.rs Outdated Show resolved Hide resolved

src/lnd.rs Show resolved Hide resolved

evanlinjin force-pushed the reorganize_for_integration branch 3 times, most recently from 9b1efd3 to 1cd9eaf Compare August 16, 2022 04:51

evanlinjin added 3 commits August 16, 2022 12:51

Fix ensure_node_connected error check and static file serving logic

b9afc20

evanlinjin force-pushed the reorganize_for_integration branch from 1cd9eaf to 257831b Compare August 16, 2022 04:53

DanGould mentioned this pull request Aug 16, 2022

Split lightning client logic from main #11

Merged

evanlinjin mentioned this pull request Aug 16, 2022

Add bash integration tests. #12

Closed

evanlinjin marked this pull request as ready for review October 4, 2022 05:04

DanGould mentioned this pull request Oct 4, 2022

Parallelize verify_funding: Tracking libifiication via #10 #16

Open

10 tasks

DanGould closed this Oct 8, 2022

DanGould mentioned this pull request Oct 14, 2022

Open Channel to Tor hidden service nodes #19

Closed

This was referenced Nov 8, 2022

Nolooking Roadmap Lightning PayJoin Beta Tracking #6

Open

Don't panic, handle errors. #57

Merged

DanGould mentioned this pull request Nov 16, 2022

Push & Display PayJoin Errors to the UI #60

Closed

4 tasks

DanGould mentioned this pull request Nov 21, 2022

Parallelize funding RPC if channel open > 1 #35

Open

DanGould reviewed Nov 30, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganise and refactor repository to prepare for integration testing. #10

Reorganise and refactor repository to prepare for integration testing. #10

evanlinjin commented Aug 14, 2022 •

edited

Loading

DanGould commented Aug 15, 2022 •

edited

Loading

DanGould Aug 15, 2022

DanGould commented Aug 15, 2022

DanGould commented Aug 15, 2022

DanGould commented Aug 29, 2022

DanGould commented Oct 4, 2022 •

edited

Loading

evanlinjin commented Oct 6, 2022 •

edited

Loading

DanGould commented Oct 8, 2022

DanGould Oct 6, 2022

Reorganise and refactor repository to prepare for integration testing. #10

Reorganise and refactor repository to prepare for integration testing. #10

Conversation

evanlinjin commented Aug 14, 2022 • edited Loading

Description

For Reviewers

Next Steps

DanGould commented Aug 15, 2022 • edited Loading

DanGould Aug 15, 2022

Choose a reason for hiding this comment

DanGould commented Aug 15, 2022

DanGould commented Aug 15, 2022

DanGould commented Aug 29, 2022

DanGould commented Oct 4, 2022 • edited Loading

evanlinjin commented Oct 6, 2022 • edited Loading

DanGould commented Oct 8, 2022

DanGould Oct 6, 2022

Choose a reason for hiding this comment

evanlinjin commented Aug 14, 2022 •

edited

Loading

DanGould commented Aug 15, 2022 •

edited

Loading

DanGould commented Oct 4, 2022 •

edited

Loading

evanlinjin commented Oct 6, 2022 •

edited

Loading