Non-Accountable mode for rust-teos

1010adigupta commented 1 year ago

Description

This PR closes #33

What?

The current rust implementation of The Eye of Satoshi (rust-teos) runs in accountable mode. That is, data coming from the tower is handed alongside signed receipts acknowledging the contract between the user and the tower. These receipts aim to hold the tower accountable in case of misbehavior. This project aims to generalize the current design so the tower can be run in accountable and non-accountable modes through a compilation flag.

Non-Accountable mode features:

The tower should not have to sign appointments or registration requests, the client doesn't have to store any receipts as you can't use them to account for the tower for its mistakes anyway.
The user will still need to send signed requests to the tower, given that's the way the tower identifies the user The tower won't hand-signed receipts to the user, though.
So the APIs need to be updated so the return of some endpoints (the ones returning signatures and storing the receipts) do not if the tower is compiled in non-accountable mode.
If the user is not OK with it, they can simply just abandon the tower after requesting the registration.

Task-completed:

[X] Modify registertower to run in the non-accountable mode
[X] Modify getregistrationreceipt to run in the non-accountable mode
[x] Modify getappointmentreceipt to run in the non-accountable mode
[x] Modify getappointment to run in the non-accountable mode
[x] Modify gettowerinfo to run in the non-accountable mode
[x] Modify retrytower to run in the non-accountable mode
[x] Modify abandontower to run in the non-accountable mode
[x] Modify tower to handle misbehaving-proofs in the non-accountable mode

Compilation Instructions:

Compile the project for non-Accountable mode: cargo build --features notAccountable
Now add the plugin to CLN by installing it, to do this run: cargo install --locked --path watchtower-plugin --features notAccountable
then run the tower using teosd now you can use client endpoints that are customised to run in this mode.

sr-gi commented 11 months ago

A couple more things:

This doesn't really compile "as is". I had to fix watchtower-plugin/src/net/http.rs. Also, regarding the PR description, explain what the PR is doing and what is the rationale, and then, add how to test it in the end if you feel it's necessary. The current description is not really helpful beyond how to compile this.

1010adigupta commented 11 months ago

will reverse changes so that, nonAccountable mode is the default one, and other necessary changes demanded, on it now.

sr-gi commented 10 months ago

I've pushed a commit to properly fix the formatting so the diff can be smaller, given that it was impossible to review otherwise. However, this still needs a lot of cleaning. Are you using any linter? There are a lot of unused variables, attributes, imports. Also imports that are repeated for accountable and non-accoutable.

You really need to go over all the modified files and check properly (both manually but also use a linter to make your life easier). Only the minimal things should go between a conditional compilation flag, so, for instance, if we are importing dependencies A, B, C, and D, and A, B are common, C is only used for accountable, and D only for non-accountable, only C and D need to be conditional, you cannot do

#[cfg(feature = "accountable")]
use A,B,C
#[cfg(not(feature = "accountable"))]
use A,B,D

1010adigupta commented 10 months ago

Hey @sr-gi I have been asking for these detailed reviews from you for a long time, and now since you have given it I will do the required changes starting now. Thank You

sr-gi commented 10 months ago

Hey @sr-gi I have been asking for these detailed reviews from you for a long time, and now since you have given it I will do the required changes starting now. Thank You

Sorry, but I couldn't give you a review like this with the code format being all over the place. I sent you several patches for the code formatting over the weeks but you never applied them, so I had to end up doing it myself, hence why the review couldn't come earlier.

1010adigupta commented 9 months ago

Hey I have tried to resolve most issues Now only two problems are remaining with the code:

You told me not to replicate functions and write code logic to optionally compile specific parts within the same function, I have written my code so as to minimize complete function replication wherever possible, the function where I have replicated the complete code is because I was not able to do it within the single function due to syntax error of using #cfg[()], it does not combine multiple lines of code the way you are telling, I tried many times, let's have a meet to discuss how we can resolve this issue, I will explain it clearly in the meet itself.
Some tests need to be rewritten for non-accountable mode.

1010adigupta commented 9 months ago

Hey I have seen the provided examples for fixing the code, implementing it now.

sr-gi commented 3 months ago

Closing due to inactivity. This was attempted but never fully implemented.

mariocynicys commented 3 months ago

Closing due to inactivity. This was attempted but never fully implemented.

Could it be useful for future trials? maybe draft it instead? @sr-gi

sr-gi commented 3 months ago

I'll reference it in the original issue in case someone wants to pick up where this was left

talaia-labs / rust-teos