-
Notifications
You must be signed in to change notification settings - Fork 63
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
Non-Accountable mode for rust-teos #222
Non-Accountable mode for rust-teos #222
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The whole PR needs proper formatting
- You don't really need to redefine the entire functions if just a tinny bit is changed. For instance, in
PublicTowerServices::register
the only bit that changes between the two methods is the signature being set or not, so you can handle that in a single method like so:
match self.watcher.register(user_id) {
Ok(receipt) => {
#[cfg(not(feature = "notAccountable"))]
let subscription_signature = receipt.signature();
#[cfg(feature = "notAccountable")]
let subscription_signature = None;
Ok(Response::new(common_msgs::RegisterResponse {
user_id: req_data.user_id,
available_slots: receipt.available_slots(),
subscription_start: receipt.subscription_start(),
subscription_expiry: receipt.subscription_expiry(),
subscription_signature,
}))
}
Err(_) => Err(Status::new(
Code::ResourceExhausted,
"Subscription maximum slots count reached",
)),
}
This really applies to every single method you have defined. If you want it to only be accessible under a certain compilation flag, guard the definition with `#[cfg((feature = ...))], otherwise, define the changes inside the method depending on the flag.
Finally, I think it may make more sense to define the accountable
flag instead of the nonAccountable
one, and make it default.
teos-common/src/receipts.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to rework way more than this in here. Both sign
and verify
only make sense for accountable mode, meaning that you can tag them to only be available if so.
The same applies to the signature
/user_signature
getters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on it
teos/Cargo.toml
Outdated
|
||
[features] | ||
notAccountable = [] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not add unnecessary empty lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still has an extra empty line
A couple more things: This doesn't really compile "as is". I had to fix |
will reverse changes so that, nonAccountable mode is the default one, and other necessary changes demanded, on it now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this a quick check. Removing some of these fields/methods will trigger plenty of modifications.
Apart from the comments, you should start making the AppointmentReceipt
conditional to the tower being compiled as accountable. This will trigger even more changes (way more than before), so do not sleep on it or you will have a rough time meeting the deadlines.
teos/Cargo.toml
Outdated
|
||
[features] | ||
notAccountable = [] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still has an extra empty line
watchtower-plugin/src/net/http.rs
Outdated
@@ -56,6 +56,7 @@ impl From<RequestError> for AddAppointmentError { | |||
} | |||
|
|||
/// Handles the logic of interacting with the `register` endpoint of the tower. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't add random line breaks
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
|
357cc16
to
f51027d
Compare
#[cfg(feature = "accountable")] | ||
subscription_signature: receipt.signature().unwrap(), | ||
#[cfg(not(feature = "accountable"))] | ||
subscription_signature: "None".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is not right.
If a field is not relevant in one of the modes (as is the case of the subscription signature) it does not have to be made optional, it has to be removed from the struct. There will be different versions of the struct depending on whether the accountability flag is set or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no such for this method already existed, so should I make a new struct in which no string filed is there ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can create a struct where the field is conditional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still mandatory. The proto file should be updated so the field is optional. "None" is a string, not an option.
@@ -125,6 +129,16 @@ impl PublicTowerServices for Arc<InternalAPI> { | |||
subscription_expiry, | |||
})) | |||
} | |||
#[cfg(not(feature = "accountable"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. You cannot just set the signature to None
. There is no point of having a compilation flag if we are going to treat this as an execution flag.
On top of that, this does not need to be two whole struct creations, just add the field conditionally to the constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can explain this in more detail, the constructor part especially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can have a struct that has some conditional fields, such as:
pub struct S {
pub data: D,
#[cfg(feature = "flag")]
pub is_set: bool,
}
The last field in this struct will only exist if the flag is set. This is the opposite of having an execution flag, in which the fields always exist but may or may nit be set depending on the parameter passed when executing. The latter is more brittle and error prone.
With respect to the constructors, you can either have two, and the more complex one calls the simpler and then sets the remaining fields, or have a setter than consumes an object. e.g.:
pub fn new(data: D) -> Self {
Self {
data: D
}
}
#[cfg(feature = "accountable")]
pub fn new(data: D, is_set: bool) -> Self {
let mut s = S::new(data);
s.is_set = is_set;
l
}
pub fn new(data: D) -> Self {
Self {
data: D
}
}
#[cfg(feature = "accountable")]
pub fn with_flag(self, is_set: bool) -> Self {
Self {
is_set,
..self
}
}
This latter will be called as:
let D = D:new(data).with_flag(is_set);
@@ -173,6 +175,7 @@ impl Watcher { | |||
/// charge of managing users. | |||
pub(crate) fn register(&self, user_id: UserId) -> Result<RegistrationReceipt, MaxSlotsReached> { | |||
let mut receipt = self.gatekeeper.add_update_user(user_id)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be made mut only if accountable, otherwise it is not mut
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change the order or this so the #[cfg(feature = "accountable")]
tag is defined only once:
#[cfg(not(feature = "accountable"))]
let receipt = self.gatekeeper.add_update_user(user_id)?;
#[cfg(feature = "accountable")]
{
...
}
#[cfg(not(feature = "accountable"))] | ||
pub(crate) fn add_appointment( | ||
&self, | ||
appointment: Appointment, | ||
user_signature: String, | ||
) -> Result<(u32, u32), AddAppointmentFailure> { | ||
let user_id = self | ||
.gatekeeper | ||
.authenticate_user(&appointment.to_vec(), &user_signature) | ||
.map_err(|_| AddAppointmentFailure::AuthenticationFailure)?; | ||
let (has_subscription_expired, expiry) = | ||
self.gatekeeper.has_subscription_expired(user_id).unwrap(); | ||
|
||
if has_subscription_expired { | ||
return Err(AddAppointmentFailure::SubscriptionExpired(expiry)); | ||
} | ||
let extended_appointment = ExtendedAppointment::new( | ||
appointment, | ||
user_id, | ||
user_signature, | ||
self.last_known_block_height.load(Ordering::Acquire), | ||
); | ||
let uuid = UUID::new(extended_appointment.locator(), user_id); | ||
if self.responder.has_tracker(uuid) { | ||
log::info!("Tracker for {uuid} already found in Responder"); | ||
return Err(AddAppointmentFailure::AlreadyTriggered); | ||
} | ||
let available_slots = self | ||
.gatekeeper | ||
.add_update_appointment(user_id, uuid, &extended_appointment) | ||
.map_err(|_| AddAppointmentFailure::NotEnoughSlots)?; | ||
// FIXME: There's an edge case here if store_triggered_appointment is called and bitcoind is unreachable. | ||
// This will hang, the request will timeout but be accepted. However, the user will not be handed the receipt. | ||
// This could be fixed adding a thread to take care of storing while the main thread returns the receipt. | ||
// Not fixing this atm since working with threads that call self.method is surprisingly non-trivial. | ||
match self | ||
.locator_cache | ||
.lock() | ||
.unwrap() | ||
.get(&extended_appointment.locator()) | ||
{ | ||
// Appointments that were triggered in blocks held in the cache | ||
Some(dispute_tx) => { | ||
self.store_triggered_appointment(uuid, &extended_appointment, user_id, dispute_tx); | ||
} | ||
// Regular appointments that have not been triggered (or, at least, not recently) | ||
None => { | ||
self.store_appointment(uuid, &extended_appointment); | ||
} | ||
}; | ||
Ok((available_slots, expiry)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not replicate the whole method just to add/remove some logic related to accountability. This can be done with a single method and some conditional tags inside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I applied conditionality to only specified parts here earlier but it required some drastic changes to overcome the errors, that's why I did this, but let me see this again, will try to come up with a solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can discuss the changes, but this kind of replication is a no-go
} else { | ||
log::error!("Cannot add accepted appointment to tower. Unknown tower_id: {tower_id}"); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and add_appointment_receipt
could be made the same function, given they share functionality. In accountable mode, it will require the receipt and in non-accountable it won't
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
#[cfg(not(feature = "accountable"))] | ||
use teos_common::test_utils::{ | ||
generate_random_appointment, get_random_registration_receipt, get_random_user_id, | ||
get_registration_receipt_from_previous, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want the code to be like this:
use teos_common::test_utils::{ generate_random_appointment, #[cfg(feature = "accountable")] get_random_appointment_receipt, get_random_registration_receipt, get_random_user_id, get_registration_receipt_from_previous, };
but this is not the proper way to do it, this gives syntax error, hence I created separate functions at few places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[cfg(feature = "accountable")]
use teos_common::test_utils::get_random_appointment_receipt;
use teos_common::test_utils::{ generate_random_appointment, get_random_registration_receipt, get_random_user_id, get_registration_receipt_from_previous];
#[cfg(feature = "accountable")] | ||
let locator = generate_random_appointment(None).locator; | ||
#[cfg(feature = "accountable")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call add_appointment_receipt
with generate_random_appointment
, you don't need to blocks for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will change the original code, since I didn't changed the architecture, can you please describe this in more detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wont.
#[cfg(feature = "accountable")]
wt_client.add_appointment_receipt(
tower_id,
generate_random_appointment(None).locator,
0,
&get_random_appointment_receipt(tower_sk),
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-arrange the assignments in the tests of this file to avoid having to tag several accountable
/non-accountable
and/our group them in the same block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will deal with the tests after finishing up these issues
watchtower-plugin/src/dbm.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code here is real messy and has way more replication that it needs. But I'll save the comments for a next pass. On top of that, it may be the case that @mariocynicys memory optimization DB PR gets merged before, so it may not be worth discussing what's in here given it may need to be changed anyway
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. |
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. |
Hey I have tried to resolve most issues
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see some progress, but this still needs some work. I've given you examples for all possible code blocks you were struggling with if I'm not mistaken. Feel free to ping me if you need further help
#[derive(Debug, Eq, PartialEq, Clone, Serialize, Deserialize)] | ||
pub struct Locators { | ||
data: [u8; LOCATOR_LEN], | ||
pub locator: Locator, | ||
} | ||
impl Locators { | ||
pub fn from_slice(data: &[u8]) -> Result<Self, &'static str> { | ||
if data.len() < LOCATOR_LEN { | ||
return Err("Data slice too short for Locator"); | ||
} | ||
let locator_data: [u8; LOCATOR_LEN] = data[..LOCATOR_LEN] | ||
.try_into() | ||
.map_err(|_| "Conversion to Locator data failed")?; | ||
let locator = Locator(locator_data); | ||
Ok(Self { | ||
data: locator_data, | ||
locator, | ||
}) | ||
} | ||
pub fn new(txid: Txid) -> Self { | ||
Locators { | ||
data: txid[..LOCATOR_LEN].try_into().unwrap(), | ||
locator: Locator(txid[..LOCATOR_LEN].try_into().unwrap()), | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still here, and it should not be necessary
@@ -121,6 +123,7 @@ pub struct Watcher { | |||
/// The last known block height. | |||
last_known_block_height: AtomicU32, | |||
/// The tower signing key. Used to sign messages going to users. | |||
/// #[cfg(feature = "accountable")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is commented out
@@ -173,6 +175,7 @@ impl Watcher { | |||
/// charge of managing users. | |||
pub(crate) fn register(&self, user_id: UserId) -> Result<RegistrationReceipt, MaxSlotsReached> { | |||
let mut receipt = self.gatekeeper.add_update_user(user_id)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can change the order or this so the #[cfg(feature = "accountable")]
tag is defined only once:
#[cfg(not(feature = "accountable"))]
let receipt = self.gatekeeper.add_update_user(user_id)?;
#[cfg(feature = "accountable")]
{
...
}
#[cfg(feature = "accountable")] | ||
subscription_signature: receipt.signature().unwrap(), | ||
#[cfg(not(feature = "accountable"))] | ||
subscription_signature: "None".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still mandatory. The proto file should be updated so the field is optional. "None" is a string, not an option.
@@ -255,6 +263,58 @@ impl Watcher { | |||
Ok((receipt, available_slots, expiry)) | |||
} | |||
|
|||
#[cfg(not(feature = "accountable"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check how to redefine this method so replication can be minimized. I'm assuming you're copying it because the return type differs from one to another, but most of the code is the same
#[cfg(not(feature = "accountable"))] | ||
pub async fn register( | ||
tower_id: TowerId, | ||
user_id: UserId, | ||
tower_net_addr: &NetAddr, | ||
proxy: &Option<ProxyInfo>, | ||
) -> Result<RegistrationReceipt, RequestError> { | ||
log::info!("Registering in the Eye of Satoshi (tower_id={tower_id})"); | ||
process_post_response( | ||
post_request( | ||
tower_net_addr, | ||
Endpoint::Register, | ||
&common_msgs::RegisterRequest { | ||
user_id: user_id.to_vec(), | ||
}, | ||
proxy, | ||
) | ||
.await, | ||
) | ||
.await | ||
.map(|r: common_msgs::RegisterResponse| { | ||
RegistrationReceipt::new( | ||
user_id, | ||
r.available_slots, | ||
r.subscription_start, | ||
r.subscription_expiry, | ||
) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This remains the same.
.map(|r: common_msgs::RegisterResponse| {
#[cfg(feature = "accountable")]
RegistrationReceipt::with_signature(
user_id,
r.available_slots,
r.subscription_start,
r.subscription_expiry,
r.subscription_signature,
);
#[cfg(not(feature = "accountable"))]
RegistrationReceipt::new(
user_id,
r.available_slots,
r.subscription_start,
r.subscription_expiry,
)
})
#[cfg(feature = "accountable")] | ||
use teos_common::test_utils::{ | ||
generate_random_appointment, get_random_appointment_receipt, | ||
get_random_registration_receipt, get_random_user_id, | ||
}; | ||
|
||
#[cfg(not(feature = "accountable"))] | ||
use teos_common::test_utils::{ | ||
generate_random_appointment, get_random_registration_receipt, get_random_user_id, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYM?
#[cfg(feature = "accountable")]
use teos_common::test_utils::get_random_appointment_receipt;
use teos_common::test_utils::{
generate_random_appointment, get_random_registration_receipt, get_random_user_id,
};
@@ -46,6 +52,15 @@ impl RetryError { | |||
) | |||
} | |||
} | |||
#[cfg(not(feature = "accountable"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl RetryError {
fn is_permanent(&self) -> bool {
#[cfg(not(feature = "accountable"))]
return matches!(
self,
RetryError::Subscription(_, true) | RetryError::Abandoned
);
#[cfg(feature = "accountable")]
matches!(
self,
RetryError::Subscription(_, true) | RetryError::Abandoned | RetryError::Misbehaving(_)
)
}
}
#[cfg(not(feature = "accountable"))] | ||
use teos_common::test_utils::{ | ||
generate_random_appointment, get_random_registration_receipt, get_random_user_id, | ||
get_registration_receipt_from_previous, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[cfg(feature = "accountable")]
use teos_common::test_utils::get_random_appointment_receipt;
use teos_common::test_utils::{ generate_random_appointment, get_random_registration_receipt, get_random_user_id, get_registration_receipt_from_previous];
#[cfg(feature = "accountable")] | ||
let locator = generate_random_appointment(None).locator; | ||
#[cfg(feature = "accountable")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wont.
#[cfg(feature = "accountable")]
wt_client.add_appointment_receipt(
tower_id,
generate_random_appointment(None).locator,
0,
&get_random_appointment_receipt(tower_sk),
);
Hey I have seen the provided examples for fixing the code, implementing it now. |
Closing due to inactivity. This was attempted but never fully implemented. |
Could it be useful for future trials? maybe draft it instead? |
I'll reference it in the original issue in case someone wants to pick up where this was left |
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:
Task-completed:
Compilation Instructions:
cargo build --features notAccountable
cargo install --locked --path watchtower-plugin --features notAccountable
teosd
now you can use client endpoints that are customised to run in this mode.