Add first persistence in client #268
Conversation
oxade
requested review from
huitseeker,
lxfind,
patrickkuo,
sblackshear,
gdanezis and
arun-koshy
| @@ -151,17 +158,18 @@ impl<A> ClientState<A> { | |||
| .iter() | |||
| .map(|(id, (_, seq, _))| (*id, *seq)) | |||
| .collect(); | |||
| let path = env::temp_dir().join(format!("DB_{:?}", ObjectID::random())); | |||
There was a problem hiding this comment.
random? shouldn't this read and write to the same path?
There was a problem hiding this comment.
Yes this is mentioned as part of the TODOs once we add state recovery
There was a problem hiding this comment.
tempfile is a good Rust crate to save you from creating a filename from an object ID.
| @@ -151,17 +158,18 @@ impl<A> ClientState<A> { | |||
| .iter() | |||
| .map(|(id, (_, seq, _))| (*id, *seq)) | |||
| .collect(); | |||
| let path = env::temp_dir().join(format!("DB_{:?}", ObjectID::random())); | |||
There was a problem hiding this comment.
tempfile is a good Rust crate to save you from creating a filename from an object ID.
| use typed_store::rocks::{open_cf, DBMap}; | ||
| use typed_store::traits::Map; | ||
|
|
||
| const PENDING_TRANSFER_KEY: &str = "PENDING_TRANSFER_KEY"; |
There was a problem hiding this comment.
I assumed renaming "transfer" to "transaction" is part of the planned terminological refactor (since orders can do much more than transfer!)? Even if we are holding off on that refactor for now, let's avoid introducing new instances of this confusing term that we'll need to change later.
There was a problem hiding this comment.
@patrickkuo We'll need to extend the use of this flag to other operations once I change it to PENDING_TRANSACTION
| impl ClientStore { | ||
| /// Open an client store by directory path | ||
| pub fn open<P: AsRef<Path>>(path: P, db_options: Option<Options>) -> ClientStore { | ||
| let db = open_cf(&path, db_options, &["pending_transfer"]).expect("Cannot open DB."); |
There was a problem hiding this comment.
What does the string pending_transfer do here + below?
There was a problem hiding this comment.
Looks like the name of the column family. This was adapter form authority_store.
However I think we can remove it for this trivial case as Rocks DB defaults to 'default' column family
| .expect("Cannot open pending_transfer CF."), | ||
| } | ||
| } | ||
| // Get the pending tx if any |
There was a problem hiding this comment.
Nit: doc comments on all public functions
| .get(&PENDING_TRANSFER_KEY.to_string()) | ||
| .map_err(|e| e.into()) | ||
| } | ||
| // Set the pending tx if any |
There was a problem hiding this comment.
| // Set the pending tx if any | |
| // Set the pending tx |
Should also specify what happens if you try to set and there's already a tx there.
| .insert(&PENDING_TRANSFER_KEY.to_string(), order) | ||
| .map_err(|e| e.into()) | ||
| } | ||
| pub fn clear_pending_transfer(&self) -> Result<(), FastPayError> { |
There was a problem hiding this comment.
Comment should specify what happens if you try to clear and there's nothing there.
| @@ -724,14 +728,15 @@ where | |||
| with_confirmation: bool, | |||
| ) -> Result<CertifiedOrder, anyhow::Error> { | |||
| ensure!( | |||
| self.pending_transfer == None || self.pending_transfer.as_ref() == Some(&order), | |||
| self.store.get_pending_transfer()?.is_none() | |||
There was a problem hiding this comment.
We probably don't want to persist pending_tx, this field should be transient.
There was a problem hiding this comment.
What if the client dies before this is complete or before it clears the pending state?
IIRC, operations are idempotent so it's okay to re-apply the same thing on wake-up.
There was a problem hiding this comment.
this will actually cause a problem, pending transfer is unset when a transaction completed, there will be no way to unset it after restart, and will prevent client from starting any new transaction.
There was a problem hiding this comment.
But the pending transfer returns the order it is pending on
So the client can simply retry the order and carry on, which is a one-time thing
How does this block future orders?
There was a problem hiding this comment.
if we have logic to re-submit pending transfer on restart then it will carry on, or else it will fail on this line, but the question is do we really want to do that automatically or let client decide how to recover from shutdown.
I think the main function of pending_transfer is to stop more than one transaction running at the same time to prevent race condition, that's why I think it's transient.
- responds to an invalid serialized batch, - closes #268
- responds to an invalid serialized batch, - closes MystenLabs#268
First piece, part of #257
The goal here is to move towards persisting client state and objects, and hopefully be able to recover a client after a crash without too much syncing.
This PR starts by adding some basic storage logic and a simple use-case which checks for pending transfer.
Overhauling the client to support persistence and recovery was reaching thousands of lines, so I chunked it out into small parts to avoid conflicting Patrick's work.
Next steps: