[fastx client] Adding robust client <-> authorities primitives #336

gdanezis · 2022-02-01T21:54:49Z

In this PR we build a library of primitives for robust interactions between the client and the set of authorities, covering failure cases in the general model of orders beyond transfers.

Specifically:

[Client logic]

We include a generic map/reduce pattern for interacting with authorities, in which we provide an async function to query each authority, and then a reduce function to aggregate responses, and adaptively end, continue or timeout the queries.
We use the abstraction to retrieve objects at all versions that are know by the authorities to be owned by an address.
We use the abstraction to retrieve all versions of an object known by the authorities and associated certificates.
We use the abstraction to update authorities to the latest state of objected owned by an address using the latest certs.

[Authority logic]

Augmented the authority with an object version in parent_sync when the object is deleted, and a link back to the cert that deleted it.
Changed the semantics of ObjectInfoRequest to return the latest ObjectRef for an object id, and the certificate that leads to it, including for deleted objects.

For another PR:

Record and propagate errors in a suitable way, probably send back to authority client logic.
Use shared cached state instead of queries where possible -- right now we are very chatty.

gdanezis · 2022-02-02T19:23:56Z

Hey @patrickkuo -- I promised to make this available for review, but I am still trying to finalise the sync_all_owned_objects to be correct is objects are either deleted or transferred to other users. Its taking longer than I expected, sorry about the delay.

patrickkuo

just some nits, otherwise LGTM

fastpay_core/src/client.rs

fastpay_core/src/unit_tests/client_tests.rs

huitseeker

Thanks! This looks like an interesting direction. A couple of comments with this first pas of review:

unit tests for get_object_by_id, get_all_owned_objects, sync_all_owned_objects would help .. or at least smaller tests. The tests that are here (thank you!) are integration tests. They're a journey, I would love something more bite-sized for daily reading.
anything that lowers the complexity of quorum_map_then_reduce_with_timeout is good. I left a few suggestions, but I'll try to help more on the 2d pass, especially with the reduce

huitseeker · 2022-02-04T13:42:30Z

fastpay_core/src/client.rs

+                    )
+                    .await;
+
+                // TODO: log or report the errors if there is one.


tracing allows you to de-couple the logging from the publishing of the logs, which means you can leave it to a further PR to actually figure out how to display things, and actually get on with figuring out interesting messages to communicate.

I wonder: is there something we could do to make the bar to entry easier and make sure logging / tracing is not left as a TODO in general?

Tracing communicates things to the devops person baby sitting a server, or user process (actually logs on the user side are usually for debug, no user should look there). Here what needs doing is collect errors, and send them to the part of the client that can do something about them: ie deprioritize interactions with this authority, etc.

I changed the comment to:

// TODO: collect errors and propagate them to the right place

Your reflection on how to systematically log is still very valid.

fastpay_core/src/authority.rs

fastpay_core/src/authority/authority_store.rs

fastpay_core/src/client.rs

huitseeker · 2022-02-04T14:44:39Z

fastpay_core/src/client.rs

+    /// This function provides a flexible way to communicate with a quorum of authorities, processing and
+    /// processing their results into a safe overall result, and also safely allowing operations to continue
+    /// past the quorum to ensure all authorities are up to date (up to a timeout).


A doc example here would increase the usability of the function, especially if it shows how to make use of timeouts.

I now have quite a few examples in the unit tests, including using timeouts. We can try to adapt them to a doc example (I do not know how to set up the scaffolding as we do in tests for docs).

The scaffolding is there already, it's a Rust built-in, cargo test will run your doc tests at the end.

fastpay_core/src/client.rs

fastpay_core/src/unit_tests/client_tests.rs

fastx_types/src/base_types.rs

gdanezis · 2022-02-04T15:49:12Z

Many thanks for the reviews @patrickkuo & @huitseeker -- you have identified some key issues to fix. Will do and respond by tomorrow.

fastpay_core/src/client.rs

fastpay_core/src/unit_tests/authority_tests.rs

oxade · 2022-02-05T07:35:20Z

fastpay_core/src/client.rs

+        (
+            BTreeMap<
+                (ObjectRef, TransactionDigest),
+                (Option<Object>, Vec<(AuthorityName, Option<SignedOrder>)>),


We might want to pass around the reference to the Object here instead of the whole object since they could be arbitrarily huge.
(in the meantime, Sam and I are trying to define a reasonable upper bound for object sizes)

The objects can be huge because they contain large allocations in the heap. However, moving an Object around (except if we clone or if it implements the copy trait) does not actually copy the allocation, but just the fixed size stack allocated part I think.

fastpay_core/src/client.rs

gdanezis · 2022-02-05T11:08:07Z

@huitseeker I have added unit tests as you suggested, but for quorum_map_then_reduce_with_timeout. To have effective unit (rather than integration) tests for get_object_by_id, get_all_owned_objects, sync_all_owned_objects, we will need to create a more expressive mocking framework around LocalAuthorityClient to induce specific responses, errors, long delays, etc. I want this, but its a bigger piece than this PR.

Do we have experience, or should we use something like: https://docs.rs/mockall/latest/mockall/ ?

lxfind

There may be an issue here:
When we delete an object by wrapping it to another object, we insert (obj_id, seq+1, ..) to parent_sync, but we don't actually mutate the sequence number of the object (it stays at seq). When that same object gets unwrapped, its sequence number becomes seq+1 and now parent_sync has two entries for the same object_id + seq.

huitseeker

To @lxfind's comment: do we not clobber the original deleted object recording when unwrapping?

huitseeker · 2022-02-06T13:02:14Z

fastpay_core/src/client.rs

            if let Ok(response) = result {
-                let certificate = response
-                    .parent_certificate
-                    .expect("Unable to get certificate");
+                let certificate = if let Some(certificate) = response.parent_certificate {
+                    certificate
+                } else {
+                    continue;
+                };
+


if let Ok(ObjectInfoResponse { parent_certificate: Some(certificate), .. }) = result {

huitseeker · 2022-02-06T13:05:26Z

fastpay_core/src/client.rs

+                    // NOTE: This implies this is a genesis object. We should check that it is.
+                    //       We can do this by looking into the genesis, or the object_refs of the genesis.
+                    //       Otherwise report the authority as potentially faulty.


This is probably calling for a specific is_genesis function somewhere?

Yes it does. It also calls for checking all information the client downloads from an authority, something that is done in at best an ah-hoc way right now, if at all.

huitseeker · 2022-02-06T13:07:27Z

fastpay_core/src/client.rs

+                            if total_stake.1 > validity {
+                                return Err(FastPayError::TooManyIncorrectAuthorities);


This is very underwhelming. 😢

Sorry I do not know what to do about this. If its the name that is underwhelming, we can change it to something else. If the amount of reporting is underwhelming, then it will have to be a separate PR. We cannot block all progress on the known issue that error reporting on the client is lacking.

Yes, for sure, there's no blocking issue here.

Remove annotation Implemented get_object_by_ref Download objects at version Added tests and fix corner cases Return certificate with retrieved object Added docs Finished sync_all_owned_objects Docs Return available objects Add an assert that we only side insert genesis objects Added a an authority test with a deleted object Add deleted objects to the parent index Added get_latest_parent_entry & tests ObjectInfoResponse sends the latest certificate Added requested_object_reference to ObjectInfoResponse Fixed bug in query() Fix sync_all_owned_objects Added tests for the client full sync Make fmt and clippy happy Clean up comments Removed unused function Make fmt happy Add docs Clean up tests Removed wrapped Remove s.truncate Fixed rebase errors Make get_object_by_id return pending orders clippy + fmt Changes followin review Added unit tests for map/reducer

gdanezis · 2022-02-06T15:38:15Z

There may be an issue here: When we delete an object by wrapping it to another object, we insert (obj_id, seq+1, ..) to parent_sync, but we don't actually mutate the sequence number of the object (it stays at seq). When that same object gets unwrapped, its sequence number becomes seq+1 and now parent_sync has two entries for the same object_id + seq.

Yes, this issue is very real, and either we have to deal with it by incrementing the version correctly, or implementing the lamport timestamps. Either should work.

gdanezis marked this pull request as draft February 1, 2022 21:55

gdanezis force-pushed the update-client-authority-interactions branch from 9bb93c5 to 14bf67a Compare February 2, 2022 12:50

gdanezis force-pushed the update-client-authority-interactions branch from 662edd5 to 3643466 Compare February 2, 2022 20:02

This was referenced Feb 4, 2022

[Client Refactoring 1] Extract authority missing certificate update out of order handling pipeline #356

Merged

[Client Refactoring 2] Add a stateless version of execution_transaction #360

Merged

gdanezis changed the title ~~[fastx client] Adding robust client <-> authorities primitives (draft)~~ [fastx client] Adding robust client <-> authorities primitives Feb 4, 2022

gdanezis force-pushed the update-client-authority-interactions branch from 5d8ca5c to dbdf6b2 Compare February 4, 2022 11:17

gdanezis marked this pull request as ready for review February 4, 2022 11:22

gdanezis mentioned this pull request Feb 4, 2022

[Client Refactoring 4] Rename download_own_object_ids #362

Merged

patrickkuo approved these changes Feb 4, 2022

View reviewed changes

huitseeker reviewed Feb 4, 2022

View reviewed changes

gdanezis mentioned this pull request Feb 4, 2022

[fastx all systems] Implement Display For FastX Data Structures #255

Closed

oxade reviewed Feb 5, 2022

View reviewed changes

fastpay_core/src/client.rs Outdated Show resolved Hide resolved

oxade reviewed Feb 5, 2022

View reviewed changes

fastpay_core/src/unit_tests/authority_tests.rs Outdated Show resolved Hide resolved

oxade reviewed Feb 5, 2022

View reviewed changes

fastpay_core/src/unit_tests/authority_tests.rs Show resolved Hide resolved

oxade reviewed Feb 5, 2022

View reviewed changes

fastpay_core/src/client.rs Outdated Show resolved Hide resolved

gdanezis force-pushed the update-client-authority-interactions branch from 3e36713 to 3f9362e Compare February 5, 2022 10:30

lxfind reviewed Feb 6, 2022

View reviewed changes

huitseeker reviewed Feb 6, 2022

View reviewed changes

gdanezis force-pushed the update-client-authority-interactions branch from 7b56a00 to ee0d7f7 Compare February 6, 2022 15:36

Make query() better

c00592e

This was referenced Feb 6, 2022

[fastx adaptor] Increase the version number of a wrapped object to not conflict with the deletion record when it was wrapped #376

Closed

[fastx client] Improve the quorum_map_then_reduce_with_timeout function #377

Closed

gdanezis merged commit db9ee35 into main Feb 6, 2022

gdanezis deleted the update-client-authority-interactions branch February 6, 2022 16:21

snyk-bot mentioned this pull request Apr 16, 2023

[Snyk] Upgrade swagger-ui-express from 4.3.0 to 4.6.2 mchern/sui#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fastx client] Adding robust client <-> authorities primitives #336

[fastx client] Adding robust client <-> authorities primitives #336

gdanezis commented Feb 1, 2022 •

edited

Loading

gdanezis commented Feb 2, 2022

patrickkuo left a comment

huitseeker left a comment

huitseeker Feb 4, 2022

gdanezis Feb 5, 2022 •

edited

Loading

huitseeker Feb 4, 2022

gdanezis Feb 5, 2022

huitseeker Feb 5, 2022

gdanezis commented Feb 4, 2022

oxade Feb 5, 2022

gdanezis Feb 5, 2022 •

edited

Loading

gdanezis commented Feb 5, 2022 •

edited

Loading

lxfind left a comment

huitseeker left a comment

huitseeker Feb 6, 2022

huitseeker Feb 6, 2022

gdanezis Feb 6, 2022

huitseeker Feb 6, 2022

gdanezis Feb 6, 2022

huitseeker Feb 6, 2022

gdanezis commented Feb 6, 2022

		if total_stake.1 > validity {
		return Err(FastPayError::TooManyIncorrectAuthorities);

[fastx client] Adding robust client <-> authorities primitives #336

[fastx client] Adding robust client <-> authorities primitives #336

Conversation

gdanezis commented Feb 1, 2022 • edited Loading

gdanezis commented Feb 2, 2022

patrickkuo left a comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdanezis Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdanezis commented Feb 4, 2022

Choose a reason for hiding this comment

gdanezis Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

gdanezis commented Feb 5, 2022 • edited Loading

lxfind left a comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gdanezis commented Feb 6, 2022

gdanezis commented Feb 1, 2022 •

edited

Loading

gdanezis Feb 5, 2022 •

edited

Loading

gdanezis Feb 5, 2022 •

edited

Loading

gdanezis commented Feb 5, 2022 •

edited

Loading